fix(settings): neue Felder erst ab dem alten sizeof - sonst war sACN nach dem Update aus

Ein Datensatz aus der Firmware vor diesem Zweig ist 208 Byte lang und traegt hinter
wake_on_change (206) noch ein Fuellbyte. sacn_enabled lag auf genau diesem Byte (207),
und read_rec() kopiert hdr.length Bytes ueber die Vorgaben - jede Lampe im Feld waere
nach der OTA mit sacn_enabled == 0 hochgekommen und haette keinen Socket gebunden,
genau das Gegenteil dessen, was Spezifikation und README versprechen.

_legacy_pad schiebt das erste neue Feld auf 208; _padding schrumpft auf 1 Byte, damit
sizeof ein Vielfaches von 4 bleibt (216). nl_settings_legacy_check pinnt den Offset zur
Uebersetzungszeit fest, denn die bisherige Pruefung sah nur die Teilbarkeit durch 4 und
liess diesen Fall durch. Der Test kuerzte auf offsetof(sacn_enabled) - eine Laenge, die
es auf keiner Lampe je gab - und ging deshalb durch; er kuerzt jetzt auf die Literalzahl
208 und stellt das Fuellbyte nach.

src/config/nl_settings.c

@@ -29,9 +29,13 @@ typedef struct {
 
 /* Where read_rec() expects the CRC, and therefore where nl_settings_save() must put it: directly
  * behind the `length` payload bytes, not at offsetof(rec_t, crc). Both offsets are only equal
- * while sizeof(nl_settings_t) is a multiple of 4 (204 today). Adding a single uint16_t would move
+ * while sizeof(nl_settings_t) is a multiple of 4 (216 today, 208 before sACN). Adding a single
- * offsetof(rec_t, crc) two bytes further than the reader looks and every stored record would
+ * uint16_t would move offsetof(rec_t, crc) two bytes further than the reader looks and every
- * silently fail its CRC on the next boot - Wi-Fi, token and name gone on every lamp. */
+ * stored record would silently fail its CRC on the next boot - Wi-Fi, token and name gone on
+ * every lamp. Aus demselben Grund gilt fuer neue Felder: sie beginnen bei oder hinter dem
+ * vorherigen sizeof (also nicht unterhalb von 208), denn ein alter Datensatz ist genau
+ * sizeof lang und traegt seine Fuellbytes mit - ein Feld darunter wuerde beim Lesen von
+ * diesen Fuellbytes (Null) ueberschrieben statt seine Vorgabe zu behalten. */
 #define NL_SETTINGS_CRC_OFF(len) (sizeof(rec_hdr_t) + (uint32_t)(len))
 
 /* The CRC runs over header and payload as one contiguous block on both sides, so the payload
@@ -44,6 +48,14 @@ typedef char nl_settings_layout_check[offsetof(rec_t, data) == sizeof(rec_hdr_t)
  * zu entfernen. */
 typedef char nl_settings_size_check[(sizeof(nl_settings_t) % 4u == 0u) ? 1 : -1];
 
+/* Das erste mit sACN angehaengte Feld muss genau dort beginnen, wo der alte Satz aufhoerte:
+ * bei 208, dem sizeof(nl_settings_t) der Firmware vor sACN. Saetze, die eine solche Firmware
+ * geschrieben hat, sind 208 Byte lang und enthalten hinter wake_on_change (Offset 206) noch
+ * ein Fuellbyte. read_rec() kopiert hdr.length Bytes ueber die Vorgaben - ein Feld unterhalb
+ * von 208 wuerde also von diesem Fuellbyte (Null) ueberschrieben und seine Vorgabe verlieren.
+ * Bei sacn_enabled hiesse das: sACN nach dem Update auf jeder Lampe aus. */
+typedef char nl_settings_legacy_check[offsetof(nl_settings_t, sacn_enabled) == 208 ? 1 : -1];
+
 static uint32_t s_addr[2];
 
 void nl_settings_defaults(nl_settings_t *s)

src/config/nl_settings.h

@@ -28,11 +28,14 @@ typedef struct {
                                * switching on or off. */
     uint8_t  wake_on_change; /* if set, such a change also switches the lamp on when it was
                                * off, unless the same call says "on": false. */
-    uint8_t  sacn_enabled;    /* sACN/E1.31-Empfang aktiv (Vorgabe 1) */
+    uint8_t  _legacy_pad;     /* war Fuellbyte im 208-Byte-Satz: neue Felder muessen
+                               * bei oder hinter dem vorherigen sizeof beginnen, sonst
+                               * ueberschreibt das Fuellbyte eines alten Datensatzes sie */
+    uint8_t  sacn_enabled;    /* Offset 208 - sACN/E1.31-Empfang aktiv (Vorgabe 1) */
     uint16_t sacn_universe;   /* 1..63999 (Vorgabe 1) */
     uint16_t sacn_start_slot; /* 1..512 (Vorgabe 1) */
     uint8_t  sacn_hold;       /* 0 = nach dem Timeout zurueck zum HA-Zustand, 1 = halten */
-    uint8_t  _padding[2];     /* padding for 4-byte alignment of sizeof(nl_settings_t) */
+    uint8_t  _padding[1];     /* padding for 4-byte alignment of sizeof(nl_settings_t) */
 } nl_settings_t;
 
 void nl_settings_defaults(nl_settings_t *s);

test/test_settings.c

@@ -208,14 +208,21 @@ static void test_sacn_roundtrip(void)
 }
 
 /* Ein Datensatz aus einer Firmware VOR sACN: die vier Felder muessen als echte Vorgaben
- * zurueckkommen, nicht als Null - sonst waere sACN auf jeder aktualisierten Lampe aus. */
+ * zurueckkommen, nicht als Null - sonst waere sACN auf jeder aktualisierten Lampe aus.
+ * Die Laenge ist bewusst die Literalzahl 208: das war sizeof(nl_settings_t) vor diesem
+ * Zweig und damit die Laenge, die auf jeder ausgelieferten Lampe im Flash steht. Ein
+ * offsetof() auf ein heutiges Feld wuerde mit der Struktur mitwandern und genau den Fall
+ * verfehlen, um den es hier geht. Das letzte Byte (207) war im 208-Byte-Satz reines
+ * Fuellbyte und daher 0 - es wird hier nachgestellt, denn genau dieses Byte legt sich
+ * beim Lesen ueber das erste neue Feld. */
 static void test_record_without_sacn_gets_defaults(void)
 {
     nl_settings_t in = sample("alt"), out;
     uint32_t crc;
     nl_settings_save(&in);
     {
-        uint16_t shorter = (uint16_t)offsetof(nl_settings_t, sacn_enabled);
+        uint16_t shorter = 208u; /* sizeof(nl_settings_t) vor sACN */
+        fake_flash_mem[NL_FL_SETTINGS_A + 12 + 207] = 0; /* Fuellbyte des alten Satzes */
         memcpy(&fake_flash_mem[NL_FL_SETTINGS_A + 6], &shorter, 2);
         crc = nl_crc32(0, &fake_flash_mem[NL_FL_SETTINGS_A], 12 + shorter);
         memcpy(&fake_flash_mem[NL_FL_SETTINGS_A + 12 + shorter], &crc, 4);