[tick, timeout] Add support for a 64 bit tick source - #53
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in 64-bit tick mode for the whal_Timeout abstraction so boards can use wider tick counters while preserving the existing 32-bit default behavior.
Changes:
- Introduces
WHAL_CFG_64BIT_TICKto switchwhal_Timeout’s tick fields andGetTickcallback betweenuint32_tanduint64_t. - Adds
WHAL_TICK_MAXand updatesWHAL_TIMEOUT_EXPIRED()to use the configured tick width. - Updates the timeout unit test to work in both 32-bit and 64-bit tick configurations (including wraparound behavior).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wolfHAL/timeout.h | Adds a build-time option for 64-bit tick sources and adjusts timeout calculations accordingly. |
| tests/core/test_timeout.c | Adapts fake tick source + wrap test to use the configured tick width (WHAL_TICK_MAX). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
wolfHAL/timeout.h:89
- The casts in WHAL_TIMEOUT_EXPIRED are currently applied to the result of the comparison (0/1) rather than to the tick subtraction. This makes the cast ineffective/misleading and defeats the intent of explicitly evaluating the elapsed tick delta at the configured width.
#ifdef WHAL_CFG_64BIT_TICK
#define WHAL_TIMEOUT_EXPIRED(t) \
((t) && ((uint64_t)((t)->GetTick() - (t)->startTick) >= (t)->timeoutTicks))
#else
#define WHAL_TIMEOUT_EXPIRED(t) \
((t) && ((uint32_t)((t)->GetTick() - (t)->startTick) >= (t)->timeoutTicks))
#endif
docs/writing_a_driver.md:342
- The 64-bit tick guidance should also mention atomicity: on 32-bit MCUs, reading/updating a 64-bit tick counter is often not atomic and can yield torn reads (non-monotonic GetTick results), which can break timeout behavior.
When the macro is defined, the board's tick source must match the width: its
`g_tick` counter and `GetTick` callback (typically `Board_GetTick`) must be
`uint64_t`, since `whal_Timeout.GetTick` becomes `uint64_t (*)(void)`. The
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #53
Scan targets checked: wolfhal-bugs, wolfhal-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security
Overall recommendation: COMMENT
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] WHAL_CFG_64BIT_TICK duplicates whal_Timeout and WHAL_TIMEOUT_EXPIRED per width and no in-tree board can enable it; a whal_Tick typedef fixes both —
wolfHAL/timeout.h:41-89 - [Medium] [review] 64-bit tick config is never exercised with a timeout that actually requires 64-bit arithmetic —
tests/core/test_timeout.c:43-46 - [Medium] [review] 64-bit tick callbacks require an atomic snapshot contract —
wolfHAL/timeout.h:72-85 - [Low] [review-security] Mixed-width builds silently corrupt whal_Timeout across library boundaries —
wolfHAL/timeout.h:47-57 - [Info] [review-security] CI matrix does not cover WHAL_CFG_64BIT_TICK combined with WHAL_CFG_NO_TIMEOUT —
.github/workflows/core-tests.yml:14
Review generated by Skoll
This PR adds optional support for a 64 bit tick source by setting the config macro
WHAL_CFG_64BIT_TICK.