diff --git a/src/app_main.c b/src/app_main.c index 8b1bee2..d3125bd 100644 --- a/src/app_main.c +++ b/src/app_main.c @@ -33,6 +33,7 @@ static tls_os_mutex_t *s_store_lock; /* serialises all nl_settings_* / nl_state_ static nl_settings_t s_settings; static volatile uint32_t s_light_beat; static volatile bool s_factory_reset_requested; +static volatile bool s_counter_cleared; static u32 s_app_stack[1024]; static u32 s_light_stack[512]; @@ -88,6 +89,15 @@ void nl_app_reboot(void) void nl_app_request_factory_reset(void) { s_factory_reset_requested = true; } +void nl_app_clear_reset_counter(void) +{ + if (s_counter_cleared) + return; + store_lock(); + s_counter_cleared = nl_resetcnt_clear() == 0; + store_unlock(); +} + int nl_app_set_wifi_credentials(const char *ssid, const char *pass) { nl_app_lock(); @@ -149,12 +159,15 @@ static void app_task(void *arg) uint32_t base = nl_flash_region_base(); nl_engine_cfg_t cfg; nl_light_state_t last, initial, pending; - bool have_last, after_ota, counter_cleared = false; + bool have_last, after_ota; bool light_alive = false, net_alive = false, http_alive = false; uint8_t boots; uint32_t boot_ms, light_seen, net_seen, http_seen; (void)arg; + /* I2: the 5 s of spec 3.4 step 7 / 5.5 count from power-up, not from "after the recovery + * scan" - otherwise the documented "< 5 s per power cycle" window silently becomes ~8 s. */ + boot_ms = nl_app_now_ms(); nl_flash_init(); nl_led_init(board); /* hardware init first (spec 3.4): LEDs dark before any flash/state scan */ store_lock(); @@ -217,7 +230,6 @@ static void app_task(void *arg) nl_http_server_start(nl_app_api_ctx()); - boot_ms = nl_app_now_ms(); light_seen = s_light_beat; net_seen = nl_net_beat; http_seen = nl_http_beat; @@ -228,11 +240,8 @@ static void app_task(void *arg) tls_os_time_delay(HZ / 10); /* 100 ms */ now = nl_app_now_ms(); - if (!counter_cleared && now - boot_ms >= NL_RESETCNT_CLEAR_MS) { - store_lock(); - counter_cleared = nl_resetcnt_clear() == 0; - store_unlock(); - } + if (!s_counter_cleared && now - boot_ms >= NL_RESETCNT_CLEAR_MS) + nl_app_clear_reset_counter(); if (s_factory_reset_requested) nl_app_factory_reset(); /* does not return */ diff --git a/src/net/nl_recovery.c b/src/net/nl_recovery.c index 65271aa..8117a5a 100644 --- a/src/net/nl_recovery.c +++ b/src/net/nl_recovery.c @@ -109,9 +109,21 @@ void nl_recovery_try(void) { char url[128]; + /* I3: the SDK's auto-connect may already be joining its own stored AP in parallel (see + nl_wifi_disable_sdk_auto_connect()). Switch it off and drop that connection before we + scan, so everything that follows is our own doing. */ + nl_wifi_disable_sdk_auto_connect(); + if (!recovery_ap_present()) return; printf("recovery: station found, joining\r\n"); + /* C1: from here on this boot is a recovery boot - it may end in a flash + reset that never + returns to app_task, so the power-cycle counter must be cleared now instead of after the + 5 s deadline. Otherwise every recovery round would count as another short power cycle and + NL_FACTORY_RESET_BOOTS rounds in front of the station would wipe Wi-Fi, token and name. + Trade-off: while a rescue station is in range, the "5 x power cycle" factory reset does + not work - a lamp in front of the station is being serviced, not being reset. */ + nl_app_clear_reset_counter(); if (!nl_wifi_join_blocking(NINLIGHT_RECOVERY_SSID, NINLIGHT_RECOVERY_PASSWORD, NL_RECOVERY_JOIN_MS)) { printf("recovery: join failed\r\n"); nl_ota_set_error("download"); @@ -119,7 +131,9 @@ void nl_recovery_try(void) } snprintf(url, sizeof url, "http://192.168.8.1/ninlight_%s_ota.img", NINLIGHT_DEVICE_ID); printf("recovery: fetching %s\r\n", url); - if (nl_ota_start(url) != 0) { + /* C1: recovery refuses an image that carries the running version - the station serves the + same file to every lamp and stays up for a whole migration batch. */ + if (nl_ota_start_recovery(url) != 0) { printf("recovery: cannot start download\r\n"); return; } diff --git a/src/net/nl_wifi.h b/src/net/nl_wifi.h index 2b0a61b..7704815 100644 --- a/src/net/nl_wifi.h +++ b/src/net/nl_wifi.h @@ -21,6 +21,11 @@ extern volatile uint32_t nl_net_beat; /* Starts the network task. An empty ssid starts the onboarding AP. Strings are copied. */ void nl_wifi_start(const char *ssid, const char *pass, const char *ap_pass); +/* Switches the SDK's own auto-connect off and drops any connection it may already have brought + * up on its own. nl_wifi_start() does this too, but the boot recovery path runs before that and + * must not have the SDK joining its stored AP in parallel (I3, final review). Safe to call any + * number of times; only writes the SDK parameter when it is actually set. */ +void nl_wifi_disable_sdk_auto_connect(void); /* Joins a station directly (no nl_wifi_sm) and blocks until it gets an IP or timeout_ms * elapses; returns true on success. For use before nl_wifi_start() only - e.g. the boot * recovery path in nl_recovery.c - since it drives the SDK Wi-Fi calls itself instead of diff --git a/src/net/nl_wifi_w600.c b/src/net/nl_wifi_w600.c index f3201a7..acf0493 100644 --- a/src/net/nl_wifi_w600.c +++ b/src/net/nl_wifi_w600.c @@ -69,6 +69,25 @@ static void clear_softap_param(void) } } +/* I3: tls_sys_auto_mode_run() (sdk/Platform/Sys/tls_sys.c) kicks off an SDK-driven join right + * after UserMain() whenever the persisted auto-connect flag is on - and tls_sys.c:264-267 turns a + * temporary WIFI_AUTO_CNT_TMP_OFF back into WIFI_AUTO_CNT_ON and persists that, so the factory + * default "off" is not something we may rely on. Any such join is indistinguishable from our own + * in the netif status events, so switch it off (and drop a connection it already made) before the + * recovery scan, not only in nl_wifi_start(). */ +void nl_wifi_disable_sdk_auto_connect(void) +{ + u8 auto_mode = WIFI_AUTO_CNT_OFF; + + tls_wifi_auto_connect_flag(WIFI_AUTO_CNT_FLAG_GET, &auto_mode); + if (auto_mode != WIFI_AUTO_CNT_OFF) { + printf("wifi: SDK auto-connect was %u, switching it off\r\n", auto_mode); + auto_mode = WIFI_AUTO_CNT_OFF; + tls_wifi_auto_connect_flag(WIFI_AUTO_CNT_FLAG_SET, &auto_mode); + } + tls_wifi_disconnect(); +} + static void start_ap(void) { struct tls_softap_info_t ap; @@ -220,7 +239,6 @@ static void net_task(void *arg) void nl_wifi_start(const char *ssid, const char *pass, const char *ap_pass) { - u8 auto_mode = WIFI_AUTO_CNT_OFF; nl_wifi_actions_t a; struct netif *nif; @@ -233,12 +251,7 @@ void nl_wifi_start(const char *ssid, const char *pass, const char *ap_pass) copy(s_ap_pass, ap_pass, sizeof s_ap_pass); /* The SDK would otherwise reconnect on its own with credentials from its parameter area. */ - tls_wifi_auto_connect_flag(WIFI_AUTO_CNT_FLAG_GET, &auto_mode); - if (auto_mode != WIFI_AUTO_CNT_OFF) { - auto_mode = WIFI_AUTO_CNT_OFF; - tls_wifi_auto_connect_flag(WIFI_AUTO_CNT_FLAG_SET, &auto_mode); - } - tls_wifi_disconnect(); + nl_wifi_disable_sdk_auto_connect(); if (tls_wifi_softap_get_state() != WM_WIFI_DISCONNECTED) { /* C2: the SDK already brought up its own softap (e.g. from NETIF_IP_NET_UP before * we got a chance to run) - tear it down, our state machine owns AP lifecycle. */ @@ -369,5 +382,21 @@ bool nl_wifi_join_blocking(const char *ssid, const char *pass, uint32_t timeout_ tls_wifi_disconnect(); /* cancel a stuck/timed-out join attempt */ return false; } + /* I3 backstop: NETIF_IP_NET_UP carries no SSID, so an SDK-initiated join to its own stored AP + * would look exactly like ours - and the lamp would then fetch http://192.168.8.1/... from a + * foreign network. tls_wifi_get_current_bss() does report the SSID actually associated with + * (sdk/Include/WiFi/wm_wifi.h:190), so verify it before the caller starts a download. */ + { + struct tls_curr_bss_t bss; + size_t n = strlen(ssid); + + memset(&bss, 0, sizeof bss); + tls_wifi_get_current_bss(&bss); + if (bss.ssid_len != (u8)n || memcmp(bss.ssid, ssid, n) != 0) { + printf("wifi: joined a different network than %s, dropping the connection\r\n", ssid); + tls_wifi_disconnect(); + return false; + } + } return true; } diff --git a/src/sys/nl_app.h b/src/sys/nl_app.h index 0f454af..df355e7 100644 --- a/src/sys/nl_app.h +++ b/src/sys/nl_app.h @@ -28,4 +28,10 @@ void nl_app_request_factory_reset(void); /* Stores new station credentials (from a successful onboarding test) and persists them. */ int nl_app_set_wifi_credentials(const char *ssid, const char *pass); +/* Clears the power-cycle counter ahead of the regular 5 s deadline (spec 5.5), idempotent. + * Used by the boot recovery path once it knows this boot is a recovery boot: a successful + * recovery never returns to the app task, so without this the counter would keep growing + * over recovery cycles and trigger a factory reset after NL_FACTORY_RESET_BOOTS rounds. */ +void nl_app_clear_reset_counter(void); + #endif diff --git a/src/sys/nl_ota.c b/src/sys/nl_ota.c index 60731fa..99360b0 100644 --- a/src/sys/nl_ota.c +++ b/src/sys/nl_ota.c @@ -10,6 +10,8 @@ #include "sys/nl_ota.h" #include "util/nl_fwup_trailer.h" +#include "nl_version.h" + /* t_http_fwup() (sdk/Src/App/OTA/wm_http_fwup.c) has no declaration in any SDK header. */ extern int t_http_fwup(char *url); @@ -19,8 +21,10 @@ extern int t_http_fwup(char *url); static char s_url[NL_OTA_URL_MAX]; static volatile bool s_busy; +static volatile bool s_reject_running_version; /* recovery path only, see nl_ota_start_recovery() */ static const char * volatile s_error; static u32 s_stack[NL_OTA_STACK]; +static char s_tail_version[NL_TRAILER_VERSION_LEN]; /* used by the hook, kept off its 2 KB stack */ void nl_ota_init(void) { @@ -42,10 +46,28 @@ bool nl_fwup_verify_tail(const uint8_t sha256[32], const uint8_t *tail, uint32_t { nl_trailer_err_t e = nl_trailer_verify(tail, tail_len, NINLIGHT_DEVICE_ID, sha256); - if (e != NL_TRAILER_OK) + if (e != NL_TRAILER_OK) { s_error = nl_trailer_err_name(e); - printf("ota: trailer %s\r\n", e == NL_TRAILER_OK ? "ok" : s_error); - return e == NL_TRAILER_OK; + printf("ota: trailer %s\r\n", s_error); + return false; + } + /* C1: recovery only - the rescue station serves the same image to every lamp that boots in + * front of it, so an image that already matches the running version must be refused here; + * otherwise the lamp flashes and reboots in an endless loop. No new last_ota_error value is + * invented for this (the vocabulary is fixed in spec 8.2); the log line below is what tells + * the two cases apart. */ + if (s_reject_running_version) { + nl_trailer_version(tail, s_tail_version); + if (strcmp(s_tail_version, NINLIGHT_FW_VERSION) == 0) { + printf("ota: recovery image already carries the running version %s, not flashing\r\n", + s_tail_version); + s_error = "trailer"; + return false; + } + printf("ota: recovery image version %s (running %s)\r\n", s_tail_version, NINLIGHT_FW_VERSION); + } + printf("ota: trailer ok\r\n"); + return true; } /* Logs the URL without any "user:pass@" userinfo, so credentials embedded in the URL never @@ -83,7 +105,7 @@ static void ota_task(void *arg) tls_os_task_del(NL_OTA_PRIO, NULL); } -int nl_ota_start(const char *url) +static int ota_start(const char *url, bool reject_running_version) { size_t n; @@ -98,6 +120,7 @@ int nl_ota_start(const char *url) } s_busy = true; s_error = NULL; + s_reject_running_version = reject_running_version; memcpy(s_url, url, n + 1); /* Persist the open light state and mark the pending update before the flash gets busy. */ @@ -122,8 +145,17 @@ int nl_ota_start(const char *url) nl_app_unlock(); (void)nl_app_save_settings(); s_busy = false; + /* Minor (final review): no value in the fixed vocabulary of spec 8.2 describes "could not + * create the download task" (out of RAM/TCBs), and "download" would claim a transfer that + * never happened. `busy` stays the closest fit - the chip really is too busy to start one - + * and this log line is what distinguishes it from "an update is already running". */ + printf("ota: cannot create download task (out of resources)\r\n"); s_error = "busy"; return -1; } return 0; } + +int nl_ota_start(const char *url) { return ota_start(url, false); } + +int nl_ota_start_recovery(const char *url) { return ota_start(url, true); } diff --git a/src/sys/nl_ota.h b/src/sys/nl_ota.h index e1e6c7f..62d0d6b 100644 --- a/src/sys/nl_ota.h +++ b/src/sys/nl_ota.h @@ -6,6 +6,11 @@ void nl_ota_init(void); /* Starts a download task for an http:// URL. 0 = started, -1 = busy or invalid. */ int nl_ota_start(const char *url); +/* Same, but for the boot recovery path (spec 8.3): an image whose trailer carries the running + * firmware version is rejected instead of flashed. The rescue station stays up while a whole + * batch of lamps migrates, so without this check every boot would re-flash the same image + * forever. A deliberate re-flash of the same version stays possible over the REST API. */ +int nl_ota_start_recovery(const char *url); bool nl_ota_busy(void); /* NULL, or one of: download, crc, trailer, device_mismatch, hash, busy */ const char *nl_ota_last_error(void);