39 lines
3.8 KiB
Markdown
39 lines
3.8 KiB
Markdown
# Codebase Audit & Remediation Plan (develop branch)
|
|
|
|
After a thorough audit of the `develop` branch focusing on the recent modular ESP-IDF port, I've identified several critical defects primarily centered around FreeRTOS concurrency, system timers, and legacy feature regressions.
|
|
|
|
Here is the step-by-step remediation plan to address these issues. **No code has been changed yet.**
|
|
|
|
## 1. Fix Critical Step Timing Bug in `app_machine.c`
|
|
**Defect:** `CMD_START_STEP` triggers `start_running(0)`, passing a hardcoded `0` for `now_ms`. The task then calculates `s_deadline_ms = 0 + remaining_ms`. When `app_machine_tick()` runs moments later, it reads the *real* system uptime (e.g., 200,000 ms). If the uptime is larger than the step duration, the step instantly completes.
|
|
**Remediation Steps:**
|
|
- Modify `start_running()` to remove the `now_ms` argument.
|
|
- Instead of calculating `s_deadline_ms` immediately, configure the state machine to defer calculation: set `s_have_deadline = false;` and `s_resume_pending = true;`.
|
|
- This safely delegates the deadline calculation to the next `app_machine_tick()` cycle, which naturally computes it using the true system `now_ms`.
|
|
- Update `app_machine_handle_cmd()` to call `start_running()` without arguments.
|
|
|
|
## 2. Fix Event Queue Concurrency in `app_machine.c`
|
|
**Defect:** The system uses a raw array `s_q` (with `s_q_head`, `s_q_tail`, and `s_q_count++`) to pass events from the machine to the UI. However, `emit()` is called by both the `machine_task` and the `temp_task` (via `app_machine_on_temp`), while `app_machine_last_event()` is read by the `ui_task`. These are non-atomic read-modify-write operations across three threads, which will inevitably corrupt the queue and crash the UI.
|
|
**Remediation Steps:**
|
|
- Replace the raw ring buffer variables (`s_q`, `s_q_head`, `s_q_tail`, `s_q_count`) with a standard FreeRTOS `QueueHandle_t s_evtq`.
|
|
- In `app_machine_init()`, initialize the queue: `s_evtq = xQueueCreate(UI_EVT_QUEUE_LEN, sizeof(ui_evt_t));`.
|
|
- Update `emit()` to use `xQueueSend(s_evtq, &ev, 0);`.
|
|
- Update `app_machine_last_event()` to use `xQueueReceive(s_evtq, out, 0) == pdTRUE`.
|
|
|
|
## 3. Restore Auto-Advance Functionality in `app_machine.c`
|
|
**Defect:** The legacy Arduino loop automatically chained processing steps together (`run == 1`). The new state machine includes a `maybe_auto_advance()` function guarded by `s_auto_advance`, but `s_auto_advance` is permanently hardcoded to `false` and never toggled. As a result, the machine halts after every single step.
|
|
**Remediation Steps:**
|
|
- Initialize `s_auto_advance = true` to match the legacy behavior of chaining steps automatically.
|
|
- (Optional) Wire up a UI command (`CMD_TOGGLE_AUTO_ADVANCE`) to allow users to turn this off if manual pausing between steps is desired.
|
|
|
|
## 4. Fix Task Watchdog Initialization in `main.c`
|
|
**Defect:** `ui_task`, `machine_task`, `input_task`, and `temp_task` all invoke `esp_task_wdt_add(NULL)`. However, the Task Watchdog Timer (TWDT) is never initialized in `app_main()`. Depending on the ESP-IDF version and `sdkconfig` defaults, this can cause silent failures or panic at boot.
|
|
**Remediation Steps:**
|
|
- In `app_main()`, invoke `esp_task_wdt_init(&wdt_config)` *before* creating the FreeRTOS tasks.
|
|
- Ensure the WDT timeout is set generously (e.g., 3-5 seconds) to accommodate the 1.5-second blocking time in `hal_temp_tick()`.
|
|
|
|
## 5. Clean up `hal_temp_tick()` Timing (Low Priority)
|
|
**Defect:** `hal_temp_tick()` blocks `temp_task` with multiple `vTaskDelay(pdMS_TO_TICKS(750))` calls for the DS18B20 conversion. While safely isolated from the UI, it forces the watchdog timeout to be artificially large and limits responsiveness if multiple sensors were ever added.
|
|
**Remediation Steps:**
|
|
- Refactor the 1-Wire sequence into a non-blocking state machine within `temp_task`, or simply keep it as-is but acknowledge the WDT requirement constraint outlined in step 4.
|