feat: expand HA MQTT integration - more entities, discovery + reliabi…

[DrewFerg11]

…lity fixes

New MQTT Discovery entities (all grouped under the existing HA device):

• Restart button (reboots the ESP32)
• Re-Home button (re-homes all modules, then blanks)
• Mode select (Off / Date / Time, plus Date & Time on dual builds)
• WiFi Signal (RSSI) and IP Address diagnostic sensors

Reliability fixes:

• Reconnect to the broker every 5s after a drop (previously never retried)
• MQTT last-will so HA marks the device unavailable on power loss
• Re-publish discovery configs when HA announces online (homeassistant/status)
• Explicit 1KB packet buffer; discovery payloads exceed PubSubClient default
• Stop clobbering the retained display state with "" on every connect

[drewferg11-hermes-bot]

Reviewed the full diff against main and the surrounding code (SplitFlapDisplay.ino mode dispatch, SplitFlapWebServer mode/setMode, SplitFlapDisplay interface). Overall the direction is good — the LWT, retry loop, discovery re-publish on HA online, explicit buffer sizing, and dropping the "" state-clobber on connect are all real improvements. Requesting changes for two real problems and one safety concern, detailed inline:

1. JSON injection / malformed discovery in deviceJson() (and the same raw interpolation in the other payloads). name is a free-form user setting exposed through the web UI; a " or \ in it breaks the discovery JSON for all 7 entities, and a crafted value injects arbitrary JSON keys. This is pre-existing for the old two payloads but this PR centralizes and multiplies it, so please escape it (or build the payloads with ArduinoJson).

2. homeToString() is invoked synchronously from the PubSubClient callback (topic_cmd_home). Homing takes seconds and blocks the MQTT loop + main loop the whole time — same class of problem the restart button already solves by deferring to loop(). Please defer re-home the same way.

3. publishDiscovery() + publishTelemetry() are called from inside the MQTT callback on homeassistant/status = online, i.e. mqttClient.publish() re-entrant inside mqttClient.loop(). PubSubClient doesn't guarantee that's safe; defer it to loop() like restart.

The reliability fixes themselves look correct (LWT topic/qos/retain args are right, the retry uses millis() correctly and returns early when disconnected so it doesn't pile up, and the mode select state-publishing for unlisted modes as ""/unknown is the right call). Happy to re-review once the above are addressed.

[drewferg11-hermes-bot]

JSON injection / malformed discovery: name is read from user settings (set freely via the web UI POST /settings as an arbitrary string) and interpolated raw into a JSON string literal here and in every discovery payload that includes device. If name contains a " or \ (e.g. My "Display"), the discovery JSON is malformed and HA silently drops the entity — the whole device group can fail to register. A hostile value like x","foo":"bar injects arbitrary keys into every discovery payload.

This was already true in the old two payloads, but this PR centralizes deviceJson() and pulls it into 7 payloads, so it's worth fixing here. Please JSON-escape name (and mdns) before interpolation — at minimum replace \ then " with \\ / \". Better: build these with ArduinoJson so escaping is automatic.

Note mdns has the same issue in the topic strings and identifiers/unique_id fields, though mdns is usually constrained to hostname chars; name is the clear bug.

[drewferg11-hermes-bot]

Sorry, let me make this more concrete.

Where name comes from: name is a free-text setting (JST_STR type) stored in NVS. It's set via the web UI's POST /settings endpoint, which calls settings.fromJson(json). The validation in JsonSetting::validate() only checks int-vector fields — for string settings like name, it unconditionally returns true. So any string the user types into the Name field in the web UI is stored as-is, no escaping, no restrictions.

What breaks: deviceJson() drops name raw between " delimiters:

"\"name\":\"" + name + "\","

If the user sets the display name to My "Cool" Display (a perfectly reasonable thing to type), the output is:

"name":"My "Cool" Display",

That's not valid JSON — the quotes inside name close the JSON string early. HA's MQTT discovery parser rejects the whole payload and silently drops the entity. Every single discovery config that includes device (all 7 of them in publishDiscovery()) fails to register. The user sees nothing in HA and has no idea why.

A backslash in the name (e.g. a path like C:\Display) has the same effect — \ in JSON must be \\, but it's passed through as a single \.

The fix: Escape name before interpolating. Minimal version — replace \ first, then ":

String safeName = settings.getString("name");
safeName.replace("\\", "\\\\");
safeName.replace("\"", "\\\"");
// then use safeName instead of name

Or, since you already include <ArduinoJson.h> in the web server, build the device block with ArduinoJson and let it handle escaping automatically — that's the most robust option and avoids hand-rolling escape logic.

mdns has the same interpolation pattern in the topic strings and identifiers/unique_id fields, but mdns is typically constrained to hostname-safe characters. name is the clear bug because it's a human-friendly label with no character restrictions.

[drewferg11-hermes-bot]

Blocking call inside the PubSubClient callback: homeToString() runs a full motor home sequence (rotate every module back to the magnet) which takes seconds. The MQTT callback runs synchronously inside mqttClient.loop(), so this blocks the MQTT keepalive and the main loop() for the entire homing duration — long enough to risk the broker dropping the connection on a slow home, and the UI/Improv are frozen meanwhile.

The restart button already defers to loop() via restartPending for exactly this reason; the re-home button should do the same. Set a homePending flag here and act on it from loop(). (The writeString path on topic_command has the same blocking-in-callback issue, but that's pre-existing; the re-home is new, so let's not add a second one.)

[drewferg11-hermes-bot]

I dug into this properly. Yes, it's a real issue and it's reachable here.

The call chain: SplitFlapMqtt::loop() calls mqttClient.loop() (line ~295). PubSubClient receives a message on topic_cmd_home and invokes the callback synchronously, inside that loop() call. The callback calls handleMessage(), which on topic_cmd_home calls display->homeToString("", ...) directly (line ~99). That runs to completion before mqttClient.loop() returns.

What homeToString does: It spins every module nearly a full rotation to find the magnet (moveTo → moveModules), then calls writeString to move to the target. moveModules is a blocking while (!isFinished) loop that steps motors and polls hall sensors on a 20ms cadence. With 2048 steps/rot at 15 RPM, one rotation takes ~4 seconds. The full home + write sequence is typically 5-10 seconds depending on where the flaps already are.

Why it matters: While homeToString is blocking inside the callback, mqttClient.loop() is not returning, so PubSubClient can't send keepalive PINGREQs. The MQTT broker's keepalive timeout is typically 15-60 seconds. A slow home (sticky magnet, 16 modules, or a second writeString call to a far position) can approach or exceed that window, at which point the broker drops the connection mid-home. The main loop() is also frozen for the entire duration, so Improv and the web server's loop()-based servicing are stalled too.

Note: moveModules does call backgroundTick as its idle callback (via moveTo), so Improv gets serviced during the motor loop. But that doesn't help MQTT keepalive — that only happens when mqttClient.loop() returns, which it can't while we're blocking inside it.

How confident am I: Confident this is reachable and blocking. Whether it actually causes a broker disconnect depends on the specific broker's keepalive setting vs. the homing duration, so it won't fail every time — but it's a real race that will bite on a slow home. The deferred pattern already exists in this PR for restartPending (set the flag in the callback, act on it from loop()). A homePending flag following the same pattern would fix it cleanly.

The pre-existing writeString call on topic_command (line ~91) has the same blocking-in-callback issue, but as I noted originally, that's not new to this PR — the re-home button is new, so this is the one worth fixing here.

[drewferg11-hermes-bot]

Calling publishTelemetry() (and publishDiscovery() just above) from inside the PubSubClient message callback invokes mqttClient.publish() re-entrantly while still inside mqttClient.loop(). PubSubClient doesn't guarantee this is safe — it can corrupt the TX buffer / drop the publish depending on the underlying WiFiClient state. This path fires whenever HA comes back online, which is exactly when you most want discovery + telemetry to land reliably.

Safer to set a flag here (e.g. haRediscoverPending = true) and run publishDiscovery() + publishTelemetry() from loop(), the same deferred pattern already used for restart.