Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf674ac43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (client.connected() && !wasConnected && !running_menu) | ||
| { | ||
| restoreAlarmLevel(&debugSerial); |
There was a problem hiding this comment.
When MQTT drops from an already-connected state, wasConnected is true, so this reconnect path can show the disconnect screen and then immediately reconnect in the same iteration, but the restore check is gated on !wasConnected and therefore never runs. In that case the LCD can remain stuck on "MQTT DISCONNECTED" even though client.connected() is true, until some unrelated UI refresh happens (e.g., alarm update or menu exit). This breaks the intended automatic recovery behavior after transient disconnects.
Useful? React with 👍 / 👎.
|
So looking at the discussion between @RobertLRead and @ForrestErickson , there were a few directions floated — rotating the LCD to show MQTT status periodically, having the PMD send heartbeats and tracking stale messages, giving alarms an expiry time, or just reacting directly to the MQTT connection state. The rotating LCD felt too passive — you'd only see it if you happened to be watching at the right second. The heartbeat/stale detection idea is honestly the right long-term fix but it needs the PMD side to change too, and it opens up the whole alarm management question (who owns the alarm, when is it stale, who clears it) which felt bigger than what this issue was asking for. So I went with the MQTT state approach since it was called out as "probably easier" in the thread. Basically the Krake now checks if MQTT was connected before and after each loop — the moment it drops, LCD shows the disconnected screen and DFPlayer fires a sound, and when it comes back the alarm display restores automatically. Entirely self-contained, no PMD changes needed. The heartbeat/stale detection is still the better solution for catching cases where WiFi is fine but messages just stop coming — happy to open a follow-up issue for that if that makes sense. Open to any thoughts on the approach here. |
… on reconnect to prevent audio lockup
…vent LCD stuck on MQTT DISCONNECTED
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 236519141f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (wasConnected && !running_menu) | ||
| { | ||
| showMqttStatusLCD(false); | ||
| playAlarmLevel(problem); |
There was a problem hiding this comment.
When MQTT drops while running_menu is true, this guard skips showMqttStatusLCD(false) on the first disconnect iteration; on subsequent iterations wasConnected is already false, so the warning can never be shown after the user exits the menu. In that scenario the LCD returns to normal alarm text (restoreAlarmLevel) even though the data is stale, and it stays that way until reconnection succeeds, which defeats the disconnect-alert behavior this change introduces.
Useful? React with 👍 / 👎.
1 participant
Links
What & Why
Validation / How to Verify
Artifacts (attach if relevant)
Checklist