Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mqtt_client_test #498

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

ellebi2000
Copy link

@ellebi2000 ellebi2000 commented May 25, 2024

This PR introduces functionality for temperature monitoring and LED control on a Raspberry Pi Pico W using MQTT. The key features include:

Temperature Monitoring:

Reads onboard temperature using the ADC.
Converts ADC readings to Celsius or Fahrenheit.
Publishes temperature to the /temperature MQTT topic every 5 seconds.

LED Control:

Controls the onboard LED via MQTT messages received on the /led topic.
Turns the LED on/off based on "On" or "Off" messages.
Publishes LED state to the /state MQTT topic.

WiFi and MQTT Configuration:

Connects to WiFi using predefined SSID and password.
Connects to the MQTT broker with the given IP address and port.
Configures an MQTT client with a predefined client ID and keep-alive period.

MQTT Callbacks:

mqtt_incoming_data_cb: Handles incoming data and controls the LED.
mqtt_incoming_publish_cb: Handles incoming publish events.
mqtt_connection_cb: Manages MQTT connection status and subscribes to the /led topic.

strncpy(mqtt_client->data, data, len);
mqtt_client->len = len;
mqtt_client->data[len] = '\0';
//Stampo i messaggi e i topic

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't mean to be picky, but I'm going through your code to include mqtt with my project and noticed this comment code all messed up

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's in Italian. It was a comment written during development, and I didn't notice it when reviewing. I'll remove it in the next commit, sorry.

@martincarapia
Copy link

Everything checks out. I just tested it and the code runs great!
image
Just those little nitpicks and it's good to be approved!

@francdoc
Copy link
Contributor

francdoc commented Jun 1, 2024

This implementation is working great. I tested your publisher sample with a subscriber from this repository (running on a Linux machine) simple_subscriber.c.

I configured your code to publish to the address "35.156.49.117" (broker.hivemq.com) on port 1883, and the MQTT-C subscriber successfully listened to the transmitted temperature data.

Thank you, ellebi2000.

@ellebi2000
Copy link
Author

I have removed the comment in Italian and others that are not strictly necessary.
I'm very happy that your implementations are working as expected @martincarapia @francdoc

@@ -0,0 +1,161 @@
//
// Created by elliot on 25/05/24.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment clarifying the license that this code is released under please?

Comment on lines +16 to +17
#define WIFI_SSID "YOUR_WIFI_SSID"
#define WIFI_PASSWORD "YOUR_WIFI_PASSWD"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lurch
Copy link
Contributor

lurch commented Jul 5, 2024

Perhaps this should be in pico_w/wifi/mqtt instead of pico_w/mqtt ?

EDIT: Also, do you really need separate mqtt and mqtt/mqtt_client directories? 🤔 Perhaps it could be "collapsed" to just a single pico_w/wifi/mqtt_client directory?

}

void control_led(bool on) {
// Public state on /state topic and on/off led board
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a few code-comments where you've said "Public blah blah topic" - were these supposed to say "Publish blah blah topic" ?

Comment on lines +76 to +77
}
static void mqtt_incoming_data_cb(void *arg, const u8_t *data, u16_t len, u8_t flags) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line between functions please 🙂 (both here and in a few other places)


static void mqtt_incoming_publish_cb(void *arg, const char *topic, u32_t tot_len) {
MQTT_CLIENT_DATA_T* mqtt_client = (MQTT_CLIENT_DATA_T*)arg;
strcpy(mqtt_client->topic, topic);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to use strncpy here? 🤷

Comment on lines +156 to +157
sleep_ms(5000);
tight_loop_contents();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to do tight_loop_contents() if you're already doing sleep_ms(5000) on the previous line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants