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

Draft PR to get help with MQTT OTA update issue #4146

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JacobK5
Copy link

@JacobK5 JacobK5 commented Sep 17, 2024

Making this draft PR so I can get help with an issue I'm running into while trying to add remote OTA updates over mqtt.

I can't show my full codebase, but these should be all the relevant changes for my OTA issue

I can't show my full codebase, but these should be all the relevant changes for my OTA issue
@JacobK5 JacobK5 changed the title Made Relevant Changes From My Own Codebase Draft PR to get help with MQTT OTA update issue Sep 17, 2024
@blazoncek
Copy link
Collaborator

Dear Lord.... 😄

Please undo all irrelevant/formatting changes if you want any advice.

@softhack007
Copy link
Collaborator

I agree with @blazoncek - I see lots of styling, but still no clue what your problem is about.

I can't show my full codebase, but these should be all the relevant changes for my OTA issue

If you can't show your codebase (closed source) then we need to know at least if this PR provides a feature that will be useful in open source WLED?

platformio.ini Outdated
@@ -11,7 +11,7 @@

# CI binaries
; default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, esp32dev, esp32_eth # ESP32 variant builds are temporarily excluded from CI due to toolchain issues on the GitHub Actions Linux environment
default_envs = nodemcuv2, esp8266_2m, esp01_1m_full, nodemcuv2_compat, esp8266_2m_compat, esp01_1m_full_compat, nodemcuv2_160, esp8266_2m_160, esp01_1m_full_160, esp32dev, esp32_eth, esp32dev_audioreactive, lolin_s2_mini, esp32c3dev, esp32s3dev_8MB, esp32s3dev_8MB_PSRAM_opi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo all changes to platformIO.ini.

@softhack007 softhack007 added the rebase needed This PR needs to be re-based to the current development branch label Sep 17, 2024
@JacobK5
Copy link
Author

JacobK5 commented Sep 21, 2024

Dear Lord.... 😄

Please undo all irrelevant/formatting changes if you want any advice.

Yeah my bad, didn't think that through at all, that was really dumb. I've now redone the changes but without any of the formatting changes.

@netmindz
Copy link
Collaborator

Have you actually managed to get this to run? When I've previously tried something similar I ran into resource issues?

