Add Home Assistant integration #87
Conversation
Docs
====
- #{platform} = ml-
- MQTT: add setup instructions
Backend
=======
- SettingValue: PLATFORM = "ml-"
- Lights Control: add readMQTT and updateMQTT, add Mqtt endpoint, client and settingsservice and registerConfig
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Docs docs/buildprocess.md, docs/statefulservice.md, docs/develop/sveltekit.md, docs/gettingstarted/installation.md, docs/network/mqtt.md, docs/moonbase/devices.md |
Replace platform/mDNS wording with ml/ml- placeholders and lowercase examples; remove/adjust FACTORY_AP_SSID examples; add Home Assistant MQTT setup instructions and update image/source. |
Factory & Config Defaults factory_settings.ini, platformio.ini |
Replace ML-#{unique_id} with #{platform}-#{unique_id} (platform = ml); change default MQTT URI to mqtt://homeassistant.local:1883; change default status topic and bump APP_DATE. |
Framework Settings & Constants lib/framework/SettingValue.cpp, lib/framework/EthernetSettingsService.h, lib/framework/MqttSettingsService.h, lib/framework/WiFiSettingsService.cpp |
Set PLATFORM constant to "ml"; update default hostname macro to #{platform}-#{unique_id}; change default MQTT status topic to light/#{unique_id}/status; normalize hostname examples to lowercase. |
MQTT Endpoint Note lib/framework/MqttEndpoint.h |
Add inline comment about QoS for Home Assistant / Mosquitto (suggest QoS 1). |
ModuleLightsControl (MQTT / HA integration) src/MoonLight/Modules/ModuleLightsControl.h |
Add Home Assistant discovery/state integration: new free functions readMQTT() and updateMQTT(); add _mqttEndpoint, _mqttClient, _mqttSettingsService, and settings handler id members; add onMqttSettingsChanged(), initializeMqtt(), cleanupMqtt(), registerConfig(); bind MQTT onConnect to publish discovery, subscribe command topics, and publish availability/state. |
UI Field Key Rename interface/src/lib/components/moonbase/FieldRenderer.svelte, src/MoonBase/Modules/ModuleDevices.h |
Rename field/type key from mdnsName → mDNSName to align UI rendering and device config. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant HA as Home Assistant
participant Broker as MQTT Broker
participant ModLight as ModuleLightsControl
participant MqttClient as PsychicMqttClient
participant Settings as MqttSettingsService
Note over ModLight,MqttClient: Initialization
ModLight->>Settings: read MQTT settings
ModLight->>MqttClient: ctor/init (attach endpoint & onConnect)
Note over MqttClient,Broker: MQTT connect
MqttClient->>Broker: CONNECT (mqtt://homeassistant.local:1883)
Broker-->>MqttClient: CONNACK
MqttClient->>ModLight: onConnect callback
rect rgb(230,245,255)
ModLight->>ModLight: registerConfig()
ModLight->>Broker: PUBLISH discovery topic (homeassistant/...)
ModLight->>Broker: SUBSCRIBE command topic
ModLight->>Broker: PUBLISH availability/status
end
HA->>Broker: READ discovery topic
Broker->>HA: discovery payload
HA->>HA: register entity
rect rgb(220,255,220)
ModLight->>Broker: publish state (JSON via readMQTT)
Broker->>HA: state update
end
rect rgb(255,240,220)
HA->>Broker: publish command (on/off/brightness)
Broker->>ModLight: deliver command
ModLight->>ModLight: updateMQTT(JSON) -> apply ModuleState
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
- Board presets p4 #74: Modifies
ModuleLightsControl— overlaps with MQTT/discovery additions in this PR. - rename UI components #75: Touches
FieldRenderer.svelte— related to themdnsName→mDNSNamerename. - V070 preps #79: Also changes
ModuleLightsControlinternals (pin/read logic) and may interact with the new MQTT state flow.
Suggested labels
enhancement
Poem
🐰 I hopped through docs and topics under moonlit air,
I tuckedmlinto hostnames and made the topics fair.
I send small JSON letters and listen for command,
A rabbit's gentle patch to help devices understand.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Add Home Assistant integration' accurately reflects the main change—MQTT functionality enabling Home Assistant control, which is the primary feature across all modified files. |
✨ Finishing touches
- 📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
docs/buildprocess.md (1)
104-104: Consider improving placeholder documentation clarity.The updated description
"ml- (was The microcontroller platform) 🌙"is less clear than the original. Consider providing more context:
- What does "ml" stand for? (MoonLight?)
- Why was this changed from the platform-specific values?
- What value will users see when this placeholder is substituted?
Suggestion:
-| #{platform} | ml- (was The microcontroller platform) 🌙 | +| #{platform} | The platform identifier "ml" (MoonLight), used in hostnames and MQTT topics |This would help users understand the placeholder substitution system better.
docs/statefulservice.md (1)
408-409: Consider improving placeholder documentation clarity.The updated description
"ml- (was The microcontroller platform) 🌙"is cryptic and less informative than typical documentation. Consider clarifying:
- What "ml" represents (MoonLight platform identifier)
- Why it changed from hardware-specific values
- Example of the substituted value
Suggested improvement:
-| #{platform} | ml- (was The microcontroller platform) 🌙 | +| #{platform} | The platform identifier "ml" (MoonLight), substituted in hostnames and MQTT topics |factory_settings.ini (1)
3-3: Clarify the placeholder documentation.The current documentation "ml- (was The microcontroller platform...)" is confusing. It should clearly state what
#{platform}resolves to rather than mixing the resolved value with historical context.🔎 Suggested improvement
-; #{platform} - ml- (was The microcontroller platform, e.g. "esp32" or "esp32c3") +; #{platform} - The platform identifier (e.g. "ml"). Previously used microcontroller names like "esp32" or "esp32c3".src/MoonLight/Modules/ModuleLightsControl.h (2)
23-30: Consider reducing log verbosity.The
readMQTTfunction usesEXT_LOGIwhich will log every MQTT read operation. For production deployments with frequent state queries, this could generate excessive logs.🔎 Suggested improvement
String js; serializeJson(root, js); - EXT_LOGI(ML_TAG, "Read HA %s", js.c_str()); + EXT_LOGD(ML_TAG, "Read HA %s", js.c_str()); }
32-44: Consider reducing log verbosity and validating brightness range.Similar to
readMQTT, usingEXT_LOGImay generate excessive logs. Additionally, the brightness value from Home Assistant should be validated to ensure it's within the expected range (0-255).🔎 Suggested improvements
String js; serializeJson(root, js); - EXT_LOGI(ML_TAG, "Update HA %s", js.c_str()); + EXT_LOGD(ML_TAG, "Update HA %s", js.c_str()); JsonDocument doc; JsonObject newState = doc.to<JsonObject>(); if (!root["state"].isNull()) newState["lightsOn"] = (root["state"] == "ON"); - if (!root["brightness"].isNull()) newState["brightness"] = root["brightness"].as<uint8_t>(); + if (!root["brightness"].isNull()) { + uint8_t brightness = root["brightness"].as<uint8_t>(); + newState["brightness"] = brightness; // uint8_t naturally constrains to 0-255 + }Note: Since
uint8_tnaturally constrains values to 0-255, explicit validation may not be necessary unless you want to reject out-of-range values before casting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/buildprocess.mddocs/develop/sveltekit.mddocs/gettingstarted/installation.mddocs/network/mqtt.mddocs/statefulservice.mdfactory_settings.inilib/framework/EthernetSettingsService.hlib/framework/MqttSettingsService.hlib/framework/SettingValue.cpplib/framework/WiFiSettingsService.cppplatformio.inisrc/MoonLight/Modules/ModuleLightsControl.h
💤 Files with no reviewable changes (1)
- docs/develop/sveltekit.md
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
lib/framework/WiFiSettingsService.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Applied to files:
factory_settings.ini
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
lib/framework/MqttSettingsService.hsrc/MoonLight/Modules/ModuleLightsControl.hdocs/statefulservice.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
platformio.ini
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
lib/framework/SettingValue.cpp
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
🧬 Code graph analysis (1)
src/MoonLight/Modules/ModuleLightsControl.h (6)
src/MoonBase/Module.h (2)
ModuleState(47-137)Module(139-210)lib/framework/MqttSettingsService.h (1)
update(109-120)src/MoonBase/Module.cpp (3)
update(271-302)update(271-271)Module(308-320)lib/framework/ESP32SvelteKit.cpp (1)
ESP32SvelteKit(22-70)lib/framework/SettingValue.h (1)
SettingValue(24-27)lib/framework/SettingValue.cpp (2)
format(59-65)format(59-59)
🪛 LanguageTool
docs/network/mqtt.md
[grammar] ~10-~10: Use a hyphen to join words.
Context: ...tings / Devices & Services, it will auto discover the MoonLight devices In Moon...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/network/mqtt.md
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
docs/gettingstarted/installation.md (1)
95-95: Documentation update aligns with platform naming standardization.The hostname example update from
ML-hometoml-homeis consistent with the broader platform identifier changes across the codebase (platform constant changed from "esp32" to "ml").lib/framework/EthernetSettingsService.h (1)
31-31: LGTM - Hostname template now uses dynamic platform placeholder.The change from hardcoded
"ML-#{unique_id}"to"#{platform}-#{unique_id}"improves code maintainability and consistency. The effective hostname remains similar (e.g., "ml-6816" instead of "ML-6816") since thePLATFORMconstant is now "ml".lib/framework/WiFiSettingsService.cpp (1)
121-153: LGTM - Log messages updated for consistency.The log messages have been updated to use lowercase hostnames (e.g.,
ml-you35instead ofML-you35), aligning with the platform naming standardization across the codebase. These are cosmetic changes with no functional impact.platformio.ini (1)
59-59: LGTM - Routine build date update.Standard build date increment from December 29 to December 30, 2025.
docs/statefulservice.md (1)
416-418: LGTM - Example demonstrates Home Assistant MQTT integration patterns.The updated example code shows proper Home Assistant MQTT discovery patterns using platform and unique_id placeholders:
mqttPath:"homeassistant/#{platform}/#{unique_id}"follows HA discovery topic structurenameanduniqueId:"#{platform}-#{unique_id}"creates unique device identifiersThese changes align with the PR objective of adding Home Assistant integration.
lib/framework/SettingValue.cpp (1)
19-19: Platform identifier "ml" is intentionally the MoonLight project branding and is properly documented.The change from "esp32" to "ml" is a deliberate rebranding for the MoonLight project (indicated by the 🌙 emoji throughout the codebase). Factory settings have been consistently updated with inline documentation noting "(was The microcontroller platform)" in factory_settings.ini line 3, and both docs/statefulservice.md and docs/buildprocess.md explicitly document this change. The placeholder is used consistently across:
- FACTORY_WIFI_HOSTNAME and FACTORY_AP_SSID
- FACTORY_MQTT_CLIENT_ID
- Home Assistant paths
- Ethernet hostname
No orphaned hardcoded "esp32" references remain in the codebase.
lib/framework/MqttSettingsService.h (1)
51-51: This is new code from the Home Assistant integration, not a breaking change. The status topic format"light/#{unique_id}/status"has never had a previous"#{platform}/#{unique_id}/status"variant in this codebase—the macro was introduced directly with this format. Additionally, the status topic is not persisted (not part of MqttSettings), so it resets to the factory default on each boot, and ModuleLightsControl dynamically sets it at runtime. No migration or backward compatibility concerns apply.Likely an incorrect or invalid review comment.
factory_settings.ini (2)
12-12: LGTM!The platform-aware naming for hostname and AP SSID is a good improvement for flexibility and consistency.
Also applies to: 17-17
46-46: LGTM!The status topic change to
light/#{unique_id}/statusaligns well with Home Assistant conventions and the device type.src/MoonLight/Modules/ModuleLightsControl.h (3)
60-64: Verify member initialization order matches declaration order.The constructor initializer list should match the order of member declarations to avoid subtle initialization bugs. Ensure that
_mqttEndpoint,_mqttClient, and_mqttSettingsServiceare declared in this order in the class definition (lines 48-50).Based on the provided code, the order appears correct, but this is worth double-checking.
103-106: LGTM!Registering the MQTT
onConnectcallback to invokeregisterConfig()is a good pattern that ensures Home Assistant discovery configuration is published on every connection, handling reconnection scenarios properly.
47-50: Add FT_MQTT feature guard or require MQTT dependency.The class unconditionally calls
sveltekit->getMqttClient()andsveltekit->getMqttSettingsService()in the constructor (lines 63-64), but these methods are only defined whenFT_MQTTis enabled. IfFT_MQTTis disabled whileFT_MOONLIGHTis enabled, this will fail to compile. Either wrap the MQTT initialization in an#if FT_ENABLED(FT_MQTT)guard or require MQTT to be enabled as a dependency for this module.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/network/mqtt.md (1)
5-22: Fix markdown formatting and grammar issues (previously flagged).Several formatting and grammar issues remain from the previous review that need attention:
- Line 16: "non secure" should be hyphenated as "non-secure"
- Lines 17–19: Unordered list indentation should be 2 spaces (not 4) for proper nested rendering
- Line 17: "by it appropriate" should be "by its appropriate"
- Line 17: "mdns" should be properly capitalized as "mDNS"
- Line 19: "outsde" is a typo and should be "outside"
- Line 19: Remove the trailing backslash
🔎 Proposed fixes
* create non admin user in home assistant e.g mqttuser * install the Mosquitto MQTT Broker via Settings / Devices & Services, it will auto-discover the MoonLight devices * Optionally enable TLS (Transport Layer Security) for secure connections in MQTT / Configure In MoonLight / Network / MQTT: * Enable MQTT -* URI: mqtt://homeassistant.local:1883 (non secure) - * replace homeassistant.local by it appropriate mdns name or IP address - * secure HA: use mqtts://homeassistant.local:8883 - * to test outsde HA e.g. use mqtts://broker.hivemq.com:8883\ +* URI: mqtt://homeassistant.local:1883 (non-secure) + * replace homeassistant.local by its appropriate mDNS name or IP address + * secure HA: use mqtts://homeassistant.local:8883 + * to test outside HA e.g. use mqtts://broker.hivemq.com:8883 * Username / password: mqttuser credentials * client id: ml-xxxx. * Apply Settings
🧹 Nitpick comments (1)
src/MoonLight/Modules/ModuleLightsControl.h (1)
116-138: Home Assistant discovery config should be retained with higher QoS.The discovery message uses QoS 0 and
retain=false. This has reliability implications:
- QoS 0: If the message is lost during network issues, Home Assistant won't discover this device until the ESP reconnects.
- Non-retained: If Home Assistant restarts after this device published its config, it won't receive the discovery message and the device won't appear.
Additionally, consider adding
availabilityanddeviceobjects to the discovery payload for better HA integration (device grouping and online/offline status).🔎 Suggested improvements
+ // Add availability topic for online/offline status + doc["availability_topic"] = settingsStateTopic; + doc["payload_available"] = "online"; + doc["payload_not_available"] = "offline"; + + // Add device info for grouping in HA + JsonObject device = doc["device"].to<JsonObject>(); + device["identifiers"].to<JsonArray>().add(settingsUniqueId); + device["name"] = settingsName; + device["manufacturer"] = "MoonModules"; + device["model"] = "MoonLight"; + String payload; serializeJson(doc, payload); - _mqttClient->publish(configTopic.c_str(), 0, false, payload.c_str()); + _mqttClient->publish(configTopic.c_str(), 1, true, payload.c_str()); // QoS 1, retained
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/buildprocess.mddocs/network/mqtt.mddocs/statefulservice.mdfactory_settings.inilib/framework/MqttEndpoint.hsrc/MoonLight/Modules/ModuleLightsControl.h
✅ Files skipped from review due to trivial changes (1)
- lib/framework/MqttEndpoint.h
🚧 Files skipped from review as they are similar to previous changes (2)
- factory_settings.ini
- docs/statefulservice.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Applied to files:
docs/buildprocess.md
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
🧬 Code graph analysis (1)
src/MoonLight/Modules/ModuleLightsControl.h (4)
src/MoonBase/Module.h (2)
ModuleState(47-137)Module(139-210)lib/framework/MqttSettingsService.h (1)
update(109-120)lib/framework/SettingValue.h (1)
SettingValue(24-27)lib/framework/SettingValue.cpp (2)
format(59-65)format(59-59)
🪛 LanguageTool
docs/network/mqtt.md
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...RI: mqtt://homeassistant.local:1883 (non secure) * replace homeassistant.loca...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/network/mqtt.md
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
docs/buildprocess.md (1)
102-106: Placeholder documentation update looks good.The updated description for
#{platform}clearly explains the "ml-" identifier and its role in hostname and MQTT topic generation, which aligns well with the platform naming standardization across this PR.src/MoonLight/Modules/ModuleLightsControl.h (5)
23-30: LGTM!The
readMQTTfunction correctly translates the module state to Home Assistant's expected JSON format. The ternary operator for the ON/OFF state and brightness extraction are properly implemented.
32-44: Implementation looks correct.The function properly translates Home Assistant commands to module state updates. The null checks before accessing JSON fields are appropriate.
Consider that Home Assistant lights may send additional attributes like
color,color_temp, oreffectdepending on configuration. If those features are planned, this function would need extension.
47-50: LGTM!The member declarations are appropriate. Raw pointers for
_mqttClientand_mqttSettingsServiceare acceptable since these are owned byESP32SvelteKitand have a longer lifetime than this module.
60-69: LGTM!Constructor initialization is correct. The initializer list order matches the member declaration order, and the
MqttEndpointis properly configured with the read/update hooks. Based on the relevant code snippets showingESP32SvelteKitprovidesgetMqttClient()andgetMqttSettingsService(), this integration follows the established pattern.
448-448: LGTM!Minor formatting adjustment with no functional change.
|
@coderabbitai, please review latest commit |
|
@ewowi: I'll review the latest commit for the Home Assistant integration changes. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MoonLight/Modules/ModuleLightsControl.h (1)
160-160: Consider error handling for topic configuration.While
configureTopicsis called, its return value (if any) is not checked. If topic configuration can fail, consider logging a warning.🔎 Optional improvement
If
MqttEndpoint::configureTopicsreturns a status or can fail, consider checking the result:- _mqttEndpoint.configureTopics(pubTopic, subTopic); + if (!_mqttEndpoint.configureTopics(pubTopic, subTopic)) { + EXT_LOGW(ML_TAG, "Failed to configure MQTT topics"); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Modules/ModuleLightsControl.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
🧬 Code graph analysis (1)
src/MoonLight/Modules/ModuleLightsControl.h (5)
src/MoonBase/Module.h (2)
ModuleState(47-137)Module(139-210)lib/framework/MqttSettingsService.h (1)
update(109-120)src/MoonBase/Module.cpp (3)
update(271-302)update(271-271)Module(308-320)lib/framework/SettingValue.h (1)
SettingValue(24-27)lib/framework/SettingValue.cpp (2)
format(59-65)format(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
src/MoonLight/Modules/ModuleLightsControl.h (7)
23-30: LGTM! Correct Home Assistant light schema mapping.The state-to-JSON conversion correctly maps internal boolean
lightsOnto HA's "ON"/"OFF" string format and passes through brightness as expected by the Home Assistant light integration.
32-44: LGTM! Proper bidirectional state translation.The JSON-to-state conversion correctly handles partial updates from Home Assistant by checking
isNull()before applying each field. The call toModuleState::updateensures proper change propagation.
47-50: LGTM! Proper member declarations.The MQTT-related members are correctly typed and will be initialized via the constructor's initializer list.
60-64: LGTM! Proper MQTT member initialization.The constructor correctly initializes the MQTT endpoint with the translation functions and obtains the MQTT client and settings service from the framework.
103-106: LGTM! Previous review concern addressed.The null check for
_mqttClientbefore registering the callback correctly prevents crashes when MQTT is disabled. This properly addresses the previous review comment.
155-155: QoS level increased for reliability.The publish call now uses QoS 1 instead of QoS 0, ensuring at-least-once delivery of the Home Assistant discovery config. This is appropriate for configuration messages that should not be lost.
468-468: LGTM! Accurate timing calculation comment.The comment correctly documents the monitor emission throttling logic: 20ms minimum, scaling with light count at 300 lights per millisecond.
| void registerConfig() { | ||
| if (!_mqttClient->connected()) { | ||
| EXT_LOGE(ML_TAG, "MQTT client not connected"); | ||
| return; | ||
| } | ||
| if (!_mqttSettingsService) { | ||
| EXT_LOGE(ML_TAG, "MQTT settings service unavailable"); | ||
| return; | ||
| } | ||
| String configTopic; | ||
| String subTopic; | ||
| String pubTopic; | ||
|
|
||
| String settingsMqttPath = "homeassistant/light/" + String(_mqttSettingsService->getClientId()); // currently configured as a homeassistent light type | ||
| String settingsName = _mqttSettingsService->getClientId(); | ||
| String settingsUniqueId = _mqttSettingsService->getClientId(); | ||
| String settingsStateTopic = SettingValue::format(FACTORY_MQTT_STATUS_TOPIC); | ||
|
|
||
| JsonDocument doc; | ||
| configTopic = settingsMqttPath + "/config"; | ||
| subTopic = settingsMqttPath + "/set"; | ||
| pubTopic = settingsMqttPath + "/state"; | ||
| doc["~"] = settingsMqttPath; | ||
| doc["name"] = settingsName; | ||
| doc["unique_id"] = settingsUniqueId; | ||
|
|
||
| doc["cmd_t"] = "~/set"; | ||
| doc["stat_t"] = "~/state"; | ||
| doc["schema"] = "json"; | ||
| doc["brightness"] = true; | ||
|
|
||
| // Add availability topic for online/offline status | ||
| doc["availability_topic"] = settingsStateTopic; | ||
| doc["payload_available"] = "online"; | ||
| doc["payload_not_available"] = "offline"; | ||
|
|
||
| // Add device info for grouping in HA | ||
| JsonObject device = doc["device"].to<JsonObject>(); | ||
| device["identifiers"].to<JsonArray>().add(settingsUniqueId); | ||
| device["name"] = settingsName; | ||
| device["manufacturer"] = "MoonModules"; | ||
| device["model"] = "MoonLight"; | ||
|
|
||
| _mqttSettingsService->setStatusTopic(settingsStateTopic); | ||
|
|
||
| String payload; | ||
| serializeJson(doc, payload); | ||
| if (!_mqttClient->publish(configTopic.c_str(), 1, false, payload.c_str())) { // QoS 1 | ||
| EXT_LOGE(ML_TAG, "Failed to publish HA discovery config"); | ||
| return; | ||
| } | ||
|
|
||
| _mqttEndpoint.configureTopics(pubTopic, subTopic); | ||
|
|
||
| EXT_LOGI(ML_TAG, "Published HA discovery to %s", configTopic.c_str()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for getClientId() method implementation
rg -n "getClientId\s*\(" --type cpp -A 5Repository: MoonModules/MoonLight
Length of output: 2242
🏁 Script executed:
# Search for MqttSettingsService class definition
rg -n "class MqttSettingsService" --type cpp -A 20Repository: MoonModules/MoonLight
Length of output: 1428
🏁 Script executed:
# Search for where clientId is assigned or formatted
rg -n "clientId\s*=" --type cpp -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 679
🏁 Script executed:
# Search for unique_id or client ID initialization
rg -n "setClientId|_clientId" --type cpp -A 2 -B 2Repository: MoonModules/MoonLight
Length of output: 628
Consider validating that getClientId() is non-empty before constructing topics.
The method correctly uses _mqttSettingsService->getClientId() which returns the configured client ID from settings with SettingValue::format(FACTORY_MQTT_CLIENT_ID) as the default. However, there is no validation that the client ID is non-empty before it's used to construct the MQTT topic paths and unique identifier. While unlikely given the default formatting, an empty client ID would result in malformed topics.
Additionally, _mqttEndpoint.configureTopics() on line 160 does not validate the return value; consider checking its result for completeness.
🤖 Prompt for AI Agents
In src/MoonLight/Modules/ModuleLightsControl.h around lines 108 to 163, the code
uses _mqttSettingsService->getClientId() directly to build MQTT topic paths and
unique_id and calls _mqttEndpoint.configureTopics(pubTopic, subTopic) without
validating results; add a guard after obtaining the clientId to ensure it is
non-empty (log an error and return if empty) before constructing
settingsMqttPath, settingsName, and settingsUniqueId, and after calling
_mqttEndpoint.configureTopics(...) check its return value (or success indicator)
and log/handle failure (cleanup/return) if it fails.
|
should we check on _mqttSettingsService->isEnabled(); in ModuleLightsControl.h as no mqtt stuff needs to be done when not enabled. Also sizeof(_mqttEndpoint) is 160 bytes, maybe dynamically create / delete depending on enabled? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20523 🏁 Script executed: Length of output: 1378 🏁 Script executed: Length of output: 4846 🏁 Script executed: Length of output: 2264 🏁 Script executed: Length of output: 5126 @ewowi: Great questions! You're right on both counts — let me address each: 1. Checking
|
|
@coderabbitai, regarding
it will be set on/off at runtime, is that also health with in your proposal? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ewowi: Good catch — no, my compromise proposal does NOT handle runtime enable/disable properly. It only checks at boot time in Let me investigate how to handle runtime changes properly: 🏁 Script executed: Length of output: 6641 🏁 Script executed: Length of output: 8377 🏁 Script executed: Length of output: 24777 🏁 Script executed: Length of output: 0 @ewowi: You're absolutely right — my compromise proposal does not handle runtime enable/disable. After examining the codebase, here's how to properly handle it: How MqttSettingsService notifies about changesThe pattern in the codebase uses update handlers from
Proper solution for ModuleLightsControlYou need to register an update handler on class ModuleLightsControl : public Module {
private:
MqttEndpoint<ModuleState>* _mqttEndpoint = nullptr; // pointer, dynamically allocated
PsychicMqttClient* _mqttClient;
MqttSettingsService* _mqttSettingsService;
update_handler_id_t _mqttSettingsUpdateHandlerId = 0; // track handler ID
public:
ModuleLightsControl(PsychicHttpServer* server, ESP32SvelteKit* sveltekit, FileManager* fileManager, ModuleIO* moduleIO)
: Module("lightscontrol", server, sveltekit),
_mqttClient(sveltekit->getMqttClient()),
_mqttSettingsService(sveltekit->getMqttSettingsService()) {
// Don't initialize MQTT here - do it in begin()
}
~ModuleLightsControl() {
cleanupMqtt();
// Remove update handler
if (_mqttSettingsUpdateHandlerId && _mqttSettingsService) {
_mqttSettingsService->removeUpdateHandler(_mqttSettingsUpdateHandlerId);
}
}
void begin() override {
Module::begin();
// ... existing code ...
// Register handler to react to MQTT settings changes (including enable/disable)
if (_mqttSettingsService) {
_mqttSettingsUpdateHandlerId = _mqttSettingsService->addUpdateHandler(
[this](const String& originId) { onMqttSettingsChanged(); },
false // don't allow removal by others
);
}
// Initialize MQTT if enabled at boot
onMqttSettingsChanged();
}
private:
void onMqttSettingsChanged() {
if (!_mqttSettingsService) return;
bool shouldBeEnabled = _mqttSettingsService->isEnabled() && _mqttClient;
bool isCurrentlyEnabled = (_mqttEndpoint != nullptr);
// Handle enable/disable transitions
if (shouldBeEnabled && !isCurrentlyEnabled) {
// Transition: disabled → enabled
EXT_LOGD(ML_TAG, "Enabling MQTT for Home Assistant");
initializeMqtt();
} else if (!shouldBeEnabled && isCurrentlyEnabled) {
// Transition: enabled → disabled
EXT_LOGD(ML_TAG, "Disabling MQTT for Home Assistant");
cleanupMqtt();
}
// If both true or both false, no transition needed (already in correct state)
}
void initializeMqtt() {
if (_mqttEndpoint) return; // already initialized
_mqttEndpoint = new MqttEndpoint<ModuleState>(readMQTT, updateMQTT, this, _mqttClient);
_mqttClient->onConnect(std::bind(&ModuleLightsControl::registerConfig, this));
// If already connected, register config immediately
if (_mqttClient->connected()) {
registerConfig();
}
}
void cleanupMqtt() {
if (_mqttEndpoint) {
delete _mqttEndpoint;
_mqttEndpoint = nullptr;
}
// Note: We can't easily remove the onConnect callback, but registerConfig checks enabled state
}
void registerConfig() {
// Defense in depth: check enabled state even though we only create endpoint when enabled
if (!_mqttSettingsService || !_mqttSettingsService->isEnabled()) {
EXT_LOGD(ML_TAG, "MQTT not enabled, skipping HA config");
return;
}
if (!_mqttClient || !_mqttClient->connected()) {
EXT_LOGE(ML_TAG, "MQTT client not connected");
return;
}
// ... rest of existing registerConfig code ...
}
};Key points:✅ Handles runtime changes: Update handler reacts to any MQTT settings change How it works:
Does this address your concern about runtime enable/disable? 🎯 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/MoonLight/Modules/ModuleLightsControl.h (2)
23-30: Add defensive null checks inreadMQTTfor consistency.The function accesses
state.data["lightsOn"]andstate.data["brightness"]without verifying the keys exist. While ArduinoJson returns default values for missing keys, explicit checks (as done inupdateMQTTon lines 40-41) improve robustness and make intent clear.🔎 Proposed refactor
void readMQTT(ModuleState& state, JsonObject& root) { - root["state"] = state.data["lightsOn"].as<bool>() ? "ON" : "OFF"; - root["brightness"] = state.data["brightness"].as<uint8_t>(); + if (!state.data["lightsOn"].isNull()) { + root["state"] = state.data["lightsOn"].as<bool>() ? "ON" : "OFF"; + } + if (!state.data["brightness"].isNull()) { + root["brightness"] = state.data["brightness"].as<uint8_t>(); + } String js; serializeJson(root, js); EXT_LOGD(ML_TAG, "Read HA %s", js.c_str()); }
61-64: Consider null-checking MQTT service pointers after initialization.The constructor stores raw pointers from
getMqttClient()andgetMqttSettingsService()without validation. Whilebegin()checks_mqttSettingsServicebefore use (line 105), adding an early guard or logging a warning if these services are unavailable would make the lifecycle more explicit and prevent potential null-pointer dereferences.🔎 Proposed validation in begin()
void begin() override { Module::begin(); + + if (!_mqttClient || !_mqttSettingsService) { + EXT_LOGW(ML_TAG, "MQTT services unavailable, Home Assistant integration disabled"); + return; + } EXT_LOGI(ML_TAG, "Lights:%d(Header:%d) L-H:%d Node:%d PL:%d(PL-L:%d) VL:%d PM:%d C3D:%d", sizeof(Lights), sizeof(LightsHeader), sizeof(Lights) - sizeof(LightsHeader), sizeof(Node), sizeof(PhysicalLayer), sizeof(PhysicalLayer) - sizeof(Lights), sizeof(VirtualLayer), sizeof(PhysMap), sizeof(Coord3D));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Modules/ModuleLightsControl.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
📚 Learning: 2025-12-28T10:28:24.157Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR #84: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Applied to files:
src/MoonLight/Modules/ModuleLightsControl.h
🧬 Code graph analysis (1)
src/MoonLight/Modules/ModuleLightsControl.h (4)
lib/framework/MqttSettingsService.h (1)
update(109-120)lib/framework/ESP32SvelteKit.cpp (1)
ESP32SvelteKit(22-70)lib/framework/SettingValue.h (1)
SettingValue(24-27)lib/framework/SettingValue.cpp (2)
format(59-65)format(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
src/MoonLight/Modules/ModuleLightsControl.h (6)
47-51: LGTM! Dynamic allocation pattern correctly implemented.The pointer-based approach for
_mqttEndpointaligns with the PR objectives to save ~160 bytes when MQTT is disabled, and the handler ID tracking enables proper cleanup.
113-130: LGTM! Runtime enable/disable transitions handled correctly.The method properly detects state changes and manages MQTT endpoint lifecycle as discussed in the PR objectives, enabling dynamic memory savings when MQTT is disabled.
132-142: LGTM! Endpoint initialization follows safe allocation pattern.The guard against double-initialization and immediate config registration when already connected are both correct. The
_mqttClientnull-check is handled by the caller (onMqttSettingsChangedline 116).
144-150: LGTM! Cleanup correctly deallocates endpoint.The defensive null-check and pointer reset prevent double-delete. The acknowledged limitation regarding the
onConnectcallback is acceptable givenregisterConfig's defensive checks at lines 154-161.
152-209: Home Assistant discovery configuration looks correct.The defensive checks, discovery payload structure with device info and availability, and error handling on publish failure are all properly implemented. The method correctly publishes the configuration and sets up bidirectional topics.
Note: Past review comments have already flagged validation concerns for
getClientId()(line 167) and checking the return value ofconfigureTopics()(line 206).
514-514: Minor formatting adjustment, no semantic change.
| // Register handler to react to MQTT settings changes (including enable/disable) | ||
| if (_mqttSettingsService) { | ||
| _mqttSettingsUpdateHandlerId = _mqttSettingsService->addUpdateHandler([this](const String& originId) { onMqttSettingsChanged(); }, | ||
| false // don't allow removal by others | ||
| ); | ||
| onMqttSettingsChanged(); // Initialize MQTT if enabled at boot | ||
| } | ||
| } |
There was a problem hiding this comment.
Add destructor to remove the MQTT settings update handler.
The code registers an update handler with _mqttSettingsService and stores the handler ID, but there's no destructor to remove it. If this module is destroyed, the handler remains registered and will attempt to call onMqttSettingsChanged() on a destroyed object, causing a crash.
🔎 Proposed destructor
Add this destructor in the public section after the constructor:
~ModuleLightsControl() {
// Clean up MQTT endpoint
cleanupMqtt();
// Remove the settings update handler
if (_mqttSettingsService && _mqttSettingsUpdateHandlerId != 0) {
_mqttSettingsService->removeUpdateHandler(_mqttSettingsUpdateHandlerId);
_mqttSettingsUpdateHandlerId = 0;
}
}🤖 Prompt for AI Agents
In src/MoonLight/Modules/ModuleLightsControl.h around lines 104 to 111, the
class registers an MQTT settings update handler but lacks a destructor to
unregister it; add a destructor (public, placed after the constructor) that
calls cleanupMqtt(), checks if _mqttSettingsService is non-null and
_mqttSettingsUpdateHandlerId is non-zero, calls
_mqttSettingsService->removeUpdateHandler(_mqttSettingsUpdateHandlerId), and
then resets _mqttSettingsUpdateHandlerId to 0 to prevent callbacks to a
destroyed object.
|
@coderabbitai, why does mqtt sometimes disconnect? See log: [ 26155][D][PsychicHttpServer.cpp:226] openCallback(): [🔮] New client connected 54 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16158 🏁 Script executed: Length of output: 1194 🏁 Script executed: Length of output: 5260 🌐 Web query: 💡 Result: Short answer What typically goes wrong
Practical steps / mitigations
References
If you want, tell me your Mosquitto version + relevant mosquitto.conf and Home Assistant logs and I’ll point to the most likely bug/workaround and exact config changes. @ewowi: Great question! Looking at your logs and the codebase, the most likely culprit is the QoS 2 subscription. Here's my analysis: Root Cause: QoS 2 with MosquittoYour code already has a comment hinting at this issue in _mqttClient->subscribe(_subTopic.c_str(), 2); // 🌙 Home Assistant / Mosquitto does not benefit from QoS 2, 1 might be better, add as UI variable?The evidence:
Recommended FixChange the subscription from QoS 2 to QoS 1: // In lib/framework/MqttEndpoint.h:229
_mqttClient->subscribe(_subTopic.c_str(), 1); // QoS 1 is more reliable with HA/MosquittoQoS 1 provides "at least once" delivery, which is sufficient for Home Assistant commands and far more stable with Mosquitto. Long-term SolutionYour comment suggests making QoS configurable via UI, which is a good idea: // Add to MqttSettings struct:
uint8_t subscriptionQos = 1; // default to QoS 1
// Then use it:
_mqttClient->subscribe(_subTopic.c_str(), _mqttSettings->getSubscriptionQos());Other Potential Causes (less likely)
VerificationAfter changing to QoS 1, monitor for:
TL;DR: Change QoS 2 → QoS 1 in MqttEndpoint.h:229. The comment was spot-on! 🎯 |
|
@coderabbitai, I also get disconnects with subscribe 1, publish 1. The original settings where subscribe 2, publish 0. But also give me disconnects. Is there a constraint saying subscribe with highest qos, publish with lowest? Should I make 2 variables: subscribe qos and publish qos for optimal tuning? But what is the optimal tuning. Could I also change some settings on the home assistant / MQTT side? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1803 🌐 Web query: 💡 Result: Likely causes and quick checklist to diagnose persistent ESP32 → Mosquitto disconnects (Home Assistant environment)
What to check now (order of priority)
References
If you want, paste:
🌐 Web query: 💡 Result: Short answer
What that means for connection stability
Practical guidance
References
@ewowi: Great question! The fact that disconnects persist across all QoS combinations tells us QoS is not the root cause — it's a symptom masking deeper issues. Let me address your questions and then tackle the real culprits: QoS Constraints & Best PracticesNo hard constraint requiring subscribe QoS > publish QoS. However: ✅ Separate variables are recommended: // In MqttSettings:
uint8_t subscribeQos = 1; // For commands coming from HA
uint8_t publishQos = 0; // For state updates going to HATypical configurations:
Your original (subscribe 2, publish 0) was unusual but valid — QoS 2 for commands ensures exactly-once, QoS 0 for state is fine if you don't need delivery guarantees. Real Root Causes (Based on ESP32 + Mosquitto patterns)The #1 cause of ESP32 MQTT disconnects is Wi-Fi modem sleep mode. Here are the likely culprits: 1. 🔴 Wi-Fi Power Save Mode (Most Likely)ESP32's default Wi-Fi sleep causes disconnects after idle periods. Fix in your ESP32 code: // In WiFiSettingsService.cpp or during WiFi initialization:
void WiFiSettingsService::onStationModeGotIP(WiFiEvent_t event, WiFiEventInfo_t info) {
// ... existing code ...
// Disable Wi-Fi sleep to prevent MQTT disconnects
WiFi.setSleep(false);
// ... rest of code ...
}2. 🟡 Keep-Alive Too LongYour current setting: 120 seconds. Mosquitto disconnects after ~1.5× keepalive if no ping is received, meaning ~3 minutes of silence triggers disconnect. Recommended: # In factory_settings.ini, change from:
-D FACTORY_MQTT_KEEP_ALIVE=120
# To:
-D FACTORY_MQTT_KEEP_ALIVE=60 ; or even 30 for more responsive detection3. 🟡 Client Loop FrequencyEnsure MQTT client loop runs frequently. Check your main loop: void loop() {
// ... other code ...
// Ensure MQTT service processes messages regularly
_mqttSettingsService->loop(); // Should be called frequently, not blocked
// Avoid long delays or blocking operations
}4. 🟢 Network/Router IssuesSome routers/APs have beacon intervals or channel configurations that cause ESP32 disconnects. Router-side checks:
Mosquitto Broker ConfigurationAdd/check these in your Home Assistant Mosquitto add-on configuration: # In Home Assistant Mosquitto add-on config:
customize:
active: true
folder: mosquitto
# Create /config/mosquitto/mosquitto.conf with:
max_inflight_messages 20 # Default, OK for most cases
max_queued_messages 1000 # Allow queuing when client offline
max_keepalive 65535 # Don't override client keepalive
persistent_client_expiration 4h # Clean up stale sessionsLow max_inflight_messages (especially 1) can block clients and cause perceived stalls; keep the default of 20. Diagnostic Steps (In Order)
Code Changes Summary1. Separate QoS settings (MqttSettingsService.h): struct MqttSettings {
// ... existing fields ...
uint8_t subscribeQos = 1; // Add this
uint8_t publishQos = 0; // Add this
};2. Disable Wi-Fi sleep (WiFiSettingsService.cpp): WiFi.setSleep(false); // Add after getting IP3. Reduce keep-alive (factory_settings.ini): -D FACTORY_MQTT_KEEP_ALIVE=60 # Change from 120Expected OutcomeAfter applying Wi-Fi sleep fix + reduced keep-alive, disconnects should drastically reduce or stop entirely. The QoS settings you choose afterward are for optimizing reliability vs. bandwidth, not fixing disconnects. My bet: Wi-Fi sleep is your culprit. Try that first! 🎯 Want me to generate the complete code changes as patches? |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.