From 56230dd3407e942e63bcdf7a9638ca97f5915e25 Mon Sep 17 00:00:00 2001 From: Dominic Masters Date: Fri, 31 Jul 2026 15:02:17 -0500 Subject: [PATCH] Fix ref-count underflow, PSP timezone units, thread restart race, color codegen - util/ref.c: refUnlock's assert(count >= 0) on an unsigned count was tautological, so a double-unlock silently underflowed to UINT32_MAX instead of asserting. Now asserts count > 0 before decrementing. - duskpsp/time/timepsp.c: timeGetRealTimeZonePSP returned hours while every other platform (and timeepoch.c's math) expects seconds. - thread.c: threadHandler reset threadId outside the mutex, after signaling STOPPED, letting a caller's immediate threadStart() race threadStartRequest()'s "thread id not 0" assert. threadId is now reset inside the same locked section. - tools/color.py: whole-number CSV channels (0, 1) produced invalid C float literals (0f, 1f) for the generated COLOR_*_3F/_4F macros. Route through float() so they always stringify with a decimal point. Also adds .claude/code-check.md: a full review pass over every subsystem in src/dusk/, with the above (plus several other findings not yet acted on) written up in detail. Co-Authored-By: Claude Sonnet 5 --- .claude/code-check.md | 858 +++++++++++++++++++++++++++++++++++++ src/dusk/thread/thread.c | 8 +- src/dusk/util/ref.c | 5 +- src/dusk/util/ref.h | 3 +- src/duskpsp/time/timepsp.c | 12 +- test/display/test_color.c | 34 ++ test/thread/test_thread.c | 21 +- tools/color.py | 9 +- 8 files changed, 927 insertions(+), 23 deletions(-) create mode 100644 .claude/code-check.md diff --git a/.claude/code-check.md b/.claude/code-check.md new file mode 100644 index 00000000..1c1493d5 --- /dev/null +++ b/.claude/code-check.md @@ -0,0 +1,858 @@ +# Dusk Code Check + +A working checklist for walking through `src/dusk/` (the core, platform- +agnostic engine layer) one subsystem at a time and reviewing how it +actually looks today -- not just what it's supposed to do. Pairs with +`STATUS.md` (maturity snapshot) and `ROADMAP.md` (forward-looking work); +this doc is about auditing what's already there. + +Each section below: **Purpose**, **Key files**, and a **Review status** +line to fill in as each system gets walked through (e.g. `Not started` / +`In progress` / `Reviewed 2026-08-01: `). + +First full pass completed 2026-08-01 (all 23 sections below reviewed). +See "Top cross-cutting findings" right after the index for the +highest-priority items across all of them before diving into any one +section -- this doc is long, don't make someone read all 23 to find +the load-bearing bugs. + +Game-specific code lives in `src/duskrpg/` (not covered here) and +platform-specific implementations live in `src/dusk/` / +`src/duskgl/` / `src/dusksdl2/` (also not covered here) -- see `CLAUDE.md` +for the layer structure. + +--- + +## Index + +- [error](#error) +- [assert](#assert) +- [util](#util) +- [event](#event) +- [time](#time) +- [thread](#thread) +- [log](#log) +- [system](#system) +- [console](#console) +- [asset](#asset) +- [locale](#locale) +- [save](#save) +- [network](#network) +- [display](#display) +- [ui](#ui) +- [entity](#entity) +- [scene](#scene) +- [physics](#physics) +- [animation](#animation) +- [script](#script) +- [input](#input) +- [engine](#engine) +- [game](#game) + +--- + +## Top cross-cutting findings + +Ranked roughly by real-world impact, not by section order. Each links +to its full writeup below. + +1. **Bounds/capacity checks that matter at runtime are `assert*` calls, + which compile to no-ops in release (`DUSK_ASSERTIONS_FAKED`, set + whenever `NDEBUG` is defined) -- confirmed by reading `assert.h` + directly.** This shows up in [entity](#entity) (`entitymanager.c`, + `component.c` -- reachable from JerryScript via `moduleentity.c` + with no return-value check) and [scene](#scene) (`sceneCreate`'s + pool-exhaustion path, reachable from `new Scene()`). Untrusted + script input can silently get back an invalid ID that later indexes + a fixed array out of bounds in a release build -- the exact + "validate untrusted data with errorThrow, not asserts" case + CLAUDE.md/project memory already calls out, just not applied here. +2. **FIXED 2026-08-01.** `util/ref.c`'s `refUnlock` decremented an + unsigned `count` guarded only by a tautological `count >= 0` assert + ([util](#util)) -- a double-unlock silently underflowed to + `UINT32_MAX` instead of tripping an assert. This wasn't just a + `util/` nit: it's the lock/unlock primitive `asset/loader/assetentry.c` + builds on (see [asset](#asset)), so an over-unlocked asset entry + became permanently un-reapable rather than erroring loudly. Now + asserts `count > 0` before decrementing; the pointless + `count >= 0` check in `refLock` was removed too. +3. **`texture.c:42-52`'s `textureInit()` reads `texture->format` to + decide which `data` fields to validate *before* the struct is + zeroed a few lines later** ([display](#display)) -- it's validating + whatever stale value happened to be in the caller's (possibly + stack) memory, not the `format` parameter actually being requested. +4. **FIXED 2026-08-01 (confirmed real before fixing).** PSP's + `timeGetRealTimeZonePSP` returned hours; every other platform + (`timeGetRealTimeZoneLinux`/Dolphin) returns seconds, and + `timeEpochInit`/`timeEpochGetHours` etc. add the timezone value + directly to a Unix-epoch-seconds timestamp -- confirmed by reading + all three platform implementations plus `timeepoch.c` directly, not + just trusting the review agent's claim. The PSP function's own + comment even said "Return timezone offset in hours". Now divides by + `1000000.0` only (microseconds -> seconds), matching the other + platforms. See [time](#time). +5. **Discussed, not changed.** `consolePrint` aborts the process (via + `stringFormatVA`'s `assert(ret < destSize)`) instead of truncating + when a single debug line exceeds 511 bytes. This is a real, if rare, + crash risk (any format producing >511 bytes of runtime-variable + content -- a long path, a long error string -- takes the whole + process down over a debug print), but changing `stringFormatVA` + itself would affect every caller in the codebase, and a + console-local truncating rewrite is easy to do wrong (vsnprintf + directly, bypassing the project's string-utils convention) without + more time to get right. Left as-is pending a decision on which + fix shape is wanted. See [console](#console). +6. **FIXED 2026-08-01.** `thread.c`'s `threadHandler` signaled + `THREAD_STATE_STOPPED` before resetting `threadId = 0`, and + `threadStartRequest` wasn't synchronized against that reset -- a + real (if rare-window) race, not hypothetical: the test suite was + already working around it by re-calling `threadInit()` instead of + exercising true stop/start reuse. `threadId` is now reset inside the + same mutex-locked section before the STOPPED signal/unlock; + `test_thread_restart` now loops 20x on the same `thread_t` without + re-initializing, which would have caught the old race. See + [thread](#thread). +7. **FIXED 2026-08-01.** `tools/color.py` emitted invalid C float + literals (`0f`, `1f`, etc.) for any whole-number CSV channel -- + confirmed by actually running the generator against `color.csv` + before and after. Was dormant only because `COLOR_*_3F`/`_4F` were + unreferenced anywhere in the repo; the first real use of a float + color variant wouldn't have compiled. Now routes channel values + through Python `float()` before interpolating (always stringifies + with a decimal point, e.g. `0.0`/`1.0`), and `test/display/test_color.c` + gained two tests that reference `COLOR_BLACK_3F`/`COLOR_WHITE_4F`/ + `COLOR_RED_3B`/`COLOR_GRAY_3F` directly -- giving the previously-dead + generated macros real compile-time + value coverage instead of + silently bit-rotting again. See [display](#display). +8. **`assetModelLoaderAsync` is confirmed fully dead** -- model loading + is genuinely synchronous end-to-end (verified by tracing the state + machine, not just repeating STATUS.md's claim). See [asset](#asset). +9. **No cycle guard on prefab `extends` chains** in + `scenePrefabResolveAndApply` -- a self-referencing or A->B->A prefab + table is a stack-overflow crash from what looks like a harmless + typo in hand-authored data. See [scene](#scene). +10. **Untested-code hot spots worth prioritizing** if test investment + is on the table: `util/ref.c`/`random.c`/`crypt.c`/`endian.c` + (zero tests), `animation/easing.c` (13 of 16 functions untested), + `event/`, `console/`, `system/`, `log/`, `input/`, `engine/`, + `locale/`, `save/`, and most of `display/` (mesh, shader, texture, + tileset, text, screen, framebuffer) have no dedicated tests at + all. + +Convention-only items (real, but lower stakes than the above): raw +stdlib string functions (`strlen`/`strchr`/`strstr`/etc.) show up +instead of `util/string.h` wrappers in `network/http/` and +`asset/loader/locale/assetlocaleloader.c` ([network](#network), +[asset](#asset)); several `display/mesh/` files use `//` header +comments and `/* */` inline comments where CLAUDE.md wants the +opposite ([display](#display)); `animation/`'s core files all use `//` +copyright headers instead of `/** */` ([animation](#animation)). + +--- + +## error + +**Purpose:** The engine's single error-handling convention. Every +fallible function returns `errorret_t`, built with `errorOk()` / +`errorThrow(fmt, ...)` / `errorChain(call)` and consumed with +`errorIsOk()`/`errorIsNotOk()`/`errorCatch()`. No `errno`, no raw error +codes anywhere else in the engine. + +**Key files:** `error.c/h`. + +**Review status:** Reviewed 2026-08-01. Solid, and tested unusually well +(`test/error/test_error.c` covers thread isolation and concurrent +throws, not just happy path). Two issues: `errorChainImpl` (error.c:86-91) +hand-rolls `snprintf`/`strlen` instead of `util/string.h`'s +`stringFormat`/`stringFormatVA` wrappers that `errorThrowImpl` uses one +function above it -- an inconsistency, not just a style nit, since +CLAUDE.md requires the wrappers everywhere. `errorThrowWithCode` +(error.h:100) is dead code -- zero call sites anywhere in `src/`/`test/`. + +--- + +## assert + +**Purpose:** Debug-time invariant checks -- `assertNotNull`, `assertTrue`, +`assertFalse`, `assertUnreachable`, `assertIsMainThread`. Distinct from +`error/`: asserts are for programmer mistakes/invariants, `errorret_t` is +for runtime-recoverable failures (see `CLAUDE.md`'s note on validating +untrusted data with `errorThrow` rather than asserting). + +**Key files:** `assert.c/h`. + +**Review status:** Reviewed 2026-08-01. Clean, no issues found. Impl +functions + macros wrapping `__FILE__`/`__LINE__` and a proper +`DUSK_ASSERTIONS_FAKED` no-op path for release builds; +`assertNotNullImpl` (assert.c:109-111) does a real "touch" dereference +after the null check as a belt-and-braces addition. Test coverage +exercises both success and `expect_assert_failure` failure paths for +every variant, including `assertDeprecated`/`assertIsMainThread`. + +--- + +## util + +**Purpose:** Small stand-alone helpers with no engine-state dependencies: +`array` (dynamic array), `crypt` (hashing/checksum), `endian` (byte-order +swap), `math` (clamp/lerp/pow2/etc.), `memory` (`memoryAllocate`/`Free`/ +`Zero`/`Copy`/`Compare` -- the only allocator the rest of the engine is +allowed to call directly), `random`, `ref` (refcounted handle with +lock/unlock callbacks), `sort` (bubble sort used for one-time static list +ordering), `string` (`stringCopy`/`Compare`/`Format`/etc. -- required in +place of libc `str*` functions per `CLAUDE.md`). + +**Key files:** `array.c/h`, `crypt.c/h`, `endian.c/h`, `math.c/h`, +`memory.c/h`, `random.c/h`, `ref.c/h`, `sort.c/h`, `string.c/h`. + +**Review status:** Reviewed 2026-08-01. Several real issues, worst was +in `ref.c`: `refUnlock` (ref.c:37-39) decremented `count` (`uint32_t`) +guarded only by `assertTrue(ref->count >= 0, ...)` -- tautologically +always true on an unsigned type, so a double-unlock silently underflowed +to `UINT32_MAX` and kept invoking `onUnlock` instead of tripping an +assert (confirmed by reading ref.c directly). **Fixed 2026-08-01** -- +`refUnlock` now asserts `count > 0` before decrementing; also fixed +`refInit`'s doc comment, which claimed a fresh ref starts at count 1 +when the code has always started it at 0. Same tautological unsigned +`>= 0` assert remains in `memory.c:98` (lower stakes -- `memoryFree`'s +own counter, not a shared lock/unlock primitive other subsystems build +on -- left as-is). `memory.h:11` declares +`static size_t MEMORY_POINTERS_IN_USE = 0;` directly in the header -- +confirmed present -- meaning every TU including it gets its own private +copy (only "works" because the accessors all live in memory.c). +`random.c`'s `randomInt` (confirmed, random.c:15-16) does +`rand() % (max - min)` with no `max > min` check (UB on `max <= min`), +and `srand()` is never called anywhere in the repo -- unseeded, +deterministic RNG. `sort.c:1` has `#include ` before the +copyright header (confirmed). Test coverage: array/math/memory/sort/ +string are thorough; `ref.c`, `random.c`, `crypt.c`, `endian.c` have +**zero** tests. `randomInt`/`randomFloat` also appear unused +engine-wide (dead code, not just untested). + +--- + +## event + +**Purpose:** Generic pub/sub primitive -- `event_t` wraps a caller-owned +array of `eventcallback_t`/user-pointer pairs; `eventInit`/`eventSubscribe`/ +`eventInvoke`. Used for things like `uifullbox_t`'s transition-end +callback and `uiloading_t`'s show/hide callbacks. Per `STATUS.md`: "Clean, +small, finished", built to replace the old input system's callback +pattern. + +**Key files:** `event.c/h`. + +**Review status:** Reviewed 2026-08-01. Well-formed -- correctly uses +swap-with-last for O(1) unsubscribe, asserts on capacity overflow and +duplicate-subscribe, no dead code (`eventUnsubscribe` used in 3 other +files). No `errorret_t` needed here since these are programmer-error +assertions rather than runtime-data validation, consistent with +CLAUDE.md's assert-vs-error guidance. One gap: `test/event/` doesn't +exist -- zero unit coverage, including the capacity-exceeded and +double-subscribe assert paths. + +--- + +## time + +**Purpose:** Frame delta/elapsed time (`TIME.delta`, exposed to JS via +`moduleTime`) plus `timeepoch_t` wall-clock timestamps (used by +`uifps.c` for the FPS counter). + +**Key files:** `time.c/h`, `timeepoch.c/h`. + +**Review status:** Reviewed 2026-08-01. `time.c:52-56` unconditionally +formats an epoch string into a 256-byte stack buffer every single +`timeUpdate()` call purely to feed a commented-out `consolePrint` -- +burns the leap-year/day-counting loop three times a frame for a +discarded value. `timeepoch.c`'s day-counting loops have no +handling/guard for negative epoch/timezone values (pre-1970). Test +coverage is solid for `time.c` (mocked ticks, boundary/backwards-time +cases) but `timeepoch.c`'s date math (leap year, rollover, format +specifiers) has no dedicated tests despite being the more bug-prone +half. The PSP-specific timezone unit bug flagged in the cross-cutting +findings (#4) was confirmed and fixed 2026-08-01 -- +`timeGetRealTimeZonePSP` was returning hours where `timeepoch.c` +expects seconds. + +--- + +## thread + +**Purpose:** Thin cross-platform thread/mutex/thread-local wrappers. +Used by the asset system's background loading and the POSIX console's +input-polling thread. + +**Key files:** `thread.c/h`, `threadmutex.c/h`, `threadlocal.h`. + +**Review status:** Reviewed 2026-08-01. Conventions followed +throughout. Real race condition, **fixed 2026-08-01**: `threadHandler` +used to unlock `stateMutex` and signal `THREAD_STATE_STOPPED` *before* +resetting `thread->threadId = 0`, but `threadStartRequest` asserts +`threadId == 0` with no synchronization against that reset -- +`threadStop()` immediately followed by `threadStart()` on the same +struct could intermittently trip the assert. Telling sign it was real, +not theoretical: `test/thread/test_thread.c` already re-called +`threadInit()` before restarting specifically "so threadId/state are +reset" -- the test suite was working around the bug rather than +exercising true reuse. `threadId` is now reset inside the same +mutex-locked section, before the signal/unlock; the test now loops +stop/start 20x on the same `thread_t` with no `threadInit()` in +between. Otherwise test coverage is good (real pthreads, try-lock +coordination, a 4-thread x 10k-iteration mutex contention test). + +--- + +## log + +**Purpose:** `logDebug`/`logError` -- the engine's only print/debug-output +path. Header-only declarations; each platform provides the actual +implementation (see `src/dusk/log/`). + +**Key files:** `log.h` (declarations only, no `.c` in core). + +**Review status:** Reviewed 2026-08-01. Clean 2-function API; all +three platform implementations (`dusklinux`, `duskpsp`, `duskdolphin`) +match the declared signatures with correct `va_copy` usage for the +dual stdout+file writes. Dolphin's `logError` correctly falls back to +raw VIDEO/console init when the framebuffer isn't ready yet and blocks +on a START-button dismiss loop; PSP writes to +`ms0:/PSP/GAME/Dusk/*.log`. No bugs found. No `test/log/` exists -- +understandable for I/O/side-effect code, but zero coverage. + +--- + +## system + +**Purpose:** Very early engine init (`systemInit`), the +`systemplatform_t` enum (via the `systemplatformlist.h` X-macro pattern), +and native OS dialog boxes (`systemdialogtype_t`: render-blocking vs +tick-blocking). + +**Key files:** `system.c/h`, `systemplatformlist.h`. + +**Review status:** Reviewed 2026-08-01. X-macro platform enum and +compile-time `#error` guards for missing platform macros used +correctly; all three platforms define the required +`systemInitPlatform`/`systemGetActiveDialogTypePlatform`/ +`systemGetCyclesPlatform`. Dead code confirmed: `systemPSPGetLanguage()` +(`duskpsp/system/systempsp.c:51`) and `systemGetLanguageDolphin()` +(`duskdolphin/system/systemdolphin.c:40`) are defined but never called +anywhere -- likely intended for locale wiring that hasn't landed. No +`test/system/` exists. + +--- + +## console + +**Purpose:** In-engine dev console: fixed-size scrollback +(`CONSOLE_HISTORY_MAX` lines x `CONSOLE_LINE_MAX` chars), `consolePrint`, +a `dirty` flag consumers (namely `ui/debug/uiconsole.c`) watch to know +when to rebuild their own cached representation. POSIX builds also poll +stdin on a background thread for interactive script execution. + +**Key files:** `console.c/h`, `consoledefs.h`. + +**Review status:** Reviewed 2026-08-01. Two real issues. `consolePrint` +(console.c:32-44) gets its length from `stringFormatVA(buffer, +CONSOLE_LINE_MAX, ...)`, and `stringFormatVA` (util/string.c:100) +**asserts** `ret < destSize` rather than truncating -- so any single +debug line over 511 bytes aborts the process instead of just getting +cut off, a harsh failure mode for a dev console. Separately, +`consoleUpdate`'s doc ("Processes pending queued script lines") and +`CONSOLE_EXEC_BUFFER_MAX` (consoledefs.h:12) describe a queued-script- +execution feature that doesn't exist -- `console_t` has no exec-queue +fields, `consoleUpdate` only toggles `visible`, and +`CONSOLE_EXEC_BUFFER_MAX` is unreferenced elsewhere (dead constant, +stale doc). No `test/console/` exists. + +--- + +## asset + +**Purpose:** The asset pipeline: `dusk.dsk` zip archive reading +(`assetfile.c/h`), lock/ref-counted entries (`asset/loader/assetentry.c/h`, +built on `util/ref.h`), batch loading (`assetbatch.c/h`), and the loader +registry (`asset/loader/assetloader.c/h`, plus one subfolder per loader +type: `dmf/` mesh+model, `display/` texture+tileset, `locale/`, `json/`, +`animation/`). See `CLAUDE.md`'s "Adding a new asset loader type" for the +extension pattern. Per `STATUS.md`: model loading's "async" path is +actually synchronous -- the one known stub in core. + +**Key files:** `asset.c/h`, `assetfile.c/h`, `assetbatch.c/h`, +`loader/assetentry.c/h`, `loader/assetloader.c/h`. + +**Review status:** Reviewed 2026-08-01. Core is well-structured and the +multi-pass reaper correctly handles parent/child dependency chains. +Found: `assetFileInit` (assetfile.c:29) has `if(!zip_stat(...) == 0)` -- +precedence makes this `(!zip_stat(...)) == 0`, which happens to +evaluate correctly for zip's 0/nonzero convention but reads as broken +and should just be `zip_stat(...) != 0`. More seriously, +`assetEntryLock`/`Unlock` sit on top of `util/ref.c`'s confirmed +double-unlock underflow bug (see **util** above) -- an over-unlocked +asset entry silently becomes un-reapable forever instead of asserting. +**Confirmed via code-trace** (not just citing STATUS.md): +`assetModelLoaderAsync` (assetmodelloader.c:19-23) is genuinely dead -- +`assetEntryStartLoading` unconditionally sets +`ASSET_ENTRY_STATE_PENDING_SYNC`, and `assetModelLoaderSync`'s state +machine never transitions to `PENDING_ASYNC` the way mesh/texture/ +tileset/locale/json/animation all do, so model loading runs fully +synchronously via `assetRequireLoaded`'s busy-loop; grep confirms +`assetModelLoaderAsync` has no callers. `assetlocaleloader.c` uses raw +stdlib (`strstr`/`strncmp`/`strchr`/`strtol`/`atoi`) instead of +`util/string.h` wrappers throughout, and tests line-prefixes via +`memoryCompare(lineBuffer, "msgid", 5)` without confirming the reused +stack buffer actually holds 5 valid bytes at that point -- a short line +could spuriously prefix-match against stale buffer contents. Test +coverage: good breadth for entry lifecycle, tileset/JSON/animation +loaders, and locale parsing; no dedicated tests for mesh/model/texture +loader internals or `assetfile.c`'s line reader. + +--- + +## locale + +**Purpose:** Active-locale tracking (`LOCALE.locale`/`LOCALE.entry`) over +`.po`-derived locale assets loaded through the asset system. See +`assets/locale/en_US.po` -- per memory, currently placeholder/test +content, not real shipped game text. + +**Key files:** `localemanager.c/h`, `localeinfo.h`. + +**Review status:** Reviewed 2026-08-01. Clean, no bugs found -- +correctly balances `assetEntryLock`/`Unlock` in both `SetLocale` and +`Dispose` (unlocks the old entry before adopting the new one). Minor +nit: `localeinfo.h` declares `static const localeinfo_t` globals +(`LOCALE_EN_US` etc.) directly in the header, so every including TU +gets its own duplicate copy -- harmless given how few TUs include it. +No `test/locale/` directory; `test/asset/test_assetlocale.c` tests the +loader, not `localemanager.c` itself. + +--- + +## save + +**Purpose:** Slot-based save data (`SAVE_FILE_COUNT_MAX` fixed slots), +yyjson documents persisted with size + CRC32 checksum. `saveLoad`/ +`saveSave` read/write JSON fresh every call; callers own the returned +doc and must free it. Actual file I/O goes through platform stream hooks +(`saveplatform_t`, one impl per platform) -- core `save.c` must never +touch the filesystem directly. `savesettings.c/h` layers engine settings +(display/audio/input/general) on top. Per `CLAUDE.md`: "mature, but +undocumented" beyond the one paragraph there -- no tests. + +**Key files:** `save.c/h`, `savefile.h`, `savestream.c/h`, +`savesettings.c/h`. + +**Review status:** Reviewed 2026-08-01. Clean and convention-compliant +-- `save.c` genuinely never touches the filesystem directly (every I/O +path goes through `saveXxxPlatform` macros, all three platforms define +them), yyjson doc ownership is honored at every call site checked, and +CRC32 is validated before parsing with every error path freeing `buf`. +One real gap: `saveFileLoad`'s `memoryAllocate(file->size)` +(savestream.c:122) allocates straight from an on-disk size field with +no sanity bound before the allocation -- a corrupted/tampered save +could request an oversized allocation, worth a max-size guard given the +"never trust incoming data" principle (arguably applies to save files, +not just network packets). Confirmed no `test/save/` exists. `saveDelete`/ +`saveExists`/`saveGet` are public API not yet called anywhere in the +repo -- likely just unconsumed so far, not truly dead. + +--- + +## network + +**Purpose:** Connection-state layer only (`networkstate_t`: +`DISCONNECTED`/`CONNECTING`/`CONNECTED`/`DISCONNECTING`) plus a one-off +HTTP client (`network/http/`: request/header/URL/thread/process split +across files). **Not** a multiplayer/replication protocol yet -- that's +`ROADMAP.md` items 8-17. Platform-specific connection polling (PSP's +`sceNetApctl`, GameCube/Wii's `if_config()`) lives under +`src/dusk/network/`. Has HTTP tests only (`test/network/http/`). + +**Key files:** `network.c/h`, `networkinfo.c/h`, `http/networkhttp.c/h`, +`http/networkhttprequest.c/h`, `http/networkhttpthread.c/h`. + +**Review status:** Reviewed 2026-08-01. Genuinely stays a thin +connection-state layer (grepped for "packet"/"replicat"/"multiplayer"/ +"udp" -- no hits, no scope creep). The HTTP client correctly validates +untrusted server input with `errorThrow` rather than trusting it +(malformed status lines, oversized headers, bad `Content-Length`, +early-closed connections all throw) -- exactly what CLAUDE.md calls +out. `networkhttpthread.c`'s request handoff is actually correct +despite request fields being written outside the mutex: the final +`state = PENDING` write happens under the same mutex the worker +acquires, giving a valid release/acquire handoff (worth a comment, not +a bug). One recurring convention violation: +`networkhttpprocess.c`/`networkhttpurl.c` use raw stdlib +`strlen`/`strchr`/`strstr`/`strncasecmp`/`strcspn` instead of +`util/string.h` wrappers in several places -- `util/string.h` currently +has no direct substitute for `strchr`/`strstr`/`strcspn`, so this may +be a missing-utility gap rather than pure oversight. No leaks found +(request/response bodies freed exactly once via +`networkHttpRequestReset`, redirect loop frees the intermediate body). +Real test coverage exists: `test/network/http/test_networkhttp.c` runs +six tests against a fake loopback server (GET/POST/PUT, query encoding, +redirects, connection-refused), each asserting zero leaked allocations. + +--- + +## display + +**Purpose:** The engine's most mature, most-optimized subsystem -- +platform-agnostic rendering built on top of `duskgl`/`duskdolphin`. +Subfolders: `mesh/` (primitive builders: cube/sphere/capsule/plane/ +triprism/quad, plus the shared `meshvertex_t`/`mesh_t` API), `shader/` +(currently one material type, unlit), `spritebatch/` (the shared +`SPRITEBATCH_SPRITES_MAX`-capacity quad batcher everything 2D goes +through -- flushes on shader/material change), `texture/` (texture + +tileset + palette), `text/` (`font_t`/`FONT_DEFAULT`'s baked-in bitmap +glyphs, `textBuildSpriteCache`/`textDrawSpriteCache`'s build-once/ +translate-per-frame pattern), `screen/` (scan-safe area), `framebuffer/`. +`color.csv` + `tools/color/csv/__main__.py` code-generate `color.h` -- +never hand-edit the generated header. + +**Key files:** `display.c/h`, `mesh/mesh.c/h`, `spritebatch/spritebatch.c/h`, +`spritebatch/spritebatchsprite.c/h`, `text/font.c/h`, `text/text.c/h`, +`texture/texture.c/h`, `texture/tileset.c/h`, `shader/shader.c/h`. + +**Review status:** Reviewed 2026-08-01. "Most mature" oversold it +slightly -- real bugs found, several confirmed by direct read: +- **texture.c:42-52 (confirmed)** -- `textureInit()` branches on + `texture->format` to validate the incoming `data` union *before* + `memoryZero(texture, ...)` runs a few lines later, so it's reading + whatever stale/uninitialized value was already in the caller's + struct, not the `format` parameter actually being requested. + Validation is effectively checking garbage. +- `spritebatchsprite.c:20-25` -- `spriteBatchSpriteTilesetPosition`'s + zero-width/height early return zeroes `sprite.min`/`max` but leaves + `uvMin`/`uvMax` uninitialized (stack garbage); the existing + zero-size test doesn't check uv fields, so this isn't caught. +- `mesh.c:43` -- `meshFlush`'s offset assert (`vertexOffset < + vertCount - 1`) wrongly rejects a valid offset at the very last + vertex; off-by-one. +- `text.c:34-37` -- bounds assert uses `tileIndex <= tileCount` + (should be `<`), currently unreachable in practice since a preceding + clamp already guards it, but the assert itself is wrong. +- `framebuffer.c:23-28` -- `frameBufferBind(NULL)` recurses into + binding the backbuffer without `errorChain`, discarding any failure + and unconditionally returning `errorOk()`. +- `displayInit()`'s six static "SIMPLE" primitive meshes + (quad/cube/sphere/plane/capsule/triprism) are never disposed in + `displayDispose()` -- one VBO+VAO leaked per primitive on the + non-legacy GL path. +- **Confirmed and fixed 2026-08-01.** `tools/color.py` (not the + `tools/color/csv/__main__.py` path CLAUDE.md's doc comment names -- + that's a stale path, only a `__pycache__/` remnant exists there; the + live script is `tools/color.py`) emitted invalid C float literals + for whole-number CSV channels (`0`, `1` -- every black/white/primary + color), e.g. `color3f(0f, 0f, 0f)`. Was harmless *only* because + `COLOR_*_3F`/`_4F` were never referenced anywhere in `src/`/`test/` + -- confirmed by actually running the generator before and after the + fix, not just reading the source. Now routes channel values through + Python `float()` first (always stringifies with a decimal point); + `test/display/test_color.c` gained tests referencing + `COLOR_BLACK_3F`/`COLOR_WHITE_4F`/`COLOR_RED_3B`/`COLOR_GRAY_3F` + directly so the previously-dead macros now have real compile-time + + value coverage. +- Style drift: `mesh.h` uses `//` header comments instead of `/** */`; + sphere/capsule/triprism use `/* */` block comments inside function + bodies where CLAUDE.md mandates `//`; every primitive builder's + JSDoc still documents a `@param color` that no longer exists in the + signature (leftover from before color moved out of `meshvertex_t`). +- The primitive `*Buffer()` flexible-geometry functions (sphereBuffer, + capsuleBuffer, etc.) are confirmed unused outside their own + `*Init()` -- only the fixed default mesh instances are actually + consumed (via `script/module/display/modulemesh.c`). + +Test coverage: `test/display/` only registers `test_color.c` and +`test_spritebatchsprite.c` -- mesh/shader/texture/tileset/palette/text/ +font/screen/framebuffer all have zero unit tests. + +--- + +## ui + +**Purpose:** Flat, static, singleton-driven widget framework -- no scene +graph. Top level is a compile-time X-macro registry +(`uielementlist.h`/`uielement.c/h`) of `{init, update, draw, dispose, +order}` tuples (screens: frame/confirm/settings/overlays/console/fps), +spliced with `duskrpg`'s own list. Composition below that is `uimenu_t` +(`widget/uimenu.c/h`) -- a flat tagged-union array of widgets laid out in +a simple row/column grid via the `MENU_*` authoring macros; no nesting. +Widgets: `uilabel`/`uiwidgetlabel` (cached glyph sprites, the pattern +every other widget's text now uses), `uibutton`, `uicheckbox`, +`uislider`, `uidropdown`, `uitab`, `uiscrolling` (empty placeholder). +`focus/` is the gamepad/keyboard directional-navigation stack (no mouse/ +pointer hit-testing exists anywhere). `frame/` is the 9-slice box plus +the confirm dialog and settings screens. `overlay/` is fullscreen +fades/loading/crop bars. `debug/` is the FPS counter and dev console +renderer. + +As of 2026-07-31: slider/tab/frame (`uiFrameDrawCached`)/duskrpg's +textbox all cache their sprite geometry instead of rebuilding it every +frame; console uses a fixed-size vertex buffer instead of alloc/free. +Still open: sprite-batch flushes are keyed on material/color, so +per-widget highlight-color changes still force a flush per widget (see +`ROADMAP.md` item 5 in the debt backlog). No JS bindings exist for UI +yet. `test/ui/`/`test/display/test_spritebatchsprite.c` cover the new +caching *logic* only -- nothing exercises real `uiXxxDraw()` calls, since +those need a live GL context (`FONT_DEFAULT`/`UI_FRAME` textures) this +repo's test binaries don't have. + +**Key files:** `ui.c/h`, `uielement.c/h`, `focus/uifocus.c/h`, +`widget/uimenu.c/h`, `widget/uilabel.c/h`, `frame/uiframe.c/h`, +`debug/uiconsole.c/h`. + +**Review status:** Reviewed 2026-07-31 (this session's sprite-caching +pass) -- see notes above; per-material flush granularity and hit-testing +still open. + +--- + +## entity + +**Purpose:** Fixed-size-array ECS. `entitymanager_t` owns +`ENTITY_COUNT_MAX` entities, each with up to `ENTITY_COMPONENT_COUNT_MAX` +components stored in a tagged union (`componentdata_t` -- flagged in +`PSP_OPTIMIZATION_PLAN.md` as the biggest PSP memory-waste candidate, +since every entity reserves space for every component type's largest +variant). `componentlist.h`'s `X()` macro auto-generates the component +enum/union/dispatch table from `entity/component/**`; engine-level +`entityprefablist.h` is an empty sentinel (real prefabs live in +`duskrpg/entity/entityprefablist.h`, resolved via +`entityPrefabResolveAndApply`). No JSON serialize/deserialize (removed by +design in favor of C-coded prefabs or JerryScript `Entity`/`Component`). + +**Key files:** `entitymanager.c/h`, `entity.c/h`, `component.c/h`, +`componentlist.h`, `entityprefab.c/h`. + +**Review status:** Reviewed 2026-08-01. Structurally sound (sentinel +prefab list, X-macro shape both match CLAUDE.md), but there's a real +security-relevant gap: **every capacity/bounds guard +(`entitymanager.c:35`, `entity.c:56`, `component.c:37-39,58-62,75-76, +140-141`) is an `assert*` call**, and `assert.h` compiles these to +`((void)0)` under `DUSK_ASSERTIONS_FAKED` (auto-set whenever `NDEBUG` +is defined -- confirmed by reading assert.h directly, this is a real +release-mode no-op, not a hypothetical). `moduleentity.c:30,67` create +entities/components from JerryScript with no return-value check, so a +script that exceeds `ENTITY_COUNT_MAX`/the per-entity component cap +silently gets back an invalid ID in release builds, which then indexes +fixed arrays out-of-bounds later. This directly contradicts the +project's own "untrusted-data limits need errorThrow, not asserts" +guidance -- and script input is about as untrusted as it gets. Minor: +`component.h:100`/`component.c:52` misplace the `*` in `void * +componentGetData(`. Dead code: `entityUpdateRemove`/ +`entityDisposeRemove`/`entityDisposeAdd` have zero call sites. + +--- + +## scene + +**Purpose:** Same shape as `entity/` one level up -- `SCENE_COUNT_MAX` +fixed-count scenes, each an isolated entity-manager pool; engine-level +`sceneprefablist.h` is an empty sentinel (real prefabs in +`duskrpg/scene/`). As of the `Scene.set()` work (2026-07-31), JerryScript +can install a `{init, update, dispose}` module as the active scene via +`modulescene.c`'s `moduleSceneSetStatic`/`moduleSceneUpdateCurrent`/ +`moduleSceneTeardownCurrent` (see `script` below) -- scene IDs are reused +from a small pool immediately after `sceneDestroy`, so don't assume a +freshly created scene has a "new" ID. + +**Key files:** `scene.c/h`, `scenebase.h`, `sceneprefab.c/h`. + +**Review status:** Reviewed 2026-08-01. Shares entity/'s assert-not- +errorThrow bounds gap (`sceneCreate` hits `assertUnreachable` then +falls through to `SCENE_ID_INVALID` on pool exhaustion in release -- +reachable from `new Scene()` in JS). Additional finding: +`scenePrefabResolveAndApply`/`scenePrefabInit` (sceneprefab.c:31-54) +recurse through a prefab's `extends` chain with **no cycle guard** -- a +self-referencing or A→B→A prefab table causes unbounded recursion/ +stack overflow from what's otherwise a one-typo mistake in a +hand-authored prefab list. No dead code, no use-after-free. Tests cover +create/destroy/isolation/update cadence well but have zero coverage for +pool exhaustion, `sceneRender()`, or `sceneprefab.c` (no prefab test +file exists at all). + +--- + +## physics + +**Purpose:** `physicsworld_t` step simulation over `physicsbodytype_t` +(static/dynamic/kinematic) and `physicshapetype_t` (cube/sphere/capsule/ +plane/custom, plus mesh shapes via `physicsshapemesh.c/h`). +`triggersystem.c/h` layers trigger-volume enter/exit events on top. Per +`STATUS.md`: "mature, recently churned" -- a revert/re-disable of "old +ent code" suggests component wiring isn't fully settled yet. Well +tested (`test/physics/`). + +**Key files:** `physicsworld.c/h`, `physicsbodytype.h`, `physicsshape.h`, +`physicsshapemesh.c/h`, `triggersystem.c/h`. + +**Review status:** Reviewed 2026-08-01. `STATUS.md`'s "component +wiring isn't fully settled" caveat looks stale on closer look: +`physicsWorldStep`/`triggerSystemStep` are live-wired in `scene.c:89-90` +and `duskrpg`'s player prefab actively uses both PHYSICS and TRIGGER -- +no `#if 0`/TODO/commented-out code found; the disable-then-revert in +history reads like a same-day whole-ECS revert, not something +physics-specific. Two real bugs found instead: +`physicsbodytype.h:23-27` documents kinematic bodies as capable of +being a player controller, but `physicsworld.c:124,166-167` only ever +writes `onGround` for the dynamic side of a collision pair -- a +kinematic controller's `onGround` is permanently false (latent today, +since the one existing player prefab uses DYNAMIC, not KINEMATIC). +`physicstest.c:213`'s barycentric interior-test branch has no guard +against a zero-area triangle (divide-by-zero/NaN risk in mesh-sphere +collision). Dead code: `physicsShapeMeshCreate` (mesh-terrain +collision) has zero production callers despite being well-tested in +isolation; `entityPhysicsGetShape`/`GetCollideMask` are unwired to the +JS module. Test depth is genuinely strong for what's implemented +(gravity convergence, dynamic-vs-dynamic separation, mask filtering, +mesh sphere/capsule resting) -- "well tested" is earned for the +features that exist, though the unused mesh-collision path and +missing trigger→JS bridge mean the subsystem's *integration* maturity +is a bit narrower than its test depth suggests. + +--- + +## animation + +**Purpose:** Keyframe + easing only -- no blend trees or state machines +yet. `keyframe_t`/`keyframeset_t` interpolate values over time via +`easing.c/h`'s easing functions; `animation.c/h` ties a keyframe set to +playback state. Per `STATUS.md`: terse commit history ("ANIM") suggests +still iterating. + +**Key files:** `animation.c/h`, `easing.c/h`, `keyframe.c/h`, +`keyframeset.c/h`. + +**Review status:** Reviewed 2026-08-01. All 8 core files use `//` +copyright headers instead of the required `/** */` block, inconsistent +with the correctly-formatted `entityanimation.{c,h}` next door. Math +checks out: all 16 easing functions satisfy f(0)=0/f(1)=1 with no +precision issues, and keyframe interpolation clamps (doesn't +extrapolate) outside the time range with no reachable divide-by-zero +-- though it doesn't validate that keyframe times are ascending. 15 of +16 `easing.h` declarations lack Javadoc. Biggest gap: **no +`test_easing.c` exists** -- only linear/in-quad get incidental coverage +via `test_keyframe.c`, so 13 of 16 easing functions are completely +untested. Dead code (test-only, unused by any `duskrpg` game code): +`keyframeSetGetValues`, `keyframeSetFireCallback`, `animationSetEvent`, +`entityAnimationSetKeyframes`/`GetValue`. + +--- + +## script + +**Purpose:** Embedded JerryScript integration. `scriptmanager.c/h` owns +the JerryScript context lifecycle and the exec/call helpers +(`scriptManagerExec`/`ExecFile`/`CallGlobal`/`CallValue`, the last two +sharing exception + promise-draining logic). `scriptproto.c/h` is the +`scriptproto_t` prototype-wrapping helper every module builds on. +`module/` holds each registered JS-facing module: +- `modulebase.h` -- the shared macro toolkit (`moduleBaseFunction`, + `moduleBaseRequireArgs`/`RequireString`/etc., `moduleBaseDefineProp`/ + `Func`/`StaticFunc`, `moduleBaseSetWrappedPointer`, `moduleBaseThrowError`). +- `moduleplatform.h`, `entity/moduleentity.c/h` + + `entity/component/*` (generic `Component` plus typed Position/Physics/ + Renderable wrappers via `modulecomponentlist.c/h`), `scene/modulescene.c/h` + (`Scene`, including `Scene.set()`), `display/modulemesh.c/h`, + `time/moduletime.c/h`, `require/modulerequire.c/h` (CommonJS-style + `require()`/`module.exports`, with a directory stack for relative + resolution -- `scriptManagerExecFile` now pushes its own file's + directory before exec so a top-level entry script's relative + `require()` calls resolve correctly). +- `modulelist.c/h` -- registers/disposes every module above in order; + `moduleListDispose()` is now actually called from + `scriptManagerDispose()` (was a latent bug -- it wasn't, until + 2026-07-31). + +Entry point convention: `assets/scripts/init.js` does +`require('./scenefile.js')` + `Scene.set(module)`. Has unit tests +(`test/script/`), including one that reads the real shipped +`overworldscene.js` off disk rather than an embedded copy. + +**Key files:** `scriptmanager.c/h`, `scriptproto.c/h`, +`module/modulebase.h`, `module/modulelist.c/h`, +`module/require/modulerequire.c/h`, `module/scene/modulescene.c/h`. + +**Review status:** Reviewed 2026-07-31 (this session's Scene.set() + +require()-dir-stack work) -- see notes above. + +--- + +## input + +**Purpose:** Bind-based input (`inputaction.c/h` maps +`INPUT_BIND_*` names to raw `inputbutton_t`s per platform), with a +configurable analog deadzone. Platform button/axis backends live under +`src/dusk/input/`; capability macros (`DUSK_INPUT_GAMEPAD`/ +`KEYBOARD`/`POINTER`) gate what's available per target. Pointer axis +values are plumbed through (`DUSK_INPUT_POINTER`), but nothing in `ui/` +consumes them yet -- no hit-testing exists (see `ui` above). + +**Key files:** `input.c/h`, `inputaction.c/h`, `inputbutton.c/h`. + +**Review status:** Reviewed 2026-08-01. Clean and convention-compliant; +deadzone math (`inputDeadzone`, input.c:244) is correct and safe. One +naming nit: `inputBind`'s param is named `act` in input.h:216 while its +own doc comment calls it `action`. **Confirmed, not just cited from +STATUS.md**: pointer input is genuinely wired end-to-end, not a +dead-end at the platform layer -- SDL2's `inputUpdateSDL2` populates +`INPUT.platform.mouseX/Y`/scroll from `SDL_GetMouseState`, +`inputButtonGetValueSDL2` handles the pointer-axis cases, and +`duskrpg/input/inputbindmap.h` binds `mouse_x`/`mouse_y` to +`INPUT_BIND_POINTERX/Y`. The dead end is specifically at the +*consumption* layer: grepping `src/dusk/ui/` for those binds or any +pointer-axis read turns up nothing -- exactly as STATUS.md says, just +now confirmed rather than assumed. No tests exist for `input/`. + +--- + +## engine + +**Purpose:** Top-level init/update/dispose orchestration +(`engineInit`/`engineUpdate`/`engineDispose`) -- the thing `main.c` calls, +which in turn calls into every subsystem above plus `game/gameInit` etc. +`ENGINE.running`/`ENGINE.version` are the only bits of global state +outside the per-subsystem structs. + +**Key files:** `engine.c/h`. + +**Review status:** Reviewed 2026-08-01, confirmed by direct read of +`engineInit`/`engineDispose`. Init order is safe (time -> console -> +system -> input -> asset -> scriptManager -> save/saveSettings -> +locale -> display -> ui -> network -> scene -> game), with display +before UI and scriptManager before scene/game so JS `Entity`/`Scene` +wrappers exist before any script runs. `engineDispose` is **not** a +clean reverse of that order, though: `localeManagerDispose()` runs +*before* `uiDispose()`/`displayDispose()`, even though locale was +initialized *before* both -- correct reverse order would tear down +UI/display first, and as written there's a real risk of UI/display +teardown code touching already-freed locale data (e.g. any localized +string still held/logged during dispose). Separately, `void +engineExit(void)` (engine.c:95) is defined but **not declared in +engine.h**, and a repo-wide grep finds no caller anywhere -- dead, +unreachable, and in violation of the "every public function must be +declared in its header" rule. No tests exist for `engine/`. + +--- + +## game + +**Purpose:** Intentionally header-only in core -- just declares +`gameInit`/`gameUpdate`/`gameDispose`. The real implementation is +`src/duskrpg/game/game.c` (currently: runs `scripts/init.js` on init). + +**Key files:** `game.h` (no `.c` in core). + +**Review status:** Reviewed 2026-08-01. Confirmed: no `.c` file exists +in `src/dusk/game/` (only `game.h` + `CMakeLists.txt`) -- a pure +interface, as intended. The real `src/duskrpg/game/game.c` +implementation is a thin JS-bootstrap shim: `gameInit` just does +`errorChain(scriptManagerExecFile("scripts/init.js", NULL))`, while +`gameUpdate`/`gameDispose` are both empty (`errorOk()` only). Not a +bug -- consistent with the ongoing migration of game logic into JS -- +but worth flagging as "not yet fleshed out" rather than broken if +anything is expected to happen there per-frame at the C layer. No +tests exist for this layer. diff --git a/src/dusk/thread/thread.c b/src/dusk/thread/thread.c index 96e928a5..137ae7b1 100644 --- a/src/dusk/thread/thread.c +++ b/src/dusk/thread/thread.c @@ -109,9 +109,15 @@ bool_t threadShouldStop(thread_t *thread) { threadMutexLock(&thread->stateMutex); thread->state = THREAD_STATE_STOPPED; + // Reset threadId while still holding the mutex -- threadStop() only + // guarantees the caller has observed STOPPED under this same lock, + // so resetting it after unlocking left a window where a caller could + // see STOPPED and immediately threadStart() again while threadId was + // still nonzero, intermittently tripping threadStartRequest()'s + // "thread id not 0" assert. + thread->threadId = 0; threadMutexSignal(&thread->stateMutex); threadMutexUnlock(&thread->stateMutex); - thread->threadId = 0; return NULL; } #endif \ No newline at end of file diff --git a/src/dusk/util/ref.c b/src/dusk/util/ref.c index faf84a9c..906281fd 100644 --- a/src/dusk/util/ref.c +++ b/src/dusk/util/ref.c @@ -27,15 +27,16 @@ void refInit( void refLock(ref_t *ref) { assertNotNull(ref, "Ref cannot be NULL."); - assertTrue(ref->count >= 0, "Cannot lock a ref with negative count."); ref->count++; if(ref->onLock != NULL) ref->onLock(ref); } bool_t refUnlock(ref_t *ref) { assertNotNull(ref, "Ref cannot be NULL."); - assertTrue(ref->count >= 0, "Cannot unlock a ref with negative count."); + // count is unsigned -- catch an over-unlock here, before it underflows + // to UINT32_MAX and silently leaves the ref un-reapable forever. + assertTrue(ref->count > 0, "Cannot unlock a ref that is already at zero."); ref->count--; if(ref->count > 0) { diff --git a/src/dusk/util/ref.h b/src/dusk/util/ref.h index e2c41af5..f84624b3 100644 --- a/src/dusk/util/ref.h +++ b/src/dusk/util/ref.h @@ -20,7 +20,8 @@ typedef struct ref_s { } ref_t; /** - * Initializes a ref with a count of 1. + * Initializes a ref with a count of 0 -- callers that want to represent + * an initial owning reference must call refLock() themselves afterward. * * @param ref The ref to initialize. * @param data Opaque context pointer accessible to all callbacks. diff --git a/src/duskpsp/time/timepsp.c b/src/duskpsp/time/timepsp.c index 04045fbc..599b6d19 100644 --- a/src/duskpsp/time/timepsp.c +++ b/src/duskpsp/time/timepsp.c @@ -29,12 +29,10 @@ double_t timeGetRealTimeZonePSP(void) { if(sceRtcGetCurrentTick(&utc_ticks) < 0) return 0.0; if(sceRtcConvertUtcToLocalTime(&utc_ticks, &local_ticks) < 0) return 0.0; - /* - Return timezone offset in hours. - Example: - UTC-6 => -6.0 - UTC+2 => 2.0 - */ + // Return timezone offset in seconds, matching every other platform's + // timeGetRealTimeZoneXxx() -- timeEpochInit() adds this directly to a + // Unix-epoch-seconds timestamp (see timeepoch.c). + // Example: UTC-6 => -21600.0, UTC+2 => 7200.0 int64_t offset_us = (int64_t)local_ticks - (int64_t)utc_ticks; - return (double_t)offset_us / (1000000.0 * 60.0 * 60.0); + return (double_t)offset_us / 1000000.0; } \ No newline at end of file diff --git a/test/display/test_color.c b/test/display/test_color.c index 9037e302..264d8d86 100644 --- a/test/display/test_color.c +++ b/test/display/test_color.c @@ -75,6 +75,38 @@ static void test_colorHex_create(void **state) { assert_int_equal(color.a, COLOR_WHITE.a); } +// Exercises the color.csv-generated macros directly (COLOR__3F/_4F +// in particular) -- whole-number CSV channels like black/white's 0/1 once +// produced invalid C float literals ("0f"/"1f") in these, so referencing +// them here means a regression fails to compile instead of staying dead +// code no one notices. +static void test_generatedColor_wholeNumberChannels(void **state) { + color3f_t black3f = COLOR_BLACK_3F; + assert_float_equal(black3f.r, 0.0f, 0.0001f); + assert_float_equal(black3f.g, 0.0f, 0.0001f); + assert_float_equal(black3f.b, 0.0f, 0.0001f); + + color4f_t white4f = COLOR_WHITE_4F; + assert_float_equal(white4f.r, 1.0f, 0.0001f); + assert_float_equal(white4f.g, 1.0f, 0.0001f); + assert_float_equal(white4f.b, 1.0f, 0.0001f); + assert_float_equal(white4f.a, 1.0f, 0.0001f); + + color3b_t red3b = COLOR_RED_3B; + assert_int_equal(red3b.r, 255); + assert_int_equal(red3b.g, 0); + assert_int_equal(red3b.b, 0); +} + +// A fractional channel (gray = 0.5) should generate the same way whole +// numbers do -- confirms the float()-based fix didn't just special-case 0/1. +static void test_generatedColor_fractionalChannel(void **state) { + color3f_t gray3f = COLOR_GRAY_3F; + assert_float_equal(gray3f.r, 0.5f, 0.0001f); + assert_float_equal(gray3f.g, 0.5f, 0.0001f); + assert_float_equal(gray3f.b, 0.5f, 0.0001f); +} + int main(int argc, char **argv) { const struct CMUnitTest tests[] = { cmocka_unit_test(test_color3f_create), @@ -83,6 +115,8 @@ int main(int argc, char **argv) { cmocka_unit_test(test_color4b_create), cmocka_unit_test(test_color_create), cmocka_unit_test(test_colorHex_create), + cmocka_unit_test(test_generatedColor_wholeNumberChannels), + cmocka_unit_test(test_generatedColor_fractionalChannel), }; return cmocka_run_group_tests(tests, NULL, NULL); diff --git a/test/thread/test_thread.c b/test/thread/test_thread.c index 585ab348..ead4330d 100644 --- a/test/thread/test_thread.c +++ b/test/thread/test_thread.c @@ -69,19 +69,20 @@ static void test_thread_data(void **state) { } static void test_thread_restart(void **state) { - // A thread can be started, stopped, and started again. + // A thread can be started, stopped, and started again on the same + // thread_t -- without re-calling threadInit() -- because threadId is + // reset under stateMutex before threadHandler() signals STOPPED, so + // threadStop() returning guarantees threadStartRequest()'s "thread id + // not 0" assert won't race against it. Looping a few times gives any + // regression of that ordering repeated chances to hit the window. thread_t thread; threadInit(&thread, helper_noop); - threadStart(&thread); - threadStop(&thread); - assert_int_equal(thread.state, THREAD_STATE_STOPPED); - - // Re-initialise so threadId / state are reset, then start again. - threadInit(&thread, helper_noop); - threadStart(&thread); - threadStop(&thread); - assert_int_equal(thread.state, THREAD_STATE_STOPPED); + for(int32_t i = 0; i < 20; i++) { + threadStart(&thread); + threadStop(&thread); + assert_int_equal(thread.state, THREAD_STATE_STOPPED); + } } // --- threadmutex_t tests --- diff --git a/tools/color.py b/tools/color.py index 39c7f47a..0525d037 100644 --- a/tools/color.py +++ b/tools/color.py @@ -48,6 +48,11 @@ out = [ js = [] for name, (r, g, b, a) in colors.items(): r8, g8, b8, a8 = (int(float(ch) * 255) for ch in (r, g, b, a)) + # Route through float() rather than the raw CSV string -- a whole-number + # channel like "0" or "1" has no decimal point, and "0f"/"1f" aren't + # valid C float literals (float() always stringifies with one, e.g. + # "0.0"/"1.0"). + rf, gf, bf, af = (float(ch) for ch in (r, g, b, a)) macro = "COLOR_" + name.upper() camel = "".join(p[0].upper() + p[1:].lower() for p in name.split("_")) @@ -55,8 +60,8 @@ for name, (r, g, b, a) in colors.items(): f"// {name}", f"#define {macro}_4B color4b({r8}, {g8}, {b8}, {a8})", f"#define {macro}_3B color3b({r8}, {g8}, {b8})", - f"#define {macro}_3F color3f({r}f, {g}f, {b}f)", - f"#define {macro}_4F color4f({r}f, {g}f, {b}f, {a}f)", + f"#define {macro}_3F color3f({rf}f, {gf}f, {bf}f)", + f"#define {macro}_4F color4f({rf}f, {gf}f, {bf}f, {af}f)", f"#define {macro} {macro}_4B", "", ]