fix: Fix-Runde 1 für Task 4 - Reset nur bei akzeptiertem OTA-Image, hängende Downloads/Sessions begrenzen

sdk/PATCHES.md

@@ -9,6 +9,28 @@ Lib/GNU, Bin/secboot.img, Tools/GNU/link_w600_2m.ld (→ ld/) und wm_tool.c (→
    (Header + Payload + 512-Byte-Endblock) bleibt unverändert. Die starke Definition des Hooks liegt seit
    Plan 3 in `src/sys/nl_ota.c` (Trailer-Prüfung über `nl_trailer_verify()`); die bisherige `weak`-Variante
    `src/sys/fwup_hook_default.c` ist entfallen.
+   Fix-Runde 1 (Task-4-Review) hat drei weitere Stellen in diesem Bereich korrigiert:
+   - `wm_fwup.c`, Bedingung um `nl_fwup_verify_tail()`: Checksummen- und Trailer-Prüfung liefen in
+     einer kurzschließenden `||`-Bedingung; bei falscher Checksumme wurde `nl_fwup_verify_tail()` nie
+     aufgerufen. Beide Fälle jetzt getrennt behandelt; ein Checksummenfehler ruft
+     `nl_ota_set_error("crc")` auf, bevor `goto request_finish` springt (sonst hätte `ota_task()` ihn
+     als generisches `"download"` gemeldet).
+   - `wm_fwup.c`, `request_finish:`-Block: `tls_sys_reset()` lief unbedingt, sobald der Transfer zu
+     Ende war - auch wenn `nl_fwup_verify_tail()` oder die Checksummenprüfung das Image gerade
+     abgelehnt hatten. Die Lampe hätte also trotz abgelehntem Image sofort neu gestartet, `ota_task()`
+     wäre nie zu Ende gelaufen und `last_ota_error` hätte nie `trailer`/`device_mismatch`/`hash`
+     gemeldet. Der Reset läuft jetzt nur noch, wenn `fwup->current_state` keinen Fehler-Bit-Satz
+     (`TLS_FWUP_STATE_ERROR`) trägt.
+   - `Src/App/OTA/wm_http_fwup.c`, `HANDLE_BODY`: Scheitert `tls_fwup_img_header_check()`, blieb
+     `nRetCode` auf `HTTP_CLIENT_SUCCESS` stehen, wodurch die abschließende
+     `if(nRetCode) socket_fwup_err(...)`-Bereinigung nie lief und die SDK-eigene fwup-Session
+     (`fwup->busy`) für jeden weiteren Versuch blockiert blieb. `nRetCode` wird jetzt in diesem Fall
+     explizit auf `HTTP_CLIENT_ERROR_BAD_HEADER` gesetzt.
+   - `Src/App/OTA/wm_http_fwup.c`, `SHUTDOWN_LINK`: Ein abbrechender Download sprang unbegrenzt oft
+     zurück nach `PREPARE_PACKET` (Resume per Range-Header) - schaltet der Server während der
+     Übertragung ab, hängt `t_http_fwup()` (und damit der aufrufende Task) für immer. Eine großzügige
+     Obergrenze (`NL_OTA_MAX_RETRY_ROUNDS`, 200 Resume-Runden) lässt `http_fwup()` danach mit
+     `HTTP_CLIENT_ERROR_SOCKET_TIME_OUT` zurückkehren, statt endlos zu warten.
 2. `Src/OS/RTOS/include/FreeRTOSConfig.h`: `configGENERATE_RUN_TIME_STATS 0`, QConnex-Debug-Hooks entfernt.
 3. `undefined reference` beim Linken auf `EarlyInit` und `pre_gpio_config` (von
    `sdk/Platform/Sys/wm_main.c` als `extern`-Hooks erwartet, aber nur von der App bereitgestellt):
@@ -21,10 +43,19 @@ Lib/GNU, Bin/secboot.img, Tools/GNU/link_w600_2m.ld (→ ld/) und wm_tool.c (→
    Ein Task-Deadlock (z.B. ein Task, der `nl_app_lock()` hält und nie freigibt) hätte den Watchdog
    damit nie ausgelöst, weil der Idle-Task munter weiter gefüttert hätte. Der `tls_watchdog_clr()`-
    Aufruf wurde entfernt; `wfi` bleibt. Der einzige verbleibende `tls_watchdog_clr()`-Aufruf im SDK
-   ist in `Platform/Common/fwup/wm_fwup.c` (Firmware-Update-Task) - das ist unkritisch, da er nur
+   ist in `Platform/Common/fwup/wm_fwup.c` (Firmware-Update-Task).
-   während eines laufenden OTA-Updates feuert und den `app_task`-Heartbeat in `src/app_main.c`
+   **Einschätzung aktualisiert (Task 4, Fix-Runde 1):** Als dieser Punkt geschrieben wurde, gab es noch
-   nicht ersetzt (der Watchdog wird sonst ausschließlich dort über den `s_light_beat`-Heartbeat
+   keinen OTA-Pfad - das Fenster war rein theoretisch. Mit Task 4 ist es real und kann durch einen
-   des `light_task` gefüttert). Getestet mit dem Konsolenkommando `wdtest`
+   langsamen oder hängenden Download mehrere Minuten dauern: Solange `wm_fwup.c` seinen eigenen
+   `tls_watchdog_clr()` aufruft, wird der Watchdog gefüttert, **egal** ob `light_task`, `nl_wifi`/`nl_mdns`
+   oder `nl_http_server` in dieser Zeit echt hängen - der App-Task-Heartbeat-Mechanismus in
+   `src/app_main.c` (der genau das eigentlich verhindern soll) greift währenddessen nicht, weil der
+   Watchdog gar nicht bei ihm ankommt. Das ist kein Rückschritt gegenüber vorher (vor Task 4 gab es
+   dieses Fenster schlicht nicht), aber die bisherige Einstufung „unkritisch" war nur so lange richtig,
+   wie der Fall real nie eintrat. Kein Fix in dieser Runde - der Watchdog-Heartbeat müsste dafür wissen,
+   ob der fwup-Task selbst noch vorankommt (z. B. über einen eigenen Beat-Zähler, den `app_task`
+   zusätzlich zu `light`/`net`/`http` prüft); das ist ein eigenständiger Task für einen späteren Plan.
+   Getestet mit dem Konsolenkommando `wdtest`
    (src/sys/nl_console_cmd.c, src/sys/nl_console.c): es hält `nl_app_lock()` für immer, wodurch
    `light_task` und `app_task` blockieren und der Watchdog den Chip zurücksetzen muss.
 5. `Platform/Drivers/internalflash/wm_internal_fls.c`: Semaphor `s_fls2_lock` serialisiert den Flash-Pfad

sdk/Platform/Common/fwup/wm_fwup.c

@@ -172,6 +172,7 @@ static void tls_fwup_img_update_header(T_BOOTER* img_param)
 /* ninlight: SHA256 + tail verification hook instead of the QConnex RSA check (see sdk/PATCHES.md) */
 #include "util/nl_sha256.h"
 #include "sys/nl_fwup_hook.h"
+#include "sys/nl_ota.h" /* nl_ota_set_error() - see sdk/PATCHES.md point 1 */
 
 
 static void fwup_scheduler(void *data)
@@ -440,9 +441,20 @@ static void fwup_scheduler(void *data)
                                 printf("%02x ", bptr[i]);
                             printf("\n");
 #endif
-                            if (org_checksum != image_checksum || !nl_fwup_verify_tail(image_sha256, RSA_SHA256, 512)) // QC_CHANGE_MK
+                            if (org_checksum != image_checksum) // QC_CHANGE_MK
 							{
+								/* ninlight: a checksum failure never reaches nl_fwup_verify_tail() below,
+								 * so report it here explicitly - otherwise last_ota_error would fall back
+								 * to the generic "download" instead of the more precise "crc" (see
+								 * sdk/PATCHES.md point 1). */
 								TLS_DBGPRT_ERR("varify incorrect[0x%02x, but 0x%02x]\n", org_checksum, image_checksum);
+								nl_ota_set_error("crc");
+								request->status = TLS_FWUP_REQ_STATUS_FCRC;
+								fwup->current_state |= TLS_FWUP_STATE_ERROR_CRC;
+								goto request_finish;
+							}
+							else if (!nl_fwup_verify_tail(image_sha256, RSA_SHA256, 512)) // QC_CHANGE_MK
+							{
 								request->status = TLS_FWUP_REQ_STATUS_FCRC;
 								fwup->current_state |= TLS_FWUP_STATE_ERROR_CRC;
 								goto request_finish;
@@ -478,7 +490,14 @@ request_finish:
 					{
 						request->complete(request, request->arg);
 					}
-					if(fwup->updated_len >= (fwup->total_len))
+					/* ninlight: only reset when the image was actually accepted. Before this fix the
+					 * condition below fired unconditionally once the transfer reached its end -
+					 * including a request that got rejected by nl_fwup_verify_tail() (trailer/device/
+					 * hash mismatch) or by the checksum check above (goto request_finish also lands
+					 * here). Resetting in that case would reboot into the OLD firmware and never let
+					 * ota_task() report the failure - so last_ota_error stayed NULL forever. See
+					 * sdk/PATCHES.md point 1. */
+					if(fwup->updated_len >= (fwup->total_len) && !(fwup->current_state & TLS_FWUP_STATE_ERROR))
 					{
 					    fwup_update_autoflag();
 					    tls_sys_reset();

sdk/Src/App/OTA/wm_http_fwup.c

@@ -16,6 +16,12 @@
 
 enum ota_state {PREPARE_PACKET, SETUP_LINK_AND_REQ, RECV_RSP, HANDLE_HEADER, HANDLE_BODY, SHUTDOWN_LINK, QUIT_OTA};
 
+/* ninlight: SHUTDOWN_LINK resumes an unfinished transfer via PREPARE_PACKET forever - a server
+ * that closes the connection early (or never sends the full body) used to hang this task, and
+ * with it the caller of t_http_fwup(), permanently (see sdk/PATCHES.md point 1). Cap the number
+ * of resume rounds generously so a slow-but-healthy download still completes. */
+#define NL_OTA_MAX_RETRY_ROUNDS 200
+
 int http_fwup(HTTPParameters ClientParams)
 {
     char* Buffer = NULL;
@@ -26,6 +32,7 @@ int http_fwup(HTTPParameters ClientParams)
     u32  partLen;
     u32 totalLen = 0;
     u32 recvLen = 0;
+    u32 retryRounds = 0;
     T_BOOTER *booter;
 #if USE_BREAK_POINT_RESUME == 0
     u32 breakFlag = 0;
@@ -124,6 +131,13 @@ int http_fwup(HTTPParameters ClientParams)
                         }
                         else
                         {
+                            /* ninlight: nRetCode is still HTTP_CLIENT_SUCCESS here (the read itself
+                             * worked, only the image header is bad), so the "if(nRetCode)" cleanup
+                             * at the end of this function used to be skipped entirely - leaving
+                             * socket_fwup_err()/tls_fwup_exit() uncalled and the fwup session stuck
+                             * busy for every later attempt. Force the cleanup path (see
+                             * sdk/PATCHES.md point 1). */
+                            nRetCode = HTTP_CLIENT_ERROR_BAD_HEADER;
                             now_state = QUIT_OTA;
                             break;
                         }
@@ -191,7 +205,16 @@ int http_fwup(HTTPParameters ClientParams)
             {
                 if(pHTTP){
                     HTTPClientCloseRequest(&pHTTP);
-                    (recvLen == totalLen)?(now_state = QUIT_OTA):(now_state = PREPARE_PACKET);
+                    if (recvLen == totalLen) {
+                        now_state = QUIT_OTA;
+                    } else if (++retryRounds > NL_OTA_MAX_RETRY_ROUNDS) {
+                        /* ninlight: give up instead of resuming forever, see the comment above
+                         * NL_OTA_MAX_RETRY_ROUNDS. */
+                        nRetCode = HTTP_CLIENT_ERROR_SOCKET_TIME_OUT;
+                        now_state = QUIT_OTA;
+                    } else {
+                        now_state = PREPARE_PACKET;
+                    }
                 }
             }
             break;

src/app_main.c

@@ -165,7 +165,9 @@ static void app_task(void *arg)
     have_last = nl_state_log_load(&last);
     store_unlock();
     if (s_settings.ota_pending) {
-        /* We came back from an update attempt: restore the light the user had on. */
+        /* We came back from an update attempt (accepted or rejected). Just clear the flag so it
+         * doesn't stick in flash forever; nl_boot_state() below restores the light from `last`/
+         * power_on_behaviour as on any other boot, no special-casing needed here. */
         s_settings.ota_pending = 0;
         printf("boot after ota attempt\r\n");
         store_lock();

src/net/nl_api.c

@@ -336,9 +336,13 @@ static void handle_command(const nl_api_ctx_t *ctx, const nl_http_request_t *req
     } else if (strcmp(type, "fw_update") == 0) {
         if (get_string(cmd, "url", 8, 255, &url) != 1 || strncmp(url, "http://", 7) != 0)
             reply_error(resp, 400, "invalid url");
-        else if (ctx->fw_update(url) != 0)
+        else if (ctx->fw_update(url) != 0) {
-            reply_error(resp, 501, "fw_update not available");
+            /* fw_update() itself can genuinely fail now (busy, or a rejected URL) - 409 with the
-        else
+             * reason, not a blanket "not implemented". */
+            nl_sys_info_t in;
+            ctx->get_info(&in);
+            reply_error(resp, 409, in.last_ota_error ? in.last_ota_error : "fw_update busy or invalid url");
+        } else
             reply(resp, 202, "{\"status\":\"updating\"}");
     } else if (strcmp(type, "reboot") == 0) {
         reply(resp, 200, "{\"ok\":true}");

src/sys/nl_ota.c

@@ -34,22 +34,30 @@ void nl_ota_set_error(const char *e) { s_error = e; }
 
 /* Called by the SDK firmware-update path once the image has been written and hashed.
    sha256 covers the whole payload (T_BOOTER header + compressed image), tail is the
-   512 byte end block that used to carry the QConnex RSA signature. */
+   512 byte end block that used to carry the QConnex RSA signature.
+   NB: this runs on the SDK's own fwup_scheduler task (tls_fwup_init(), stack
+   FWUP_TASK_STK_SIZE*4, ~2 KB) - NOT on the 8 KB nl_ota task stack. Keep this hook minimal:
+   one short printf, no extra locals (a version buffer used to sit here and was removed). */
 bool nl_fwup_verify_tail(const uint8_t sha256[32], const uint8_t *tail, uint32_t tail_len)
 {
     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 rejected (%s)\r\n", s_error);
+    printf("ota: trailer %s\r\n", e == NL_TRAILER_OK ? "ok" : s_error);
-        return false;
+    return e == NL_TRAILER_OK;
-    }
+}
-    {
+
-        char v[NL_TRAILER_VERSION_LEN];
+/* Logs the URL without any "user:pass@" userinfo, so credentials embedded in the URL never
-        nl_trailer_version(tail, v);
+   reach the console. s_url is validated by nl_ota_start() to start with "http://". */
-        printf("ota: trailer ok, incoming version %s\r\n", v);
+static void log_url(const char *url)
-    }
+{
-    return true;
+    const char *at = strchr(url + 7, '@');
+
+    if (at != NULL)
+        printf("ota: downloading http://%s\r\n", at + 1);
+    else
+        printf("ota: downloading %s\r\n", url);
 }
 
 static void ota_task(void *arg)
@@ -57,7 +65,7 @@ static void ota_task(void *arg)
     int rc;
 
     (void)arg;
-    printf("ota: downloading %s\r\n", s_url);
+    log_url(s_url);
     rc = t_http_fwup(s_url);
     /* Only reached on failure - the SDK resets the chip itself after a good image. */
     if (s_error == NULL)
@@ -82,7 +90,7 @@ int nl_ota_start(const char *url)
     if (url == NULL || strncmp(url, "http://", 7) != 0)
         return -1;
     n = strlen(url);
-    if (n == 0 || n >= sizeof s_url)
+    if (n <= 7 || n >= sizeof s_url) /* n == 7 is bare "http://" with no host */
         return -1;
     if (s_busy) {
         s_error = "busy";
@@ -105,6 +113,14 @@ int nl_ota_start(const char *url)
 
     if (tls_os_task_create(NULL, "nl_ota", ota_task, NULL, (u8 *)s_stack, sizeof(s_stack),
                            NL_OTA_PRIO, 0) != TLS_OS_SUCCESS) {
+        /* The task never ran, so clear the ota_pending flag we just persisted above - otherwise
+         * the next boot wrongly logs "boot after ota attempt" for an update that never started. */
+        nl_settings_t *s;
+        nl_app_lock();
+        s = nl_app_settings();
+        s->ota_pending = 0;
+        nl_app_unlock();
+        (void)nl_app_save_settings();
         s_busy = false;
         s_error = "busy";
         return -1;

test/test_api.c

@@ -323,8 +323,8 @@ static void test_commands(void)
     TEST_ASSERT_EQUAL_INT(1, fake_factory_calls);
 
     call(NL_HTTP_PUT, "/api/v1/system/command", TOKEN, "{\"command\":{\"type\":\"fw_update\",\"url\":\"http://x/y.img\"}}");
-    TEST_ASSERT_EQUAL_INT(501, resp.status);
+    TEST_ASSERT_EQUAL_INT(409, resp.status);
-    TEST_ASSERT_EQUAL_STRING("fw_update not available", error_text());
+    TEST_ASSERT_EQUAL_STRING("fw_update busy or invalid url", error_text());
     TEST_ASSERT_EQUAL_STRING("http://x/y.img", fake_fw_url);
     fake_fw_result = 0;
     call(NL_HTTP_PUT, "/api/v1/system/command", TOKEN, "{\"command\":{\"type\":\"fw_update\",\"url\":\"http://x/y.img\"}}");