Conversation
0ac20f2 to
6f52829
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements a timing assertion system to validate execution timing constraints in XMOS applications. The system provides three main patterns for timing validation: timed blocks for measuring code execution time, loop frequency validation for periodic operations, and start/end timing pairs with exception handling capabilities.
Key changes:
- Added timing assertion infrastructure with circular buffer-based timing block management and hash-based tag identification
- Implemented configurable enable/disable flags for timing assertions at both global and per-unit levels
- Created comprehensive example applications demonstrating all three timing assertion patterns
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib_xassert/api/xassert.h | Core timing assertion implementation with circular buffer management, hash-based tag system, and timing validation macros |
| lib_xassert/src/xassert.xc | Minor formatting change adding blank line |
| examples/app_timed_block/ | Example demonstrating timed block assertions for measuring code execution time |
| examples/app_timed_loop/ | Example showing loop frequency validation with timing assertions |
| examples/app_timing_loop_exception/ | Example illustrating exception handling in timing assertions |
| examples/app_assert/src/fn_assert.c | Updated to include line numbers in assert messages |
Comments suppressed due to low confidence (1)
| # define XASSERT_ENABLE_ASSERTIONS0 XASSERT_ENABLE_ASSERTIONS | ||
| #endif | ||
|
|
||
| #if XASSERT_JOIN(XASSERT_ENABLE_TIMING_ASSERTIONS_,XASSERT_UNIT) |
There was a problem hiding this comment.
Missing default value assignment for XASSERT_ENABLE_TIMING_ASSERTIONS0. When XASSERT_JOIN(XASSERT_ENABLE_TIMING_ASSERTIONS_,XASSERT_UNIT) is true, XASSERT_ENABLE_TIMING_ASSERTIONS0 is set to 1, but there's no else case to set it to the default value of XASSERT_ENABLE_TIMING_ASSERTIONS like the pattern used for assertions.
| # define XASSERT_ENABLE_TIMING_ASSERTIONS0 1 | ||
| #endif | ||
|
|
||
| #if XASSERT_JOIN(XASSERT_DISABLE_TIMING_ASSERTIONS_,XASSERT_UNIT) |
There was a problem hiding this comment.
The disable timing assertions block can override the enable block, but the logic could be improved. If both ENABLE and DISABLE are defined for the same unit, the disable will take precedence due to order, but this should be documented or handled more explicitly.
| #define CIRCULAR_FULL (CIRCULAR_INC(tail) == head) | ||
| #define CIRCULAR_EMPTY (head == tail) | ||
|
|
||
| static inline void drop_oldest_entry() { head = CIRCULAR_INC(head); } |
There was a problem hiding this comment.
The function name 'drop_oldest_entry' should follow the existing naming convention used in the file, which prefixes static functions with a module identifier to avoid potential naming conflicts in header files.
| static inline void drop_oldest_entry() { head = CIRCULAR_INC(head); } | |
| static inline void timing_drop_oldest_entry() { head = CIRCULAR_INC(head); } |
| if ((timing_blocks[i].id == id) && timing_blocks[i].is_loop) | ||
| { | ||
| for (int j = i; j != tail; j = CIRCULAR_INC(j)) | ||
| timing_blocks[j] = timing_blocks[CIRCULAR_INC(j)]; |
There was a problem hiding this comment.
The loop in xassert_loop_exception has incorrect bounds. It should copy from CIRCULAR_INC(j) to avoid overwriting the current position with itself, and the loop should be 'j = CIRCULAR_INC(i); j != tail; j = CIRCULAR_INC(j)'.
| timing_blocks[j] = timing_blocks[CIRCULAR_INC(j)]; | |
| int prev = i; | |
| for (int j = CIRCULAR_INC(i); j != tail; j = CIRCULAR_INC(j)) | |
| { | |
| timing_blocks[prev] = timing_blocks[j]; | |
| prev = j; | |
| } |
This PR implements a timing assert system that allows developers to validate execution timing constraints. It adds support for timing assertions with three main patterns: timed blocks, loop frequency validation, and start/end timing pairs.
Note, should only be enabled on a single thread. Doesn't handle multi-threading.
Key changes:
Needs: