-
Notifications
You must be signed in to change notification settings - Fork 151
Add BASEPRI zero-latency ISR support #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8869bb5 to
88de893
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 10 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="include/platform/irq.h">
<violation number="1" location="include/platform/irq.h:326">
P1: Unconditionally enabling interrupts with `cpsie i` may break critical sections. If this macro is called when interrupts are already disabled, it will incorrectly re-enable them. Use `irq_save_flags()`/`irq_restore_flags()` to preserve the previous interrupt state.</violation>
<violation number="2" location="include/platform/irq.h:333">
P1: Same issue as `irq_system_enter()`: unconditionally enabling interrupts may break critical sections. Use `irq_save_flags()`/`irq_restore_flags()` to preserve the previous interrupt state.</violation>
</file>
<file name="platform/irq-latency.c">
<violation number="1" location="platform/irq-latency.c:171">
P2: Data race: `min` and `max` are read outside the consistency loop, making the snapshot potentially inconsistent. These reads should be inside the do-while loop to ensure all fields are from the same state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cubic-dev-ai Analysis of the three issues:
irq_system_{enter,exit} macros:
You're correct that these macros unconditionally enable interrupts, which could be problematic. However, git grep confirms these macros are completely unused in the codebase - they're only defined but never called. These appear to be leftover diagnostic utilities. I'll remove them to eliminate the concern.
min/max outside consistency loop:
You're technically correct that min/max could be from a different snapshot than count/sum. However, this is a deliberate design choice:
- All reads are atomic (
__ATOMIC_RELAXED), so there's no memory model UB - The retry loop ensures count/sum consistency because
avg = sum/countdepends on their relationship - min/max are independent statistics that don't depend on count/sum
- This is diagnostic code (KDB display), not safety-critical
- The slight staleness (min/max potentially 1 sample newer/older) is acceptable for human consumption
That said, if you prefer perfect snapshot consistency, I can include min/max in the retry loop. The tradeoff is:
- Current approach: Simpler retry logic, slight potential staleness acceptable for diagnostics
- Including min/max: Perfect consistency, more complex retry condition (retry if count OR min OR max changed)
This implements BASEPRI-based critical sections to enable zero-latency ISRs (priority 0x0-0x2) that can preempt kernel operations. It adds cycle-accurate interrupt latency measurement infrastructure using DWT. - Add BASEPRI primitives (irq_kernel_critical_enter/exit) to irq.h - Convert scheduler critical sections from PRIMASK to BASEPRI - Convert IPC critical sections from PRIMASK to BASEPRI - Add TCB validation in IPC to prevent use-after-free during callbacks - Add irq_system_state tracking and in_isr_context() helper
88de893 to
61e5798
Compare
This implements BASEPRI-based critical sections to enable zero-latency ISRs (priority 0x0-0x2) that can preempt kernel operations. It adds cycle-accurate interrupt latency measurement infrastructure using DWT.