From f675d168701f32d8fa8b2dd233b4a610367e9026 Mon Sep 17 00:00:00 2001 From: Roman Fordinal Date: Sun, 23 Aug 2026 14:58:25 +0200 Subject: [PATCH 1/6] feat: real binary semaphore and a vTaskDelay that sleeps Two FreeRTOS primitives the shim did not emulate. xSemaphoreCreateBinary did not exist, and xSemaphoreTake ignored ticksToWait, so firmware that waits on a confirm semaphore with a deadline could never see the deadline expire. SemaphoreHandle_t now points at a tagged base; xSemaphoreTake, xSemaphoreGive and xQueuePeek dispatch on the tag. The binary semaphore is a mutex plus a condvar plus one token: it honours ticksToWait, reports pdFALSE on timeout, and takes a give from any thread. ticksToWait == 0 is a non-blocking poll. The mutex arm is untouched. SimMutex keeps the same recursive_mutex, holder and holdCount that xSemaphoreGetMutexHolder and xQueuePeek read. A mutex-only host program compiled against the old and new headers prints identical output. vTaskDelay was an empty body, so every delay took zero time. It now sleeps, converting ticks through portTICK_PERIOD_MS rather than assuming a tick is a millisecond. A zero delay yields, as FreeRTOS does. xSemaphoreCreateCounting is deliberately absent: nothing asks for it. --- src/freertos/semphr.h | 122 +++++++++++++++++++++++++++++++++++++----- src/freertos/task.h | 15 +++++- 2 files changed, 122 insertions(+), 15 deletions(-) diff --git a/src/freertos/semphr.h b/src/freertos/semphr.h index cc32125..bd03b35 100644 --- a/src/freertos/semphr.h +++ b/src/freertos/semphr.h @@ -1,54 +1,148 @@ #pragma once +#include +#include +#include #include #include "FreeRTOS.h" #include "task.h" +// FreeRTOS hands out mutexes and binary semaphores through the same +// SemaphoreHandle_t. They are different objects with different rules, so the +// handle carries a tag and the shared entry points below dispatch on it. +enum class SimSemaphoreKind { Mutex, Binary }; + +struct SimSemaphoreBase { + const SimSemaphoreKind kind; + explicit SimSemaphoreBase(SimSemaphoreKind k) : kind(k) {} +}; + // Use a real recursive mutex for the rendering semaphore. -struct SimMutex { +struct SimMutex : SimSemaphoreBase { + SimMutex() : SimSemaphoreBase(SimSemaphoreKind::Mutex) {} std::recursive_mutex mtx; // Track "holder" for xSemaphoreGetMutexHolder compatibility (not thread-safe, // good enough for simulator) TaskHandle_t holder = nullptr; uint16_t holdCount = 0; }; -typedef SimMutex *SemaphoreHandle_t; + +// A real binary semaphore: one token, created empty. A take with no token +// waits, and reports failure when ticksToWait runs out. A give may come from +// any thread, including one that never took it. +struct SimBinarySemaphore : SimSemaphoreBase { + SimBinarySemaphore() : SimSemaphoreBase(SimSemaphoreKind::Binary) {} + std::mutex mtx; + std::condition_variable cv; + bool available = false; +}; + +typedef SimSemaphoreBase *SemaphoreHandle_t; + +namespace sim_semaphore_detail { + +// Single non-virtual base, so a tag-checked downcast is exact. Nothing frees a +// handle in this shim (there is no vSemaphoreDelete), so no base-pointer delete +// happens and the base needs no virtual destructor. +inline SimMutex *asMutex(SemaphoreHandle_t sem) { + return static_cast(sem); +} +inline SimBinarySemaphore *asBinary(SemaphoreHandle_t sem) { + return static_cast(sem); +} + +// FreeRTOS timeouts are in ticks. portTICK_PERIOD_MS (FreeRTOS.h) is the one +// tick-rate definition this shim has, so convert through it rather than +// assuming a tick is a millisecond. +inline std::chrono::milliseconds ticksToDuration(uint32_t ticks) { + return std::chrono::milliseconds(static_cast(ticks) * + portTICK_PERIOD_MS); +} + +} // namespace sim_semaphore_detail inline SemaphoreHandle_t xSemaphoreCreateMutex() { return new SimMutex(); } -inline bool xSemaphoreTake(SemaphoreHandle_t sem, uint32_t /*ticksToWait*/) { +inline SemaphoreHandle_t xSemaphoreCreateBinary() { + return new SimBinarySemaphore(); +} + +// Returns pdTRUE (true) on success, pdFALSE (false) when ticksToWait expired. +// Only the binary semaphore can fail: a mutex take waits as long as it must, +// which is what every caller of the recursive mutex already assumes. +inline bool xSemaphoreTake(SemaphoreHandle_t sem, uint32_t ticksToWait) { if (!sem) return true; - sem->mtx.lock(); - sem->holder = xTaskGetCurrentTaskHandle(); - sem->holdCount++; + if (sem->kind == SimSemaphoreKind::Binary) { + auto *bin = sim_semaphore_detail::asBinary(sem); + std::unique_lock lk(bin->mtx); + const auto hasToken = [bin] { return bin->available; }; + if (ticksToWait == portMAX_DELAY) { + bin->cv.wait(lk, hasToken); + } else if (!bin->cv.wait_for( + lk, sim_semaphore_detail::ticksToDuration(ticksToWait), + hasToken)) { + // ticksToWait == 0 lands here too: wait_for tests the predicate once and + // returns, which is the non-blocking "drain a stale give" poll. + return false; + } + bin->available = false; + return true; + } + auto *mtx = sim_semaphore_detail::asMutex(sem); + mtx->mtx.lock(); + mtx->holder = xTaskGetCurrentTaskHandle(); + mtx->holdCount++; return true; } inline bool xSemaphoreGive(SemaphoreHandle_t sem) { if (!sem) return true; - if (sem->holdCount > 0) { - sem->holdCount--; + if (sem->kind == SimSemaphoreKind::Binary) { + auto *bin = sim_semaphore_detail::asBinary(sem); + bool wasEmpty; + { + std::lock_guard lk(bin->mtx); + wasEmpty = !bin->available; + bin->available = true; + } + // Notify outside the lock: the woken thread would only block on it again. + bin->cv.notify_one(); + // FreeRTOS reports pdFALSE for a give that overflows a full semaphore. + return wasEmpty; } - if (sem->holdCount == 0) { - sem->holder = nullptr; + auto *mtx = sim_semaphore_detail::asMutex(sem); + if (mtx->holdCount > 0) { + mtx->holdCount--; } - sem->mtx.unlock(); + if (mtx->holdCount == 0) { + mtx->holder = nullptr; + } + mtx->mtx.unlock(); return true; } inline TaskHandle_t xSemaphoreGetMutexHolder(SemaphoreHandle_t sem) { - return sem ? sem->holder : nullptr; + if (!sem || sem->kind != SimSemaphoreKind::Mutex) + return nullptr; + return sim_semaphore_detail::asMutex(sem)->holder; } // xQueuePeek on a mutex: returns pdTRUE if the mutex is available (not taken). +// On a binary semaphore, "available" means a token is waiting to be taken. inline int xQueuePeek(SemaphoreHandle_t sem, void *, uint32_t) { if (!sem) return pdTRUE; - bool locked = sem->mtx.try_lock(); + if (sem->kind == SimSemaphoreKind::Binary) { + auto *bin = sim_semaphore_detail::asBinary(sem); + std::lock_guard lk(bin->mtx); + return bin->available ? pdTRUE : pdFALSE; + } + auto *mtx = sim_semaphore_detail::asMutex(sem); + bool locked = mtx->mtx.try_lock(); if (locked) { - sem->mtx.unlock(); + mtx->mtx.unlock(); return pdTRUE; } return pdFALSE; diff --git a/src/freertos/task.h b/src/freertos/task.h index a0f24a2..8f98e04 100644 --- a/src/freertos/task.h +++ b/src/freertos/task.h @@ -1,4 +1,5 @@ #pragma once +#include #include #include "FreeRTOS.h" @@ -84,4 +85,16 @@ inline void vTaskDelete(TaskHandle_t h) { } inline unsigned int uxTaskGetStackHighWaterMark(TaskHandle_t) { return 2048; } inline void vTaskList(char *) {} -inline void vTaskDelay(int) {} +// vTaskDelay's argument is in FreeRTOS ticks, and portTICK_PERIOD_MS +// (FreeRTOS.h) is this shim's one tick-rate definition. Convert through it, do +// not assume one tick is one millisecond. A no-op here made every firmware +// retry loop and every yield-every-N-rows call run in zero time. +inline void vTaskDelay(int ticks) { + if (ticks <= 0) { + // FreeRTOS treats a zero delay as a yield, not a sleep. + std::this_thread::yield(); + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds( + static_cast(ticks) * portTICK_PERIOD_MS)); +} From ca76ac12f9cdd1e4d341dcb2075016d2fb79ccf4 Mon Sep 17 00:00:00 2001 From: Roman Fordinal Date: Sun, 23 Aug 2026 14:58:33 +0200 Subject: [PATCH 2/6] docs: document the FreeRTOS shim's two emulation gaps What the binary-semaphore and vTaskDelay gaps were, what replaced them, the tick-rate basis (portTICK_PERIOD_MS is the shim's only definition), and the before/after regression evidence for making vTaskDelay sleep. Marks what was verified by running versus read off the code, including the one cost the code alone cannot bound: the JPEG decoder's yield every four file IO operations. --- docs/freertos-shim.md | 169 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 docs/freertos-shim.md diff --git a/docs/freertos-shim.md b/docs/freertos-shim.md new file mode 100644 index 0000000..8d4c437 --- /dev/null +++ b/docs/freertos-shim.md @@ -0,0 +1,169 @@ +# The FreeRTOS shim + +The simulator replaces the FreeRTOS API with host shims: `src/freertos/FreeRTOS.h`, +`src/freertos/task.h`, `src/freertos/semphr.h`. A task is a `std::thread` +(`src/freertos/task.h:26`), a mutex is a `std::recursive_mutex` +(`src/freertos/semphr.h:21`), a task notification is a condvar +(`src/freertos/FreeRTOS.h:34`). + +Two primitives were missing rather than simplified. This doc says what they +were, what replaced them, and what the change cost the existing timing. + +## Gap 1: no binary semaphore, and no timeout + +Read off the code, before this change: + +- `xSemaphoreCreateBinary()` did not exist. Only `xSemaphoreCreateMutex()` did. +- `xSemaphoreTake` ignored `ticksToWait` entirely (the parameter was unnamed and + unused). It always blocked until it got the mutex, then returned `true`. + +So firmware that creates a confirm semaphore and waits on it with a deadline +could not time out. The concrete case in the firmware this simulator builds: +`lib/BlePositionServer/src/BlePositionServer.cpp:261` creates it with +`xSemaphoreCreateBinary()`, `:621` polls it with `ticksToWait == 0` to drain a +stale give, `:629` waits 3000 ms and treats anything but `pdTRUE` as a dropped +reply, and `:75` gives it from the BLE callback thread. Under the old shim +`:261` did not compile, `:621` would have taken the mutex, and `:629` could +never have returned `pdFALSE`. + +## Gap 2: `vTaskDelay` was a no-op + +`inline void vTaskDelay(int) {}` -- an empty body, so every delay took zero +time. A firmware retry loop of 40 iterations x 25 ms ran instantly, and every +`yield every N rows` helper yielded nothing. + +## What was added + +**One handle type, two objects, tag dispatch.** `SemaphoreHandle_t` is now +`SimSemaphoreBase *` (`src/freertos/semphr.h:40`). `SimSemaphoreBase` carries a +`SimSemaphoreKind` tag; `SimMutex` and `SimBinarySemaphore` derive from it. The +mutex was not retyped and its internals were not touched: `SimMutex` still holds +the same `std::recursive_mutex`, `holder` and `holdCount` +(`src/freertos/semphr.h:21-30`), because `xSemaphoreGetMutexHolder` and +`xQueuePeek` read them. + +`xSemaphoreTake`, `xSemaphoreGive` and `xQueuePeek` are shared entry points, so +each one branches on the tag and leaves the mutex arm exactly as it was. + +**The binary semaphore** (`src/freertos/semphr.h:33-38`) is a `std::mutex` plus +a `std::condition_variable` plus one `bool available`, created empty: + +- `xSemaphoreTake(sem, portMAX_DELAY)` waits forever. +- `xSemaphoreTake(sem, n)` waits at most `n` ticks and returns `false` + (`pdFALSE`) if the token never arrived. +- `xSemaphoreTake(sem, 0)` tests once and returns. `wait_for` with a zero + duration evaluates the predicate and gives up, which is the non-blocking poll + the drain-a-stale-give call needs. +- A successful take clears the token, so the next take blocks again. +- `xSemaphoreGive` takes the semaphore's own lock, sets the token, drops the + lock, then notifies. It requires nothing of the calling thread, so a BLE + callback thread can give a semaphore the activity thread is waiting on. +- `xSemaphoreGive` on an already-full semaphore returns `false`, which is what + FreeRTOS does (`errQUEUE_FULL`). +- `xQueuePeek` on a binary semaphore reports whether a token is waiting. + `xSemaphoreGetMutexHolder` on one returns `nullptr`. + +`xSemaphoreCreateCounting` was **not** added. Nothing in the build asks for it. + +**Return type kept as `bool`.** Real FreeRTOS returns `BaseType_t`. The shim +already returned `bool` and every call site either ignores the result or +compares it against `pdTRUE` / `pdFALSE`, where `false == pdFALSE == 0` and +`true == pdTRUE == 1`. Leaving the signature alone keeps the mutex arm's diff to +zero. + +**No `vSemaphoreDelete`.** There was none before and there is none now, so +nothing ever deletes through the base pointer and the base needs no virtual +destructor (`src/freertos/semphr.h:44-46`). + +**`vTaskDelay` now sleeps** (`src/freertos/task.h:92`). A positive tick count +becomes a `std::this_thread::sleep_for`. Zero or negative yields instead of +sleeping, which is what FreeRTOS does with a zero delay. + +## Tick rate + +One tick is one millisecond. Basis: `portTICK_PERIOD_MS` is defined as `1` and +is the only tick-rate definition in the shim (`src/freertos/FreeRTOS.h:10`). +There is no `configTICK_RATE_HZ` and no `pdMS_TO_TICKS` here. Both new +conversions multiply by `portTICK_PERIOD_MS` rather than hardcoding 1 +(`src/freertos/semphr.h:58-59`, `src/freertos/task.h:98-99`), so redefining +that macro moves both. + +This matches the firmware's own target config, which sets +`CONFIG_FREERTOS_HZ=1000`, so a tick is 1 ms on device too. + +Measured on this host: `vTaskDelay(1)` costs 1.057 ms per call over 1000 calls, +and `vTaskDelay(0)` costs 0.00066 ms. So the sleep overshoots its nominal +duration by about 6 percent. Verified by running. + +## Regression: what making `vTaskDelay` real cost + +A no-op `vTaskDelay` is load-bearing until proven otherwise, so the same gate +ran before and after. + +Gate: build the firmware's `simulator` env, then run a scripted session that +boots, enters the map on a persisted fix, and screenshots it. + +``` +pio run -e simulator +CROSSPOINT_SIM_INPUT_SCRIPT='1200:ENTER;12000:QUIT' \ +CROSSPOINT_SIM_SCREENSHOTS='6000:./qa-artifacts/map.bmp' \ + /program +``` + +| | before | after | +|---|---|---| +| build | SUCCESS | SUCCESS | +| unique compiler warnings | 4 | 4, same 4 | +| map render | 4 tiles, 2874 ways, 18 places, 22 ms | 4 tiles, 2874 ways, 18 places, 20 ms | +| screenshot | 480x800, 2 grey levels, 11.88 percent dark | byte-identical to before | +| process wall clock | 12.12 s | 12.12 s | + +Verified by running, 2026-08-23. The screenshot draws tiles, ways, place labels, +scale bar and compass in both runs, and the two BMPs compare byte for byte +equal. No gate behind an env var was needed; the real sleep is unconditional. + +**What the gate does not cover.** No `vTaskDelay` call site is on the boot -> +home -> map path, so the gate proves the change is harmless there, not +everywhere. The remaining call sites in the firmware, and their cost at +1.057 ms per tick, read off the code: + +- `lib/Xtc/Xtc.cpp:20` -- one tick every 8 thumbnail rows. A 480-row thumbnail + pays about 63 ms. +- `lib/PngToBmpConverter/PngToBmpConverter.cpp:80` -- one tick every 8 decoded + rows, same shape. +- `src/activities/reader/TxtReaderActivity.cpp:162` -- one tick per 20 indexed + pages. A 2000-page book pays about 106 ms. +- `lib/JpegToBmpConverter/JpegToBmpConverter.cpp:176` -- one tick every 4 file + IO operations. The IO count is not visible from the call site, so this is the + one site whose cost is **open**; a JPEG-heavy screen is what would show it. +- `src/activities/map/MapTransferReceiver.cpp:159` -- a poll loop with its own + millisecond deadline. It used to spin a core flat out; now it sleeps. Strictly + better. +- `src/activities/reader/KOReaderSyncActivity.cpp:56` -- 100 ticks, so about + 106 ms where it used to be 0. + +## Standalone checks + +The gate cannot reach the binary semaphore: the firmware gates that code behind +a BLE capability flag the simulator build does not set, so the file compiles to +stubs. It was verified with a host program compiled straight against these +headers instead. Verified by running, 2026-08-23: + +- an empty binary semaphore, `ticksToWait == 0`, returns `pdFALSE` in under + 20 ms +- `ticksToWait == 300` returns `pdFALSE` after 300 ms +- a give from a second thread wakes a blocked 3000-tick take after 150 ms +- the token is consumed: the next zero-tick take fails again +- a second give on a full semaphore returns `pdFALSE` +- `vTaskDelay(200)` sleeps 200 ms, `vTaskDelay(0)` returns immediately + +Mutex parity was checked the same way: one program exercising only the mutex API +(create, peek free, take, holder, recursive re-take, peek from another thread +while held, give, give again, holder cleared, null handle for all four entry +points) compiled against the old headers and the new ones and printed identical +output. + +Note one pre-existing quirk that parity run confirms is unchanged: `xQueuePeek` +on a mutex uses `try_lock`, and a `std::recursive_mutex` grants `try_lock` to +its own holder. So peek from the holding thread reports the mutex as free. Only +another thread sees it as taken. From 6268152600fda55f5418fec8585a36b559e3737b Mon Sep 17 00:00:00 2001 From: Roman Fordinal Date: Sun, 23 Aug 2026 15:06:03 +0200 Subject: [PATCH 3/6] feat: add pdMS_TO_TICKS to the FreeRTOS shim The shim had no pdMS_TO_TICKS, so firmware that expresses its timeouts in milliseconds could not compile against it at all -- independent of what the semaphore or the delay then did with the result. It goes in FreeRTOS.h because that is where real FreeRTOS reaches it from: projdefs.h defines it and FreeRTOS.h includes that. The expression is FreeRTOS's own, rewritten through the one tick-rate macro this shim has. FreeRTOS computes (ms * configTICK_RATE_HZ) / 1000 with integer division; the port defines portTICK_PERIOD_MS as 1000 / configTICK_RATE_HZ, so substituting reduces it exactly to a divide by portTICK_PERIOD_MS. Same round-down, and a divide cannot overflow the way the multiply can. At portTICK_PERIOD_MS == 1 it is the identity. All three tick conversions now go through that one macro, so redefining it moves the delay, the semaphore timeout and this together. No other absent FreeRTOS symbol was added. The recursive-mutex trio is reachable only from a firmware library the simulator env ignores, and the two remaining names appear in firmware comments, never in a call. --- src/freertos/FreeRTOS.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/freertos/FreeRTOS.h b/src/freertos/FreeRTOS.h index 84ca905..6613d6c 100644 --- a/src/freertos/FreeRTOS.h +++ b/src/freertos/FreeRTOS.h @@ -1,5 +1,6 @@ #pragma once #include +#include #include #include @@ -9,6 +10,18 @@ #define eIncrement 1 #define portTICK_PERIOD_MS 1 +// Real FreeRTOS defines this in projdefs.h, reached through FreeRTOS.h, as +// ( ( TickType_t ) ( xTimeInMs ) * configTICK_RATE_HZ ) / 1000 +// with integer division, so it rounds down and a sub-tick delay becomes 0 +// ticks. There is no configTICK_RATE_HZ here, and portTICK_PERIOD_MS above is +// the shim's one tick-rate definition. The port defines +// portTICK_PERIOD_MS == 1000 / configTICK_RATE_HZ, so substituting it reduces +// that expression exactly to a divide by it -- same rounding, and no +// multiply to overflow. At portTICK_PERIOD_MS == 1 it is the identity: +// pdMS_TO_TICKS(n) == n. +#define pdMS_TO_TICKS(xTimeInMs) \ + ((uint32_t)((uint32_t)(xTimeInMs) / (uint32_t)portTICK_PERIOD_MS)) + using BaseType_t = int; // ESP-IDF's portMUX_TYPE is a spinlock used with taskENTER_CRITICAL / From 234b58f17f9ffc26b1d2ba853f08db2edb53e713 Mon Sep 17 00:00:00 2001 From: Roman Fordinal Date: Sun, 23 Aug 2026 15:06:03 +0200 Subject: [PATCH 4/6] docs: record the pdMS_TO_TICKS gap and correct the shim's summary CLAUDE.md said the shim maps SemaphoreHandle_t to a recursive mutex. That stopped being true when the binary semaphore landed, so the clause now names both arms. The topic doc gains the third gap, the reduction that produced the macro with its ESP-IDF citations, the scope where that reduction is exact (checked over 140 tick-rate and millisecond pairs), the assertion output including the firmware's four call sites verbatim, and the four absent symbols found in the same sweep that were deliberately left out. --- CLAUDE.md | 2 +- docs/freertos-shim.md | 90 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1e3faa6..6146783 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ The simulator is a collection of host-side reimplementations of the firmware's h - **Orientation rotation lives in two places.** The firmware's renderer rotates content into the landscape framebuffer (90 CCW for `Portrait`). The simulator undoes that with `SDL_RenderCopyEx`. If you change one, change the other. The dst rect is landscape-shaped and centre-offset because `SDL_RenderCopyEx` rotates around the dst centre. - **HiDPI / dithering.** Set `SDL_HINT_RENDER_SCALE_QUALITY=1` *before* `SDL_CreateTexture`, plus `SDL_WINDOW_ALLOW_HIGHDPI` and `SDL_RenderSetLogicalSize`. Without all three, Bayer-dithered grays render as harsh black/white stripes on Retina. - **POSIX fds, not std::fstream, in [src/HalStorage.cpp](src/HalStorage.cpp).** This was a deliberate rewrite. fstream's separate get/put pointers, eofbit-blocks-seek behaviour, and write-only seek restrictions caused several silent-corruption bugs. Do not reintroduce fstream here. All paths are prefixed with `./fs_` so the simulated filesystem stays sandboxed under the binary's working directory; `/books/` on the SD card maps to `./fs_/books/`. Directory iteration skips only the special `.` and `..` entries; firmware applies its own hidden-file policy. -- **FreeRTOS shim.** [src/freertos/](src/freertos/) maps `xTaskCreate` to `std::thread`, task notifies to a condvar + counter, and `SemaphoreHandle_t` to `std::recursive_mutex`. A `thread_local SimTaskHandle*` lets each task thread find its own handle. +- **FreeRTOS shim.** [src/freertos/](src/freertos/) maps `xTaskCreate` to `std::thread`, task notifies to a condvar + counter, and `SemaphoreHandle_t` to a tagged handle with two arms, a `std::recursive_mutex` for `xSemaphoreCreateMutex` and a real binary semaphore for `xSemaphoreCreateBinary`. A `thread_local SimTaskHandle*` lets each task thread find its own handle. - **`_exit(0)` not `return 0`.** [src/simulator_main.cpp](src/simulator_main.cpp) ends with `_exit(0)` after `SDL_Quit()` to skip C++ global destructors. The render task is `[[noreturn]]`, so running destructors while it is mid-render races and produces a "quit unexpectedly" dialog. Keep this. - **Time uses `steady_clock`.** `millis()` / `micros()` in [src/Arduino.h](src/Arduino.h) deliberately use `steady_clock`, not `system_clock`, so wall-clock changes do not perturb timing. diff --git a/docs/freertos-shim.md b/docs/freertos-shim.md index 8d4c437..c3f8aae 100644 --- a/docs/freertos-shim.md +++ b/docs/freertos-shim.md @@ -4,7 +4,7 @@ The simulator replaces the FreeRTOS API with host shims: `src/freertos/FreeRTOS. `src/freertos/task.h`, `src/freertos/semphr.h`. A task is a `std::thread` (`src/freertos/task.h:26`), a mutex is a `std::recursive_mutex` (`src/freertos/semphr.h:21`), a task notification is a condvar -(`src/freertos/FreeRTOS.h:34`). +(`src/freertos/FreeRTOS.h:47`). Two primitives were missing rather than simplified. This doc says what they were, what replaced them, and what the change cost the existing timing. @@ -32,6 +32,18 @@ never have returned `pdFALSE`. time. A firmware retry loop of 40 iterations x 25 ms ran instantly, and every `yield every N rows` helper yielded nothing. +## Gap 3: no `pdMS_TO_TICKS` + +Also absent, and not merely unused: the firmware passes every one of its +millisecond timeouts through it. Four call sites, all in +`lib/BlePositionServer/src/BlePositionServer.cpp` -- `:249` +`vTaskDelay(pdMS_TO_TICKS(20))`, `:404` `vTaskDelay(pdMS_TO_TICKS(50))`, `:629` +`xSemaphoreTake(sem, pdMS_TO_TICKS(kConfirmTimeoutMs))` with +`kConfirmTimeoutMs = 3000` (`:612`), and `:653` +`vTaskDelay(pdMS_TO_TICKS(kRetryDelayMs))` with `kRetryDelayMs = 25` (`:608`). +Without the macro that file does not compile against the shim at all, whatever +the semaphore does. + ## What was added **One handle type, two objects, tag dispatch.** `SemaphoreHandle_t` is now @@ -79,14 +91,56 @@ destructor (`src/freertos/semphr.h:44-46`). becomes a `std::this_thread::sleep_for`. Zero or negative yields instead of sleeping, which is what FreeRTOS does with a zero delay. +**`pdMS_TO_TICKS`** (`src/freertos/FreeRTOS.h:13-23`) lives in `FreeRTOS.h` +because that is where real FreeRTOS reaches it from -- it is defined in +`projdefs.h`, which `FreeRTOS.h` includes. The shim's expression is + +``` +#define pdMS_TO_TICKS(xTimeInMs) \ + ((uint32_t)((uint32_t)(xTimeInMs) / (uint32_t)portTICK_PERIOD_MS)) +``` + +which is FreeRTOS's own expression rewritten through the one macro this shim +has, not an invented one. In the pinned ESP-IDF (5.5.2.260206, on disk): + +- `components/freertos/FreeRTOS-Kernel/include/freertos/projdefs.h:46` defines + it as `((TickType_t)(xTimeInMs) * configTICK_RATE_HZ) / 1000U`, integer + division, so it rounds **down** and a sub-tick delay yields 0 ticks. +- `components/freertos/FreeRTOS-Kernel/portable/riscv/include/freertos/portmacro.h:126` + defines `portTICK_PERIOD_MS` as `1000 / configTICK_RATE_HZ`. + +Substituting the second into the first gives `xTimeInMs / portTICK_PERIOD_MS`. +Same rounding, and strictly safer: FreeRTOS multiplies before dividing and can +overflow a 32-bit tick type, a divide cannot. At `portTICK_PERIOD_MS == 1` it is +the identity, so `pdMS_TO_TICKS(n) == n`. + +The reduction is exact whenever `configTICK_RATE_HZ` divides 1000 evenly, which +every rate that yields a whole-millisecond `portTICK_PERIOD_MS` does. Checked by +running both expressions against each other over 10 tick rates x 14 millisecond +values: 140 pairs, 0 mismatches. A rate that does not divide 1000 evenly does +differ (at 333 Hz, `pdMS_TO_TICKS(3)` is 0 in FreeRTOS and 1 here), but such a +rate cannot be expressed by `portTICK_PERIOD_MS` in the first place, so the shim +cannot reach that case. + +**Nothing else was added.** Sweeping every FreeRTOS symbol the firmware +references against the shim turned up four more absentees, and none of them is a +gap the build has: + +- `xSemaphoreCreateRecursiveMutex`, `xSemaphoreTakeRecursive`, + `xSemaphoreGiveRecursive` -- used only in the firmware's `lib/hal/HalStorage.cpp`, + and `hal` is in the simulator env's `lib_ignore`. The simulator ships its own + `src/HalStorage.cpp`, which uses none of them. +- `vPortEnterCritical`, `xTaskPriorityDisinherit` -- appear in firmware comments + only, never called. + ## Tick rate One tick is one millisecond. Basis: `portTICK_PERIOD_MS` is defined as `1` and -is the only tick-rate definition in the shim (`src/freertos/FreeRTOS.h:10`). -There is no `configTICK_RATE_HZ` and no `pdMS_TO_TICKS` here. Both new -conversions multiply by `portTICK_PERIOD_MS` rather than hardcoding 1 -(`src/freertos/semphr.h:58-59`, `src/freertos/task.h:98-99`), so redefining -that macro moves both. +is the only tick-rate definition in the shim (`src/freertos/FreeRTOS.h:11`). +There is no `configTICK_RATE_HZ` here. All three conversions go through +`portTICK_PERIOD_MS` rather than hardcoding 1 (`src/freertos/FreeRTOS.h:22-23`, +`src/freertos/semphr.h:58-59`, `src/freertos/task.h:98-99`), so redefining that +macro moves all three. This matches the firmware's own target config, which sets `CONFIG_FREERTOS_HZ=1000`, so a tick is 1 ms on device too. @@ -157,6 +211,30 @@ headers instead. Verified by running, 2026-08-23: - a second give on a full semaphore returns `pdFALSE` - `vTaskDelay(200)` sleeps 200 ms, `vTaskDelay(0)` returns immediately +`pdMS_TO_TICKS` was checked the same way, including the firmware's four call +sites verbatim. All 13 assertions passed, and the two `static_assert`s compiled, +which is what proves the macro is usable in a constant expression like the real +one: + +``` +pdMS_TO_TICKS(0) = 0 want 0 ok +pdMS_TO_TICKS(1) = 1 want 1 ok +pdMS_TO_TICKS(25) = 25 want 25 ok +pdMS_TO_TICKS(3000) = 3000 want 3000 ok +pdMS_TO_TICKS(4294967295u) = 4294967295 want 4294967295 ok (no overflow) +pdMS_TO_TICKS(10 + 15) = 25 want 25 ok (argument parenthesised) +pdMS_TO_TICKS(3000) / 2 = 1500 want 1500 ok (result parenthesised) +pdMS_TO_TICKS(20) = 20 want 20 ok (BlePositionServer.cpp:249) +pdMS_TO_TICKS(50) = 50 want 50 ok (BlePositionServer.cpp:404) +pdMS_TO_TICKS(kConfirmTimeoutMs) = 3000 want 3000 ok (BlePositionServer.cpp:629) +pdMS_TO_TICKS(kRetryDelayMs) = 25 want 25 ok (BlePositionServer.cpp:653) +xSemaphoreTake(sem, pdMS_TO_TICKS(3000)) != pdTRUE -> 1 after 3000 ms ok +vTaskDelay(pdMS_TO_TICKS(25)) slept 25 ms ok +``` + +The last two run the call sites' whole statements, not just the macro, so the +3000 ms confirm timeout that could never expire before now expires in 3000 ms. + Mutex parity was checked the same way: one program exercising only the mutex API (create, peek free, take, holder, recursive re-take, peek from another thread while held, give, give again, holder cleared, null handle for all four entry From 1c153cc796e7139806471f9525bf3c330eb917f7 Mon Sep 17 00:00:00 2001 From: Roman Fordinal Date: Sun, 23 Aug 2026 16:54:41 +0200 Subject: [PATCH 5/6] docs: the confirm timeout has since run inside real firmware The binary semaphore and the real vTaskDelay were verified by a standalone host program because the firmware gated that code out. It no longer does: 23 consecutive 3000 ms give-up waits were measured through real firmware on two independent clocks, which is the path the old no-op shim made unreachable. Also record the shim-only compile failure this work found: a firmware file can use portMUX_TYPE, portENTER_CRITICAL, vTaskDelay, pdMS_TO_TICKS and the semaphore API without including any FreeRTOS header, because another library pulls Arduino.h in for it. Every error is then a FreeRTOS name, which reads as a shim gap and is not. --- docs/freertos-shim.md | 54 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/docs/freertos-shim.md b/docs/freertos-shim.md index c3f8aae..3dbd859 100644 --- a/docs/freertos-shim.md +++ b/docs/freertos-shim.md @@ -198,10 +198,11 @@ everywhere. The remaining call sites in the firmware, and their cost at ## Standalone checks -The gate cannot reach the binary semaphore: the firmware gates that code behind -a BLE capability flag the simulator build does not set, so the file compiles to -stubs. It was verified with a host program compiled straight against these -headers instead. Verified by running, 2026-08-23: +At the time these were written the gate could not reach the binary semaphore: +the firmware gated that code behind a BLE capability flag the simulator build +did not set, so the file compiled to stubs. It was verified with a host program +compiled straight against these headers instead. Verified by running, +2026-08-23: - an empty binary semaphore, `ticksToWait == 0`, returns `pdFALSE` in under 20 ms @@ -245,3 +246,48 @@ Note one pre-existing quirk that parity run confirms is unchanged: `xQueuePeek` on a mutex uses `try_lock`, and a `std::recursive_mutex` grants `try_lock` to its own holder. So peek from the holding thread reports the mutex as free. Only another thread sees it as taken. + +## Since exercised by real firmware, not only by a host program + +Later the same day the capability flag was turned on for the simulator build, +so the confirm-timeout path ran inside real firmware over a fake BLE link. +Verified by running, 2026-08-23: + +- **The 3000 ms wait is 3000 ms, 23 times in a row.** One multi-line reply at + the pessimistic MTU splits into 23 blocks, and against a peer that never + acknowledges anything each block cost one full timeout: 23 waits, gaps + measured at 2981.9 ms minimum and 3063.4 ms maximum, mean 3003.4 ms, 69.1 s + to drain. Timed on two independent clocks -- the peer's monotonic clock and + the firmware's own millisecond log -- and both agree. Under the old shim + every one of those waits returned success immediately, so the firmware's + whole give-up path was dead code that looked exercised. +- **A give from another thread wakes it, inside firmware.** The token is given + from the fake radio's callback thread and taken on the activity thread, which + is the arrangement the standalone check simulated. +- **`vTaskDelay` sleeping is what makes a retry budget mean anything.** A + 40-attempt, 25 ms retry loop is 1 s of real time now and was 0 s before. + +## A firmware file can use these names without including them + +Worth stating because the failure reads like a shim gap and is not. A firmware +translation unit compiled fine on device while using `portMUX_TYPE`, sixteen +`portENTER_CRITICAL`/`portEXIT_CRITICAL` sites, `vTaskDelay`, `pdMS_TO_TICKS` +and the whole semaphore API **without naming a single FreeRTOS header**. It +built because a different library it did include -- a Bluetooth stack -- pulls +`Arduino.h` in, and that drags FreeRTOS along with it. Replace that library +with a header-compatible fake that does not, and the file stops compiling. + +The symptom is unmistakable once you know it: every error is a +`was not declared in this scope` on a FreeRTOS name, and none is on a name +belonging to the library that was replaced. The fix is in the firmware, not +here -- name the three headers it actually uses: + +```cpp +#include +#include +#include +``` + +Those are the paths ESP-IDF owns, so the change is correct on both targets and +needs no conditional. Expect one of these per firmware file that has been +free-riding on some other library's include graph. From 8ca7eaf0e03c11524097f71e8d0cef2f05c2a0e5 Mon Sep 17 00:00:00 2001 From: Roman Fordinal Date: Sun, 23 Aug 2026 19:12:02 +0200 Subject: [PATCH 6/6] docs: name the downstream firmware generically, for an upstream reader The prose cited lib/BlePositionServer paths an upstream reviewer cannot open. Reworded to 'the firmware this simulator was developed against'; the behaviour being explained is unchanged and still checkable against FreeRTOS itself. The captured transcript keeps its BlePositionServer.cpp:NNN tags verbatim -- editing recorded output would falsify it -- with a line above saying those tags name a downstream firmware and not files in this repo. --- docs/freertos-shim.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/freertos-shim.md b/docs/freertos-shim.md index 3dbd859..e7c8f82 100644 --- a/docs/freertos-shim.md +++ b/docs/freertos-shim.md @@ -18,8 +18,8 @@ Read off the code, before this change: unused). It always blocked until it got the mutex, then returned `true`. So firmware that creates a confirm semaphore and waits on it with a deadline -could not time out. The concrete case in the firmware this simulator builds: -`lib/BlePositionServer/src/BlePositionServer.cpp:261` creates it with +could not time out. The concrete case, in the firmware this simulator was +developed against: its BLE server creates the semaphore with `xSemaphoreCreateBinary()`, `:621` polls it with `ticksToWait == 0` to drain a stale give, `:629` waits 3000 ms and treats anything but `pdTRUE` as a dropped reply, and `:75` gives it from the BLE callback thread. Under the old shim @@ -34,10 +34,9 @@ time. A firmware retry loop of 40 iterations x 25 ms ran instantly, and every ## Gap 3: no `pdMS_TO_TICKS` -Also absent, and not merely unused: the firmware passes every one of its -millisecond timeouts through it. Four call sites, all in -`lib/BlePositionServer/src/BlePositionServer.cpp` -- `:249` -`vTaskDelay(pdMS_TO_TICKS(20))`, `:404` `vTaskDelay(pdMS_TO_TICKS(50))`, `:629` +Also absent, and not merely unused: that firmware passes every one of its +millisecond timeouts through it. Four call sites, all in its BLE server -- +`vTaskDelay(pdMS_TO_TICKS(20))`, `vTaskDelay(pdMS_TO_TICKS(50))`, `xSemaphoreTake(sem, pdMS_TO_TICKS(kConfirmTimeoutMs))` with `kConfirmTimeoutMs = 3000` (`:612`), and `:653` `vTaskDelay(pdMS_TO_TICKS(kRetryDelayMs))` with `kRetryDelayMs = 25` (`:608`). @@ -217,6 +216,9 @@ sites verbatim. All 13 assertions passed, and the two `static_assert`s compiled, which is what proves the macro is usable in a constant expression like the real one: +Captured verbatim. The `BlePositionServer.cpp:NNN` tags name call sites in +the downstream firmware this was checked against, not files in this repo. + ``` pdMS_TO_TICKS(0) = 0 want 0 ok pdMS_TO_TICKS(1) = 1 want 1 ok