fix: config parser/storage robustness — sized skipping, 4 KB configs, factory embed

src/factory_config.c

@@ -0,0 +1,79 @@
+#include "factory_config.h"
+
+#include <stdio.h>
+#include <string.h>
+
+#include "opendisplay_config_storage.h"
+
+/*
+ * CRC-16/CCITT-FALSE over the outer config packet body with the two length
+ * bytes forced to zero — the canonical toolbox config CRC, matching the
+ * reference firmware (factory_config.cpp toolboxOuterCrc) and the generator
+ * (tools/config_packet.py outer_packet_crc). Kept local so this file has no
+ * dependency on the parser's static helper.
+ */
+static uint16_t factory_outer_crc16(const uint8_t *data, uint32_t body_len)
+{
+  uint16_t crc = 0xFFFFu;
+
+  for (uint32_t i = 0; i < body_len; i++) {
+    const uint8_t b = (i < 2u) ? 0u : data[i];
+
+    crc ^= (uint16_t)((uint16_t)b << 8);
+    for (int bit = 0; bit < 8; bit++) {
+      if ((crc & 0x8000u) != 0u) {
+        crc = (uint16_t)(((uint32_t)crc << 1) ^ 0x1021u);
+      } else {
+        crc = (uint16_t)((uint32_t)crc << 1);
+      }
+    }
+  }
+  return crc;
+}
+
+static bool factory_packet_valid(const uint8_t *data, uint32_t len)
+{
+  if (len < 4u || len > MAX_CONFIG_SIZE) {
+    return false;
+  }
+  const uint16_t declared = (uint16_t)data[0] | ((uint16_t)data[1] << 8);
+
+  if (declared != len) {
+    return false;
+  }
+  const uint16_t calc = factory_outer_crc16(data, len - 2u);
+  const uint16_t given = (uint16_t)data[len - 2u] | ((uint16_t)data[len - 1u] << 8);
+
+  return given == calc;
+}
+
+#ifdef FACTORY_HAS_EMBED
+static bool factory_embed_present(const factory_flash_cfg_t *fc)
+{
+  if (fc == NULL || fc->magic != FACTORY_CFG_MAGIC) {
+    return false;
+  }
+  if (fc->len < 4u || fc->len > MAX_CONFIG_SIZE) {
+    return false;
+  }
+  return factory_packet_valid(fc->data, fc->len);
+}
+#endif
+
+bool tryProvisionFactoryEmbed(void)
+{
+#ifdef FACTORY_HAS_EMBED
+  if (!factory_embed_present(&g_factory_embed)) {
+    return false;
+  }
+
+  printf("No valid stored config; provisioning from factory embed...\r\n");
+  if (saveConfig((uint8_t *)(void *)g_factory_embed.data, g_factory_embed.len)) {
+    printf("Factory config saved to settings\r\n");
+    return true;
+  }
+
+  printf("ERROR: Factory embed present but saveConfig failed\r\n");
+#endif
+  return false;
+}

src/factory_config.h

@@ -0,0 +1,37 @@
+#ifndef FACTORY_CONFIG_H
+#define FACTORY_CONFIG_H
+
+#include <stdbool.h>
+#include <stdint.h>
+
+#include "opendisplay_config_storage.h" /* MAX_CONFIG_SIZE */
+
+/*
+ * Optional build-time factory config embed. scripts/factory_config_gen.py runs as
+ * a PlatformIO pre-build step and (when OPENDISPLAY_FACTORY_CONFIG_HEX / the
+ * custom_factory_config_hex option is set) writes src/generated/factory_config_data.c
+ * defining g_factory_embed and adds -DFACTORY_HAS_EMBED. Otherwise it writes a
+ * stub and this file's provisioning path compiles to a no-op. Ported from the
+ * reference firmware (Firmware/src/factory_config.{h,cpp}); the generator's
+ * MAX_PACKET (tools/config_packet.py) equals MAX_CONFIG_SIZE so data[] fits the
+ * padded blob exactly.
+ */
+#define FACTORY_CFG_MAGIC 0xFAC70A5Au
+
+typedef struct __attribute__((packed)) {
+  uint32_t magic;
+  uint32_t len;
+  uint8_t data[MAX_CONFIG_SIZE];
+} factory_flash_cfg_t;
+
+#ifdef FACTORY_HAS_EMBED
+extern const factory_flash_cfg_t g_factory_embed;
+#endif
+
+/*
+ * If a valid factory embed is present and no valid config is stored yet, persist
+ * the embedded packet to settings/NVS. Returns true only if a config was written.
+ */
+bool tryProvisionFactoryEmbed(void);
+
+#endif

src/opendisplay_ble.c

@@ -7,6 +7,7 @@
 #include "opendisplay_led.h"
 #include "opendisplay_buzzer.h"
 #include "opendisplay_pipe.h"
+#include "factory_config.h"
 #include "opendisplay_battery.h"
 #include "opendisplay_sensor_sht40.h"
 #include "opendisplay_sensor_bq27220.h"
@@ -527,7 +528,17 @@ void opendisplay_ble_init(void)
 	int err;
 
 	(void)initConfigStorage();
-	if (loadGlobalConfig(&s_od_global_config)) {
+#ifdef FACTORY_CLEAR_CONFIG_ON_BOOT
+	/* One-shot clear build (scripts/factory_config_gen.py). */
+	printf("[OD] factory clear build: erasing stored config\r\n");
+	(void)clearStoredConfig();
+#endif
+	bool config_loaded = loadGlobalConfig(&s_od_global_config);
+	if (!config_loaded && tryProvisionFactoryEmbed()) {
+		/* No valid stored config, but a factory embed was just provisioned. */
+		config_loaded = loadGlobalConfig(&s_od_global_config);
+	}
+	if (config_loaded) {
 		printf("[OD] config loaded: displays=%u\r\n",
 		       (unsigned)s_od_global_config.display_count);
 	} else {

src/opendisplay_config_parser.c

@@ -99,6 +99,45 @@ static uint16_t config_toolbox_outer_crc16(const uint8_t *data, uint32_t body_le
     return crc;
 }
 
+/*
+ * On-wire data size (excluding the 2-byte [number][type] header) of every known
+ * config packet type. Returns 0 for a genuinely unknown type.
+ *
+ * These sizes are cross-checked three ways and all agree: the reference firmware
+ * structs (Firmware/src/structs.h), this port's structs (opendisplay_structs.h),
+ * and the py-opendisplay serializer docstrings (protocol/config_serializer.py).
+ * The one place the old code disagreed with the wire was wifi_config (0x26):
+ * this port skipped a hardcoded 162 bytes, but the serializer emits 160
+ * ("Serialize WifiConfig to 160 bytes") and the reference struct is 160 — the
+ * extra 2 bytes desynced every packet after wifi. Corrected to 160 here.
+ *
+ * 0x28 (touch_controller) and 0x29 (passive_buzzer) have no parse case on this
+ * branch, but are sized here so the default branch skips them by their true size
+ * instead of dropping every packet after them. Sibling PRs add real parse cases;
+ * the sized skip is exactly what lets those branches stay independent.
+ */
+static uint16_t config_packet_data_size(uint8_t packetId)
+{
+    switch (packetId) {
+        case CONFIG_PKT_SYSTEM:        return 22u;   /* 0x01 system_config      */
+        case CONFIG_PKT_MANUFACTURER:  return 22u;   /* 0x02 manufacturer_data  */
+        case CONFIG_PKT_POWER:         return 30u;   /* 0x04 power_option       */
+        case CONFIG_PKT_DISPLAY:       return 46u;   /* 0x20 display            */
+        case CONFIG_PKT_LED:           return 22u;   /* 0x21 led                */
+        case CONFIG_PKT_SENSOR:        return 30u;   /* 0x23 sensor_data        */
+        case CONFIG_PKT_DATA_BUS:      return 30u;   /* 0x24 data_bus           */
+        case CONFIG_PKT_BINARY_INPUT:  return 30u;   /* 0x25 binary_inputs      */
+        case CONFIG_PKT_WIFI:          return 160u;  /* 0x26 wifi_config        */
+        case CONFIG_PKT_SECURITY:      return 64u;   /* 0x27 security_config    */
+        case CONFIG_PKT_TOUCH:         return 32u;   /* 0x28 touch_controller   */
+        case CONFIG_PKT_PASSIVE_BUZZER:return 32u;   /* 0x29 passive_buzzer     */
+        case CONFIG_PKT_NFC:           return 32u;   /* 0x2A nfc_config         */
+        case CONFIG_PKT_FLASH:         return 32u;   /* 0x2B flash_config       */
+        case CONFIG_PKT_DATA_EXTENDED: return 288u;  /* 0x2C data_extended      */
+        default:                       return 0u;
+    }
+}
+
 bool parseConfigBytes(uint8_t* configData, uint32_t configLen, struct GlobalConfig* globalConfig) {
     if (globalConfig == NULL || configData == NULL) {
         printf("Invalid parameters for parseConfigBytes\n");
@@ -377,6 +416,12 @@ bool parseConfigBytes(uint8_t* configData, uint32_t configLen, struct GlobalConf
                 }
                 break;
 
+            case CONFIG_PKT_WIFI: // wifi_config (0x26)
+                /* The nRF54 radio has no Wi-Fi, but the packet is still parsed and
+                 * stored (not skipped) so a client's Wi-Fi settings survive a config
+                 * read-back. The old code skipped a hardcoded 162 bytes; the packet
+                 * is 160 bytes on the wire, so that off-by-2 desynced every packet
+                 * after wifi. */
             case CONFIG_PKT_PASSIVE_BUZZER: // passive_buzzer (0x29) - parse but don't log
                 if (offset > configLen) {
                     printf("Offset overflow before passive_buzzer\r\n");
@@ -412,14 +457,18 @@ bool parseConfigBytes(uint8_t* configData, uint32_t configLen, struct GlobalConf
                     globalConfig->loaded = false;
                     return false;
                 }
-                if (offset + 162 <= configLen - 2) {
-                    offset += 162;
+                if (offset + sizeof(struct WifiConfig) <= configLen - 2) {
+                    memcpy(&globalConfig->wifi_config, &configData[offset], sizeof(struct WifiConfig));
+                    globalConfig->wifi_config_loaded = true;
+                    offset += sizeof(struct WifiConfig);
                     if (offset > configLen) {
                         printf("Offset overflow after wifi\r\n");
                         globalConfig->loaded = false;
                         return false;
                     }
                 } else {
+                    printf("wifi_config: need %zu, have %u\r\n",
+                           sizeof(struct WifiConfig), (unsigned)(configLen - 2 - offset));
                     offset = configLen - 2; // Skip to CRC
                 }
                 break;
@@ -532,10 +581,34 @@ bool parseConfigBytes(uint8_t* configData, uint32_t configLen, struct GlobalConf
                 }
                 break;
 
-            default:
-                printf("Unknown pkt 0x%02X @%u\r\n", packetId, (unsigned)(offset - 2));
-                offset = configLen - 2; // Skip to CRC
+            default: {
+                /*
+                 * A type with no parse case above (e.g. 0x28 touch / 0x29 buzzer on
+                 * this branch). Skip it by its known on-wire size so later packets
+                 * are still parsed, instead of jumping to the CRC and silently
+                 * dropping everything after it. Only a genuinely unknown type ID
+                 * (not in the size table) forces skip-to-CRC, because the TLV format
+                 * carries no per-packet length to recover from.
+                 */
+                uint16_t knownSize = config_packet_data_size(packetId);
+                if (knownSize != 0u) {
+                    if (offset + knownSize <= configLen - 2) {
+                        printf("Known-unparsed pkt 0x%02X @%u, skipping %u B\r\n",
+                               packetId, (unsigned)(offset - 2), (unsigned)knownSize);
+                        offset += knownSize;
+                    } else {
+                        printf("Known-unparsed pkt 0x%02X @%u: need %u, have %u\r\n",
+                               packetId, (unsigned)(offset - 2), (unsigned)knownSize,
+                               (unsigned)(configLen - 2 - offset));
+                        offset = configLen - 2; // Truncated packet; stop.
+                    }
+                } else {
+                    printf("Unknown pkt 0x%02X @%u, skip-to-CRC (drops later pkts)\r\n",
+                           packetId, (unsigned)(offset - 2));
+                    offset = configLen - 2; // Skip to CRC
+                }
                 break;
+            }
         }
     }
 

src/opendisplay_config_storage.c

@@ -35,7 +35,11 @@ bool initConfigStorage(void)
 
 bool saveConfig(uint8_t *config_data, uint32_t len)
 {
-	opendisplay_config_storage_t rec;
+	/* static, not on the stack: opendisplay_config_storage_t is ~4 KB with the
+	 * 4096-byte MAX_CONFIG_SIZE, which would blow CONFIG_MAIN_STACK_SIZE (4096).
+	 * Config writes are serialized on the main thread, so a single scratch is
+	 * safe. */
+	static opendisplay_config_storage_t rec;
 
 	if (len > MAX_CONFIG_SIZE) {
 		return false;
@@ -46,15 +50,22 @@ bool saveConfig(uint8_t *config_data, uint32_t len)
 	rec.data_len = len;
 	rec.crc = calculateConfigCRC(config_data, len);
 	memcpy(rec.data, config_data, len);
+	/* Commit the RAM cache only after the write succeeds: on failure the cache
+	 * must keep reporting the last persisted config, not an unsaved one. */
+	if (settings_save_one(OD_SETTINGS_KEY, &rec,
+			      offsetof(opendisplay_config_storage_t, data) + len) != 0) {
+		return false;
+	}
 	s_cached = rec;
 	s_loaded = true;
-	return settings_save_one(OD_SETTINGS_KEY, &rec,
-				 offsetof(opendisplay_config_storage_t, data) + len) == 0;
+	return true;
 }
 
 bool loadConfig(uint8_t *config_data, uint32_t *len)
 {
-	opendisplay_config_storage_t rec;
+	/* static for the same reason as saveConfig: the record is ~4 KB and must
+	 * not sit on the 4 KB main stack. */
+	static opendisplay_config_storage_t rec;
 	ssize_t got;
 
 	if (config_data == NULL || len == NULL) {

src/opendisplay_config_storage.h

@@ -4,7 +4,18 @@
 #include <stdbool.h>
 #include <stdint.h>
 
-#define MAX_CONFIG_SIZE 512
+/*
+ * Matches the reference firmware (Firmware/src/config_parser.h) and the factory
+ * generator's MAX_PACKET (scripts/factory_config_gen.py / tools/config_packet.py).
+ * The stored record is [magic4][version4][crc4][data_len4][data[len]] = 16 + len
+ * bytes, saved as a single Zephyr settings/NVS item. The nRF54L RRAM NVS sector
+ * is 4096 B, so the largest storable record is ~sector - 4*ATE = 4064 B. The BLE
+ * write paths cap an inbound config at MAX_CONFIG_CHUNKS(20)*CONFIG_CHUNK_SIZE(200)
+ * = 4000 B (chunked) or 200 B (single-shot), so any client-writable config fits
+ * (16 + 4000 = 4016 B < 4064 B). A blob larger than that is unreachable over BLE
+ * and, if ever provisioned, fails cleanly via settings_save_one (see saveConfig).
+ */
+#define MAX_CONFIG_SIZE 4096
 
 typedef struct {
   uint32_t magic;

src/opendisplay_constants.h

@@ -11,6 +11,7 @@
 #define CONFIG_PKT_BINARY_INPUT   0x25
 #define CONFIG_PKT_WIFI           0x26
 #define CONFIG_PKT_SECURITY       0x27
+#define CONFIG_PKT_TOUCH          0x28
 #define CONFIG_PKT_PASSIVE_BUZZER 0x29
 #define CONFIG_PKT_NFC            0x2A
 #define CONFIG_PKT_FLASH          0x2B

src/opendisplay_pipe.c

@@ -513,9 +513,26 @@ static void pipe_send_raw(uint8_t connection, const uint8_t *data, uint16_t len)
   if (!s_notify || len == 0u) {
     return;
   }
-  if (!opendisplay_ble_pipe_notify(data, len)) {
-    printf("[OD] pipe notify failed len=%u\r\n", (unsigned)len);
+  /*
+   * bt_gatt_notify() returns -ENOMEM (notify → false) when the TX buffer pool is
+   * momentarily exhausted; it never blocks. A single response almost always
+   * succeeds on the first try. A multi-chunk config read (now up to
+   * (MAX_CONFIG_SIZE+93)/94 = 44 notifications back-to-back) can outrun the pool,
+   * so retry with a short yield while the link is still up — buffers free as the
+   * BT RX thread processes completions. This keeps the pool small (prj.conf) yet
+   * lets large reads through without dropping chunks. Bail if notifications are
+   * no longer enabled (disconnected), so a dead link cannot spin here.
+   */
+  for (int attempt = 0; attempt < 200; attempt++) {
+    if (opendisplay_ble_pipe_notify(data, len)) {
+      return;
+    }
+    if (!opendisplay_ble_pipe_notify_enabled()) {
+      break;
+    }
+    k_msleep(1);
   }
+  printf("[OD] pipe notify failed len=%u\r\n", (unsigned)len);
 }
 
 static void pipe_send(uint8_t connection, const uint8_t *data, uint16_t len)
@@ -790,7 +807,13 @@ static void handle_config_read(uint8_t connection)
 {
   static uint8_t config_data[MAX_CONFIG_SIZE];
   uint32_t config_len = MAX_CONFIG_SIZE;
-  const uint16_t max_chunks = 10u;
+  /* Derive the chunk cap from MAX_CONFIG_SIZE like the reference firmware
+   * (communication.cpp: (MAX_CONFIG_SIZE + 93) / 94). Each chunk carries at
+   * least 94 config bytes (chunk 0: 100-byte response - 2 status - 2 chunk# - 2
+   * total-len; later chunks carry 96), so 94 is the conservative per-chunk rate.
+   * The old hardcoded 10 truncated any read past ~940 bytes. Flow control in
+   * pipe_send_raw keeps the larger burst from dropping chunks on a full TX pool. */
+  const uint16_t max_chunks = (uint16_t)((MAX_CONFIG_SIZE + 93) / 94);
 
   if (!initConfigStorage()) {
     uint8_t err[] = { 0xFFu, RESP_CONFIG_READ, 0x00u, 0x00u };