-
Notifications
You must be signed in to change notification settings - Fork 135
Refactored mqtt feature. #971
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
Conversation
- made it a microTask - Mongoose MQTT client callbacks are set up to call methods of the Mqtt instance (using lambdas to capture this). - Error Handling in attemptConnection: The _mqttclient.connect() now has a non-blocking callback for success. If connect() itself returns false, it means the connection attempt couldn't even be initiated (e.g., invalid parameters before network activity). This case is now handled by calling onMqttDisconnect. ( should fix OpenEVSE#969 ) - checkAndPublishUpdates() method consolidates the logic for periodically checking if any state (claims, config, etc.) has changed and needs to be published. The publishInitialState() is called upon connection to send the current state of all relevant items. - notify...Changed() methods. These should be called by the respective modules (Config, EvseManager, Manual, Scheduler, Limit) when their data changes, allowing MqttTask to publish the updates promptly if connected, or upon next connection. This is more event-driven than purely polling versions in its own loop, though the loop still serves as a fallback and for connection management. TODO: use this in related modules and remove mqtt versions check
|
Seems there's a problem with compiling the divert_sim: I couldn't check why yet. |
|
I have set the mqtt microtask loop to 50ms What do you think is the most appropriate value? |
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactors the MQTT feature into a MicroTasks-based Mqtt class and replaces legacy global mqtt_* calls with mqtt. instance methods.
- Converts
mqtt.hto define anMqtttask class and removes C-style extern functions. - Updates all code sites to use
mqtt.publishXxx()andmqtt.isConnected()instead ofmqtt_*globals. - Cleans up legacy includes, adjusts setup/loop in
main.cpp, and fixes a typo in override error messages.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/web_server.cpp | Swapped global mqtt_* calls for mqtt.publishOverride(), fixed typo, remove stale TODO |
| src/scheduler.cpp | Replaced mqtt_publish_schedule() with mqtt.publishSchedule() |
| src/mqtt.h | Converted to Mqtt class, removed externs, missing ArduinoJson include |
| src/main.cpp | Added mqtt.begin(), removed mqtt_loop(), switched to mqtt.publishData() |
| src/current_shaper.h | Removed unused #include "mqtt.h" |
| src/app_config_mqtt.h | Removed unused #include "mqtt.h" |
| src/app_config_mode.h | Removed unused #include "mqtt.h" |
| src/app_config.cpp | Swapped mqtt_connected() and mqtt_restart() for mqtt.isConnected() and mqtt.restartConnection() |
Comments suppressed due to low confidence (3)
src/mqtt.h:4
- The header declares
publishData(JsonDocument &data)but doesn't include<ArduinoJson.h>, which is needed forJsonDocument.
#include <MongooseMqttClient.h>
src/mqtt.h:63
- The comment says the handler is made static, but
handleMqttMessageis a non-static member. Update or remove the outdated comment.
// Made static because MongooseMqttClient might need a C-style function pointer
src/web_server.cpp:668
- This TODO and commented-out call is now stale. Either implement the new
mqtt.publishLimit()call or remove the comment to keep the code clean.
// todo: mqtt_publish_limit(); // update limit props to mqtt
|
Gave this a test and looks to work, @KipK do you think it is good to merge? |
It runs nows for few weeks without any trouble and fix the mqtt reconnection bug. Looks all good to me |
Refactored mqtt feature.
TODO: use notify_xxx_changed() methods in related modules and remove mqtt versions check