From 9d94f710f7b00d109bf6f6ee57996f43bce5c67f Mon Sep 17 00:00:00 2001 From: Luke Hoersten Date: Fri, 17 Jul 2026 18:56:03 -0500 Subject: main: code-review fixes — brightness units, OTA stall cap, stats + locking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - display: s_last_pwm cached the raw 0-100 percentage at init but display_wake writes it straight to REG_PWM (0-255 duty) — first wake ran visibly dim until a /config brightness change. Shared pct_to_duty helper now converts in both paths. - http_api: cap consecutive OTA recv timeouts (a stalled client spun forever holding s_ota_in_progress, wedging OTA until reboot); cap viewport name at 54 chars so viewport- fits the 63-byte mDNS label; log mdns_service_refresh failures; /state builds JSON from a snapshot instead of holding the state lock across ~25 cJSON allocs; respond_400 delegates to respond_status. - stream_server: bytes_in_window now counts painted frames only (frames discarded while asleep inflated the first post-wake window's MB/s, avg-jpeg and chunk/wire averages; recv_bytes folded in); so_rcvbuf carried into the stats snapshot under the mux; drop dead HEADER_BYTES; merge read_body_instrumented into read_n. - state_machine: transition mutex serializes concurrent wake/sleep (display side-effects ran after the state lock dropped, so racing callers could leave the backlight contradicting st->state); wake-path placeholder paint now takes the decoder lock like the stream path (concurrent esp_lcd_panel_draw_bitmap from two tasks isn't safe). - local_screens: overlay paint takes the decoder lock too; check the overlay timer create; panel dims from viewport_state.h. - jpeg_decoder: try_lock before init returns busy instead of passing a NULL semaphore to xSemaphoreTake. - net_eth: clear cached IP string on link down (stale /state + info). - dead code: display_present_bgr888, TOUCH_FT5426_ADDR, touch s_task, JPEG_DECODER_MAX_OUTPUT_BYTES; doc drift in nvs_config.h / jpeg_decoder.h / app_main flag legend. --- main/http_api.c | 59 +++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 18 deletions(-) (limited to 'main/http_api.c') diff --git a/main/http_api.c b/main/http_api.c index 5e84db6..6954974 100644 --- a/main/http_api.c +++ b/main/http_api.c @@ -33,14 +33,22 @@ static const char *TAG = "http_api"; #define MAX_BODY_BYTES 2048 #define MIN_IDLE_TIMEOUT 5000 +// mDNS hostname is "viewport-" and a DNS label caps at 63 bytes, +// so the name itself must stay <= 63 - strlen("viewport-") = 54 chars. +#define MAX_VIEWPORT_NAME 54 // ============================================================================ // GET /state // ============================================================================ static esp_err_t state_get_handler(httpd_req_t *req) { + // Snapshot under the lock, build JSON unlocked — the stream decode-task + // takes this mutex on every painted frame, so keep the hold time to a + // struct copy rather than ~25 cJSON allocations. viewport_state_lock(); - viewport_state_t *st = viewport_state_get(); + viewport_state_t snap = *viewport_state_get(); + viewport_state_unlock(); + viewport_state_t *st = &snap; uint64_t now_us = (uint64_t)esp_timer_get_time(); uint64_t up_ms = (now_us - st->boot_us) / 1000; @@ -88,8 +96,6 @@ static esp_err_t state_get_handler(httpd_req_t *req) round((double)temp_c * 10.0) / 10.0); } - viewport_state_unlock(); - // Most recent closed window of live-stream stats. Populated every // 30 painted stream frames; zero before the first window rolls. // last_paint_event_us_low is the low 32 bits of the Scrypted @@ -188,12 +194,17 @@ static esp_err_t config_get_handler(httpd_req_t *req) // ============================================================================ // POST /config — partial-update, atomic, validated // ============================================================================ -static esp_err_t respond_400(httpd_req_t *req, const char *reason) +static esp_err_t respond_status(httpd_req_t *req, const char *status, const char *body) { - ESP_LOGW(TAG, "/config 400: %s", reason); - httpd_resp_set_status(req, "400 Bad Request"); + httpd_resp_set_status(req, status); httpd_resp_set_type(req, "text/plain"); - return httpd_resp_send(req, reason, HTTPD_RESP_USE_STRLEN); + return httpd_resp_send(req, body, body ? HTTPD_RESP_USE_STRLEN : 0); +} + +static esp_err_t respond_400(httpd_req_t *req, const char *reason) +{ + ESP_LOGW(TAG, "400: %s", reason); + return respond_status(req, "400 Bad Request", reason); } static esp_err_t read_body(httpd_req_t *req, char *buf, size_t cap) @@ -233,9 +244,10 @@ static esp_err_t config_post_handler(httpd_req_t *req) if ((j = cJSON_GetObjectItemCaseSensitive(root, "viewport"))) { if (!cJSON_IsString(j) || j->valuestring[0] == '\0' || - strlen(j->valuestring) >= sizeof(vp)) { + strlen(j->valuestring) > MAX_VIEWPORT_NAME) { cJSON_Delete(root); - return respond_400(req, "viewport must be a non-empty string < 64 chars"); + return respond_400(req, "viewport must be a non-empty string <= 54 chars" + " (mDNS hostname label limit)"); } strncpy(vp, j->valuestring, sizeof(vp) - 1); have_vp = true; @@ -333,7 +345,11 @@ static esp_err_t config_post_handler(httpd_req_t *req) display_set_brightness(bright); } if (name_or_orient_changed) { - mdns_service_refresh(); + esp_err_t mdns_err = mdns_service_refresh(); + if (mdns_err != ESP_OK) { + ESP_LOGW(TAG, "mdns_service_refresh failed: %s", + esp_err_to_name(mdns_err)); + } } httpd_resp_set_status(req, "204 No Content"); @@ -383,13 +399,6 @@ static esp_err_t state_post_handler(httpd_req_t *req) // ============================================================================ // POST /frame // ============================================================================ -static esp_err_t respond_status(httpd_req_t *req, const char *status, const char *body) -{ - httpd_resp_set_status(req, status); - httpd_resp_set_type(req, "text/plain"); - return httpd_resp_send(req, body, body ? HTTPD_RESP_USE_STRLEN : 0); -} - static esp_err_t frame_post_handler(httpd_req_t *req) { // Content-Type must be image/jpeg. @@ -573,10 +582,24 @@ static esp_err_t firmware_post_handler(httpd_req_t *req) uint8_t buf[4096]; int remaining = req->content_len; int last_logged_pct = -1; + int consecutive_timeouts = 0; while (remaining > 0) { int want = remaining < (int)sizeof(buf) ? remaining : (int)sizeof(buf); int n = httpd_req_recv(req, (char *)buf, want); - if (n == HTTPD_SOCK_ERR_TIMEOUT) continue; + if (n == HTTPD_SOCK_ERR_TIMEOUT) { + // A stalled client would otherwise spin here forever with + // s_ota_in_progress held, wedging OTA until reboot. Each + // timeout is one httpd recv-timeout period, so ~10 in a row + // means the sender is gone. + if (++consecutive_timeouts >= 10) { + err_status = "408 Request Timeout"; + err_msg = "body stalled"; + result = ESP_FAIL; + goto done; + } + continue; + } + consecutive_timeouts = 0; if (n <= 0) { err_status = "400 Bad Request"; err_msg = "body read failed"; -- cgit v1.2.3