@@ -58,6 +65,9 @@ void onMqttMessage(char* topic, char* payload, AsyncMqttClientMessageProperties
DEBUG_PRINT(F("MQTT msg: "));
DEBUG_PRINTLN(topic);

// set connected to true. If we got a message, we must be connected (this fixes a lot of issues with AsyncMqttClient)
mqtt->setConnected(true); // note that setConnected is a function that I added to AsyncMqttClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like a hack. Can you explain what issues you saw with the connected state?

Copy link
Author

Choose a reason for hiding this comment

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

I often received messages but wasn't able to send a response because the client said it was disconnected, even though it clearly was connected since it got the messages successfully. This fixed that issue for me. It definitely is a bit hacky though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI I will veto this change (and corresponding AsyncMqttClient change) as it is inappropriate unless proved (with traces) MQTT client isn't working correctly.

DEBUG_PRINTLN(F("Initializing MQTT"));
// set the important variables
mqttEnabled = true;
strlcpy(mqttServer, "server address here", MQTT_MAX_SERVER_LEN + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Surely these connection values are already defined as part of WLED or you wouldn't be able to use it all. No need to add a new compile time value?

DEBUG_PRINTLN(url);

// make client for HTTP request
HTTPClient http;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this also handle HTTPS?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't tested that out, it might be worth trying

Copy link
Collaborator

Choose a reason for hiding this comment

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

Releases from GitHub are HTTPS only, so this is going to be needed for most users

}
else
{
DEBUG_PRINT(F("Update failed, only wrote : "));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most users will not be running a debug build, so I think that at least the key messages should also be sent as m via MQTT.

It looks like you started doing this and then just swapped to debug only

Copy link
Author

Choose a reason for hiding this comment

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

That's fair. I wanted to avoid sending mqtt messages at any point in this process until I got it working first just to make sure that isn't causing any issues.

if (otaInProgress)
{
// stop the loop while ota in progress
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect it's this change that is causing you the issues with the MQTT clients connection state.

This feels too heavy handed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I concur.

Copy link
Author

Choose a reason for hiding this comment

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

This is unrelated to the connection state issue, I had those issues well before I tried adding this.

@@ -461,8 +466,10 @@ pinManager.allocateMultiplePins(pins, sizeof(pins)/sizeof(managed_pin_type), Pin
// fill in unique mdns default
if (strcmp(cmDNS, "x") == 0) sprintf_P(cmDNS, PSTR("wled-%*s"), 6, escapedMac.c_str() + 6);
#ifndef WLED_DISABLE_MQTT
if (mqttDeviceTopic[0] == 0) sprintf_P(mqttDeviceTopic, PSTR("wled/%*s"), 6, escapedMac.c_str() + 6);
if (mqttClientID[0] == 0) sprintf_P(mqttClientID, PSTR("WLED-%*s"), 6, escapedMac.c_str() + 6);
if (mqttDeviceTopic[0] == 0) sprintf_P(mqttDeviceTopic, PSTR("lights/%*s"), 6, escapedMac.c_str() + 6);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like changes from your own closed source code leaking in here. Revert

#endif
WLED_GLOBAL AsyncMqttClient *mqtt _INIT(NULL);
WLED_GLOBAL bool mqttEnabled _INIT(false);
WLED_GLOBAL char mqttStatusTopic[40] _INIT(""); // this must be global because of async handlers
WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN+1] _INIT(""); // main MQTT topic (individual per device, default is wled/mac)
WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN+1] _INIT(""); // main MQTT topic (individual per device, default is lights/mac)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated changes

@@ -757,6 +759,11 @@ WLED_GLOBAL int8_t spi_sclk _INIT(SPISCLKPIN);
WLED_GLOBAL StaticJsonDocument<JSON_BUFFER_SIZE> doc;
WLED_GLOBAL volatile uint8_t jsonBufferLock _INIT(0);

// global buffers for mqtt responses
WLED_GLOBAL StaticJsonDocument<JSON_BUFFER_SIZE> mqttResponseDoc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really the first time we are sending a response via MQTT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

MQTT can receive JSON but is not responding in JSON. I would still re-use existing global buffer if possible.

This is a heavy toll on available RAM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Concur. There's no need for a second buffer -- the JSON response is being generated after the incoming message is processed, so the buffer can be cleared and re-used without releasing the lock.

#endif
WLED_GLOBAL AsyncMqttClient *mqtt _INIT(NULL);
WLED_GLOBAL bool mqttEnabled _INIT(false);
WLED_GLOBAL char mqttStatusTopic[40] _INIT(""); // this must be global because of async handlers
WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN+1] _INIT(""); // main MQTT topic (individual per device, default is wled/mac)
WLED_GLOBAL char mqttDeviceTopic[MQTT_MAX_TOPIC_LEN+1] _INIT(""); // main MQTT topic (individual per device, default is lights/mac)
WLED_GLOBAL char mqttResponseTopic[MQTT_MAX_TOPIC_LEN + 1] _INIT(""); // MQTT response topic (individual per device, default is lights/mac/r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Best practice for service-like calls in a pub-sub framework is to accept the response topic as part of the request. Apart from removing any possibility of a misconfigured server, it allows the client to specify a unique ephemeral topic for each transaction, ensuring that you don't ever accidentally receive a reply from some other client's service call.

@JacobK5
Copy link
Author

JacobK5 commented Sep 23, 2024

Have you actually managed to get this to run? When I've previously tried something similar I ran into resource issues?

It runs for me and always freezes after about 100kb are downloaded. The free heap looks good at that point, so to me it looks like there isn't a resource issue and that the issue has to do with the connection being interrupted by some other process, but I could be missing something.

@JacobK5
Copy link
Author

JacobK5 commented Sep 23, 2024

Just for the record for anybody seeing this now, this was not originally intended to be an actual pull request for changes to be made to WLED. I was having issues with my closed source project that I used WLED for and asked for help in the forums, and was asked to make a draft PR here so they could see my code. That being said, adding remote OTA updates over MQTT likely would be a useful addition to WLED.

@blazoncek
Copy link
Collaborator

this was not originally intended to be an actual pull request for changes to be made to WLED

FYI getting help for a closed source project form open source developers requires some sacrifice. PR is a minimum that you can provide as a way of "thank you". Even if it is not merged in the end. It is contribution to knowledge and ideas that count.

@netmindz
Copy link
Collaborator

Have you actually managed to get this to run? When I've previously tried something similar I ran into resource issues?

It runs for me and always freezes after about 100kb are downloaded. The free heap looks good at that point, so to me it looks like there isn't a resource issue and that the issue has to do with the connection being interrupted by some other process, but I could be missing something.

I'm not sure I would call "freezes after about 100kb are downloaded" as running successfully 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question rebase needed This PR needs to be re-based to the current development branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants