Merge pull request #31 from AndrewDemsDS/fix/display-tristate-52

fix(display): make the Display switch actually control the panel

Author: AndrewDemsDS

firmware/esp32-matter/CMakeLists.txt

@@ -2,7 +2,7 @@
 #   . ~/esp/esp-idf-v5.5.4/export.sh && . $ESP_MATTER_PATH/export.sh
 #   idf.py set-target esp32 && idf.py build flash monitor
 cmake_minimum_required(VERSION 3.16)
-set(PROJECT_VER "1.0.10")
+set(PROJECT_VER "1.0.11")
 
 # Unified versioning (issue #77): the Matter softwareVersion INT is DERIVED from PROJECT_VER --
 # MAJOR*10000+MINOR*100+PATCH -> a readable, strictly-monotonic uint32. This keeps the human

firmware/esp32-matter/main/app_main.cpp

@@ -83,7 +83,8 @@ static constexpr uint32_t kMfgClusterId = 0xFFF1FC00;
 // reference so the first combined frame is always valid.
 static HisenseCommand s_cmd   = { HISENSE_MODE_COOL, 24, false,
                                   HISENSE_FAN_AUTO, HISENSE_SWING_OFF,
-                                  HISENSE_SWING_OFF, HISENSE_FEATURE_NONE, false };
+                                  HISENSE_SWING_OFF, HISENSE_FEATURE_NONE,
+                                  HISENSE_DISPLAY_NOCHANGE };
 static volatile bool s_from_bus = false;// true while pushing status->attrs: suppress
                                         // the resulting PRE/POST_UPDATE from re-sending a cmd
 
@@ -152,6 +153,22 @@ static void flush_cmd()
     if (hisense_send_frame(f, n)) arm_sync_hold();          // arm the settle only if it enqueued
     else                          ESP_LOGW(TAG, "cmd dropped (TX queue full)");
 }
+/* Bench bridge for the diag console's `tx` (#52 display-byte hunt). Lives here, not
+ * in diag_console.cpp, so s_cmd and arm_sync_hold stay private: the probe frame is
+ * built from the CURRENT command state, so it differs from what the A/C is already
+ * running by exactly the byte under test. Arms the settle window like any other
+ * send, otherwise the next status frame resyncs s_cmd mid-probe.
+ * Returns 0 sent, -1 offset/build rejected, -2 TX queue full. */
+extern "C" int diag_tx_override(int off, uint8_t val)
+{
+    uint8_t f[HISENSE_CMD_FRAME_LEN + 2];
+    size_t n = hisense_build_command_override(&s_cmd, f, sizeof(f), off, val);
+    if (!n) return -1;
+    if (!hisense_send_frame(f, n)) return -2;
+    arm_sync_hold();
+    return 0;
+}
+
 static void send_power(bool on)
 {
     uint8_t f[HISENSE_CMD_FRAME_LEN];
@@ -192,13 +209,20 @@ static void apply_sleep(uint8_t profile)
     size_t n = hisense_build_sleep_frame(profile, f, sizeof(f));
     if (n && hisense_send_frame(f, n)) arm_sync_hold();
 }
-// #19 cheap win: panel display on/off. display_on rides the combined command frame (@20:
-// 0xC0 on / 0x40 off); flush_cmd() rebuilds + sends it. No status feedback (the A/C doesn't
-// report display state), so the OnOff attr is optimistic — reflects the last command.
+// #19 cheap win: panel display on/off. `display` rides the combined command frame (@20:
+// 0xC0 on / 0x40 off / 0x00 leave-alone); flush_cmd() rebuilds + sends it. No status feedback
+// (the A/C doesn't report display state), so the OnOff attr is optimistic — reflects the last
+// command.
+//
+// ONE-SHOT (#52): reset to NOCHANGE after the frame goes out. `display` is packed into EVERY
+// combined command, so leaving ON/OFF latched would re-assert the panel state on every later
+// mode/setpoint/fan change and fight the user's remote. Leave-alone is the only correct resting
+// value.
 static void apply_display(bool on)
 {
-    s_cmd.display_on = on;
+    s_cmd.display = on ? HISENSE_DISPLAY_ON : HISENSE_DISPLAY_OFF;
     flush_cmd();
+    s_cmd.display = HISENSE_DISPLAY_NOCHANGE;
 }
 
 static void on_recommission(uint8_t reason);   // fwd decl (defined in the "77" section below)

