Nanostack hal timer shortcut #6627
Conversation
|
This PR is something between a fix and feature, as it improves a existing feature. Discussed already about this with @kjbracey-arm |
| @@ -12,15 +12,25 @@ | |||
|
|
|||
| static Timer timer; | |||
| static Timeout timeout; | |||
|
|
|||
| // High priority event queue is needed if the timer callback is not interrupt safe or the | |||
| // critical section implementation is not disabling interrupts. | |||
There was a problem hiding this comment.
Phrasing is a bit wonky above - this isn't an "or". If the critical section is not disabling interrupts, then it must be claiming a mutex, which means the timer callback is not interrupt safe, and any other code doing critical sections won't be protected against a called-from-interrupt timer.
Maybe something like
// If critical sections are implemented using mutexes, timers must be called in thread context, and
// we use the high-priority event queue for this.
// If critical sections disable interrupts, we can call timers directly from interrupt. Avoiding the
// event queue can save ~1600B of RAM if the rest of the system is not using the event queue either.
// Caveats of this tunable are listed on arm_hal_interrupt.c.
There was a problem hiding this comment.
Your version is much better, changed to it and force pushed.
|
@kjbracey-arm if its between, should it rather be feature and 5.9 ? Although looking at the changes, it does not change anything by default, just provides new configuration. |
|
It's not adding a new option, it's further optimising what happens when that option is on. Option was new in 5.8, and Tero's bunch are probably the only current users. Saves memory, but with a bunch of caveats. It's not a fix, it is an effective change to behaviour - this timer is now called from interrupt context, not thread context. That matches the intent of the option and the stated caveats, and /should/ be fine for correctly-written Nanostack code that only does I'd call it a refactor - won't actually fix any visible bugs, but will save memory, with the usual slim chance of app breakage. App breakage that could only occur with the option on, mind. |
nanostack-hal.critical-section-usable-from-interrupt -tunable was previously added to optionally make critical section code interrupt safe. The IRQ safe critical section is a prequisite for interrupt safe timer callbacks. The same flag can be used to enable calling of the timer callbacks directly from the timer interrupt context, without bouncing them via event thread. This removes the code and RAM consumed by EventQueue and the thread serving the high priority events. If the system does not have any dependencies on mbed_shared_queues, by setting this flag the static RAM usage is now further reduced by ~1600 bytes and code size by 4KB. Note: the default behavior is not changed, one needs to override the "nanostack-hal.critical-section-usable-from-interrupt" to have "true".
Compiler reminded that a variable declaration was left behind when the code using it was put behind #ifdef. Add the missing #ifdef. Warning being fixed: ---8<---8<---- [Warning] ns_event_loop.c@44,0: ARMmbed#177-D: variable "event_thread_id" was declared but never referenced
TeroJaasko
force-pushed
the
nanostack_hal_timer_shortcut
branch
from
4b157e0 to
a23b7cf
Compare
|
Still worth trying the "ticker" mode option, as discussed, which cuts this code out entirely. Not sure if that would actually save significant (or any) extra memory or not |
|
Looking at this, I'm nervous there's no error check on the |
|
MBED_ASSERT() seems to able to print the correct message even from interrupt. The assert(false) in timer_callback() seems to dump just a "Error - writing to a file in an ISR or critical section". See the trace below. I never realized they actually had functional difference. One learns something new every day. :) I'm also in favour of some kind of assert() as it would help in tuning the queue size to match the worst case load. But it might also be a bit harsh for users who are perfectly fine with lost timer interrupts. On the other hand, debugging a system with unexpectedly and silently lost timer callbacks is just horrible. Adding assertion there sounds to me same kind compatibility break as the recent "Mutex xxxx error -6. Not allowed in ISR context" trap which did point out valid bugs in existing code, but which also forced application to change. |
|
Yeah, You're right, this could well break applications silently getting away with it, so maybe don't do it now. I was thinking the primary user was the tick generation, which if it failed would kill the event loop forever, so would have already been noticed, but there are other high-res timer users. (Not in your system, but with Nanostack). |
|
/morph build |
Build : SUCCESSBuild number : 1754 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 1391 |
Test : FAILUREBuild number : 1560 |
|
Echo test completed too fast, apparently. Relaunching. |
Test : SUCCESSBuild number : 1568 |
Description
nanostack-hal: timer: conditionalize the use of high pri event thread
nanostack-hal.critical-section-usable-from-interrupt -tunable was
previously added to optionally make critical section code interrupt safe.
The IRQ safe critical section is a prequisite for interrupt safe timer
callbacks.
The same flag can be used to enable calling of the timer callbacks
directly from the timer interrupt context, without bouncing them via
event thread. This removes the code and RAM consumed by EventQueue
and the thread serving the high priority events.
If the system does not have any dependencies on mbed_shared_queues,
by setting this flag the static RAM usage is now further reduced
by ~1600 bytes and code size by 4KB.
Note: the default behavior is not changed, one needs to override the
"nanostack-hal.critical-section-usable-from-interrupt" to have "true".
Pull request type
[x ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change