firmware/esp32-matter/main/diag_console.cpp

@@ -3,6 +3,7 @@
 // for Matter coexistence: diag_on_status() only snapshots (no socket I/O under the
 // CHIP stack lock); the `watch` stream and the TCP accept loop run in their own tasks.
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
 #include <unistd.h>
@@ -176,13 +177,52 @@ static int cmd_decode(int argc, char **argv)
     return 0;
 }
 
+// Bench probe for #52 (display byte unknown). Sends the CURRENT command frame with one
+// payload byte replaced, so a miss is a no-op rather than a surprise mode/setpoint change.
+//
+// Deliberately single-shot: there is no auto-sweep. hisense_parse_status() decodes no
+// display state (power_display/q_display are ProductType CAPABILITY bits, not live
+// status), so the only oracle is the panel LED and every probe needs a human looking at
+// the unit before the next one goes out. An unattended sweep would just log offsets.
+static int cmd_tx(int argc, char **argv)
+{
+    if (argc < 3) {
+        printf("usage: tx <offset> <value>   (both accept 0x.. or decimal)\r\n"
+               "  payload offsets %u..%u; header and checksum/terminator are rejected\r\n"
+               "  current suspect (#52): tx 36 0xC0  then  tx 36 0x40\r\n"
+               "  watch the PANEL after each probe -- there is no status read-back\r\n",
+               (unsigned) HISENSE_CMD_HEADER_LEN, (unsigned) HISENSE_CMD_CHK_OFFSET - 1);
+        return 1;
+    }
+    long off = strtol(argv[1], NULL, 0);
+    long val = strtol(argv[2], NULL, 0);
+    if (val < 0 || val > 0xFF) { printf("value out of range (0..0xFF)\r\n"); return 1; }
+
+    switch (diag_tx_override((int) off, (uint8_t) val)) {
+    case -1:
+        printf("rejected: offset %ld is outside the payload [%u,%u)\r\n",
+               off, (unsigned) HISENSE_CMD_HEADER_LEN, (unsigned) HISENSE_CMD_CHK_OFFSET);
+        return 1;
+    case -2:
+        printf("TX queue full -- NOT sent, retry\r\n");
+        return 1;
+    default:
+        break;
+    }
+    printf("sent: frame[%ld] = 0x%02lX (every other byte = current A/C state)\r\n"
+           "  -> check the panel now; any change is attributable to this byte alone\r\n"
+           "  -> to undo, drive the unit normally from HA (rebuilds the baseline frame)\r\n",
+           off, (unsigned long) val);
+    return 0;
+}
+
 // Compact codec self-check (subset of the host golden vectors).
 static int cmd_selftest(int, char **)
 {
     int fails = 0;
     uint8_t f[64];
     HisenseCommand c = { HISENSE_MODE_COOL, 22, false, HISENSE_FAN_LOW,
-                         HISENSE_SWING_OFF, HISENSE_SWING_OFF, HISENSE_FEATURE_ECO, false };
+                         HISENSE_SWING_OFF, HISENSE_SWING_OFF, HISENSE_FEATURE_ECO, HISENSE_DISPLAY_NOCHANGE };
     size_t n = hisense_build_command(&c, f, sizeof(f));
     if (!(n && f[16] == 0x0B && f[18] == 0x50 && f[19] == 0x2D && f[33] == 0x30)) fails++;
     uint8_t hi = 0, lo = 0;
@@ -290,6 +330,8 @@ extern "C" void diag_console_start(void)
             { "watch",    "stream decoded frames ~1Hz: watch [on|off]", NULL, cmd_watch, NULL, NULL },
             { "decode",   "offline-decode a pasted hex frame", NULL, cmd_decode, NULL, NULL },
             { "selftest", "compact on-target codec self-check", NULL, cmd_selftest, NULL, NULL },
+            { "tx",       "#52 bench probe: tx <offset> <value> — current frame, one byte overridden",
+                          NULL, cmd_tx, NULL, NULL },
         };
         for (size_t i = 0; i < sizeof(cmds) / sizeof(cmds[0]); i++) esp_console_cmd_register(&cmds[i]);
     }

firmware/esp32-matter/main/diag_console.h

@@ -1,6 +1,6 @@
 // Diagnostic telnet console (:2323) embedded in the esp-matter A/C firmware, so the
 // node stays a Matter device AND exposes recon-style diagnostics over the network:
-//   nc <node-ip> 2323   ->  token | poll | watch | decode | selftest | help
+//   nc <node-ip> 2323   ->  token | poll | watch | decode | selftest | tx | help
 #pragma once
 #include "hisense_rs485.h"
 
@@ -16,6 +16,11 @@ void diag_console_start(void);
 // bus callback while the CHIP stack lock is held.
 void diag_on_status(const HisenseState *st);
 
+// Implemented in app_main.cpp. Sends the current command frame with ONE payload byte
+// overridden (see hisense_build_command_override). Backs the console's `tx`.
+// Returns 0 sent, -1 offset/build rejected, -2 TX queue full.
+int diag_tx_override(int off, uint8_t val);
+
 #ifdef __cplusplus
 }
 #endif

firmware/esp32-matter/sdkconfig.defaults

@@ -46,8 +46,8 @@ CONFIG_CUSTOM_DEVICE_INFO_PROVIDER=y
 # so it (and the fallback NUMBER) MUST stay equal to PROJECT_VER or the device reports a stale
 # softwareVersionString. Edit PROJECT_VER in CMakeLists.txt, then update both lines below.
 # esp32-lint.sh enforces this equality (fails the commit/CI if they drift).
-CONFIG_DEVICE_SOFTWARE_VERSION_NUMBER=10010
-CONFIG_DEVICE_SOFTWARE_VERSION_STRING="1.0.10"
+CONFIG_DEVICE_SOFTWARE_VERSION_NUMBER=10011
+CONFIG_DEVICE_SOFTWARE_VERSION_STRING="1.0.11"
 # --- OTA hardening (faster + reliable on marginal Wi-Fi) ---
 # Delta OTA: ship a diff (tens of KB) instead of the full ~1.5MB image over BDX.
 CONFIG_ENABLE_DELTA_OTA=y

firmware/src/rs485-driver/hisense_rs485.cpp

@@ -347,7 +347,14 @@ size_t hisense_build_sleep_frame(uint8_t profile, uint8_t *out, size_t out_cap)
     return hisense_build_single_field(17, v, out, out_cap);
 }
 
-size_t hisense_build_command(const HisenseCommand *cmd, uint8_t *out, size_t out_cap)
+/* Shared body for hisense_build_command() and hisense_build_command_override().
+ * ovr_off < 0 means "no override" and reproduces the original behaviour byte for
+ * byte, so the golden vectors keep passing. A non-negative ovr_off patches ONE
+ * pre-checksum byte just before finalize_frame(), which is the only correct place
+ * to do it: the checksum is computed over the patched frame and the 0xF4 stuffing
+ * happens afterwards, so callers can address true frame offsets and ignore both. */
+static size_t build_command_impl(const HisenseCommand *cmd, uint8_t *out, size_t out_cap,
+                                 int ovr_off, uint8_t ovr_val)
 {
     if (cmd == NULL || out == NULL || out_cap < HISENSE_CMD_FRAME_LEN + 2) {
         return 0;
@@ -448,15 +455,46 @@ size_t hisense_build_command(const HisenseCommand *cmd, uint8_t *out, size_t out
     // offset 35: baseline filler 0x00 (majority of single-purpose samples).
     frame[35] = 0x00;
 
-    // offset 36: display. 0xC0 on / 0x40 off / 0x00 "leave alone".
-    frame[36] = cmd->display_on ? 0xC0 : 0x00;
+    // offset 36: display. 0xC0 on / 0x40 off / 0x00 "leave alone". All three CONFIRMED
+    // on hardware 2026-07-19 (#52) -- previously this sent 0x00 for "off", i.e. the
+    // leave-alone value, which is why the ep9 Display switch never turned the panel off.
+    switch (cmd->display) {
+    case HISENSE_DISPLAY_ON:  frame[36] = 0xC0; break;
+    case HISENSE_DISPLAY_OFF: frame[36] = 0x40; break;
+    case HISENSE_DISPLAY_NOCHANGE:
+    default:                  frame[36] = 0x00; break;
+    }
+
+    // Bench override (#52 display-byte hunt): patch one payload byte AFTER the
+    // normal packing so the frame is otherwise a known-good current-state command,
+    // and BEFORE the checksum so the result is still a valid frame on the wire.
+    if (ovr_off >= 0 && (size_t) ovr_off < HISENSE_CMD_FRAME_LEN) {
+        frame[ovr_off] = ovr_val;
+    }
 
     finalize_frame(frame, HISENSE_CMD_CHK_OFFSET, HISENSE_CMD_END_OFFSET);
 
     // Byte-stuff an 0xF4 checksum byte before it goes on the wire.
     return hisense_stuff_checksum(frame, HISENSE_CMD_FRAME_LEN, out, out_cap);
 }
 
+size_t hisense_build_command(const HisenseCommand *cmd, uint8_t *out, size_t out_cap)
+{
+    return build_command_impl(cmd, out, out_cap, -1, 0);
+}
+
+size_t hisense_build_command_override(const HisenseCommand *cmd, uint8_t *out, size_t out_cap,
+                                      int ovr_off, uint8_t ovr_val)
+{
+    // Refuse the header and the checksum/terminator: patching those produces a
+    // frame the A/C drops (or, worse, a reframe), which reads as "this offset does
+    // nothing" and would silently poison a sweep.
+    if (ovr_off < (int) HISENSE_CMD_HEADER_LEN || ovr_off >= (int) HISENSE_CMD_CHK_OFFSET) {
+        return 0;
+    }
+    return build_command_impl(cmd, out, out_cap, ovr_off, ovr_val);
+}
+
 /* ---------------------------------------------------------------------------
  * Status parsing (input already un-stuffed by the bus task)
  * -------------------------------------------------------------------------*/

firmware/src/rs485-driver/hisense_rs485.h

@@ -209,6 +209,23 @@ typedef enum {
     // simultaneously will just pick turbo.
 } HisenseFeature;
 
+/* Panel display / LED, frame[36] (= payload @20). All three values CONFIRMED on the
+ * bench 2026-07-19 against a live W41H1 (issue #52): 0x40 darkened the panel and 0xC0
+ * lit it again, with every other byte held at the unit's current state.
+ *
+ * The tri-state is REQUIRED, not a nicety. `display` rides the combined command frame,
+ * so it is present on every mode / setpoint / fan / swing change. A plain bool mapping
+ * false->0x40 would force the display off on every one of those commands; NOCHANGE's
+ * 0x00 ("leave alone") is what keeps ordinary traffic from touching the panel. Only an
+ * explicit display request should send ON/OFF, and it should be one-shot (reset to
+ * NOCHANGE after the frame goes out), otherwise later frames keep re-asserting it and
+ * fight the user's remote. */
+typedef enum {
+    HISENSE_DISPLAY_NOCHANGE = 0,  // frame[36]=0x00 leave the panel alone (default)
+    HISENSE_DISPLAY_ON       = 1,  // frame[36]=0xC0 CONFIRMED lights the panel
+    HISENSE_DISPLAY_OFF      = 2,  // frame[36]=0x40 CONFIRMED darkens the panel
+} HisenseDisplay;
+
 /* ---------------------------------------------------------------------------
  * Command struct: everything hisense_build_command() can pack into ONE
  * combined "write" frame.
@@ -236,13 +253,8 @@ typedef struct {
     HisenseSwingMode vswing;
     HisenseSwingMode hswing;
     HisenseFeature   feature;          // eco / turbo / none
-    bool             display_on;       // VERIFY: 0x00 body byte 20 is
-                                        // assumed "don't touch display"; only
-                                        // display_on={true->0xC0,false->0x40}
-                                        // triggers an explicit write. There's
-                                        // no tri-state "leave alone" flag here
-                                        // -- add one if the bench test shows
-                                        // 0x00 actually forces display off.
+    HisenseDisplay   display;          // panel display/LED. CONFIRMED on the bench
+                                        // 2026-07-19 (see HisenseDisplay).
 } HisenseCommand;
 
 /* ---------------------------------------------------------------------------
@@ -462,6 +474,23 @@ void hisense_deinit(void);
 // return value, never assume HISENSE_CMD_FRAME_LEN.
 size_t hisense_build_command(const HisenseCommand *cmd, uint8_t *out, size_t out_cap);
 
+// BENCH ONLY (#52 display-byte hunt). Same frame as hisense_build_command(), with
+// exactly ONE pre-checksum byte replaced. Every other byte stays at the caller's
+// current known-good state, so a failed probe is a no-op rather than a surprise
+// mode/setpoint change -- that is what makes an offset sweep safe to run against a
+// live A/C.
+//
+// `ovr_off` is an ABSOLUTE frame offset and must land in the payload:
+// [HISENSE_CMD_HEADER_LEN, HISENSE_CMD_CHK_OFFSET). Header and checksum/terminator
+// offsets are rejected (returns 0) because patching them yields a frame the A/C
+// simply drops, which is indistinguishable from "this byte does nothing" and would
+// poison a sweep with false negatives.
+//
+// Returns the on-the-wire length (byte-stuffing included), or 0 on a rejected
+// offset / the same errors as hisense_build_command().
+size_t hisense_build_command_override(const HisenseCommand *cmd, uint8_t *out, size_t out_cap,
+                                      int ovr_off, uint8_t ovr_val);
+
 // Builds the literal power on/off frame, ported byte-for-byte from
 // messages.h `on[]`/`off[]` (NOT synthesized -- these carry several bytes
 // whose individual semantics are unconfirmed, see hisense_rs485.cpp).

firmware/src/sdk-edits/matter_drivers.cpp

@@ -98,7 +98,7 @@ static const chip::EndpointId kDisplayEp   = 9;  // OnOff -> panel display (#19
  * ------------------------------------------------------------------------ */
 static HisenseCommand s_cmd = { HISENSE_MODE_COOL, 24, false,
                                 HISENSE_FAN_AUTO, HISENSE_SWING_OFF,
-                                HISENSE_SWING_OFF, HISENSE_FEATURE_NONE, false };
+                                HISENSE_SWING_OFF, HISENSE_FEATURE_NONE, HISENSE_DISPLAY_NOCHANGE };
 
 /* Latest parsed A/C status, written by the bus task, read by the downlink
  * handler. Poll cadence is seconds apart so a plain snapshot copy is adequate. */
@@ -494,13 +494,18 @@ static void hisense_apply_mute(bool on)
     if (n) hisense_send_frame(f, n);
 }
 
-/* Panel display on/off (#19 parity with the esp32 build). display_on rides the
- * combined command frame (@20: 0xC0 on / 0x40 off); flush rebuilds + sends it.
- * Write-only -- the A/C reports no display state, so there's no status echo. */
+/* Panel display on/off (#19 parity with the esp32 build). `display` rides the
+ * combined command frame (@20: 0xC0 on / 0x40 off / 0x00 leave-alone); flush rebuilds
+ * + sends it. Write-only -- the A/C reports no display state, so there's no status echo.
+ *
+ * ONE-SHOT (#52): reset to NOCHANGE afterwards. The field is packed into EVERY combined
+ * command, so a latched ON/OFF would re-assert the panel on every later mode/setpoint/fan
+ * change and fight the user's remote. */
 static void hisense_apply_display(bool on)
 {
-    s_cmd.display_on = on;
+    s_cmd.display = on ? HISENSE_DISPLAY_ON : HISENSE_DISPLAY_OFF;
     hisense_flush_command();
+    s_cmd.display = HISENSE_DISPLAY_NOCHANGE;
 }
 
 /* Sleep profile (0=off, 1..4 = General/Old/Young/Kids). Driven from two places --

firmware/src/version.txt

@@ -1 +1 @@
-1.2.14
+1.2.15