Add a C re-entrant critical section API #18
Conversation
| static volatile uint32_t interruptEnableCounter = 0; | ||
| void critical_section_enter() | ||
| { | ||
| __disable_irq(); |
There was a problem hiding this comment.
If the interrupts are disabled and you call critical_section_enter and then critical_section_exit, it'll enable interrupts. Wouldn't it be better to save/restore interrupt state?
There was a problem hiding this comment.
Don't disable interrupts, call critical_section_enter.
There was a problem hiding this comment.
I won't disable interrupts, but library code (for example) might not care if it runs with interrupts enabled or not, but still re-enable interrupts by mistake if it uses these functions.
There was a problem hiding this comment.
That said, I could preserve the primask if critical_section_enter is called and interruptEnableCounter is 0.
| /* not allowed to overflow the interruptEnableCounter. */ | ||
| if (interruptEnableCounter == UINT32_MAX) { | ||
| /* Generate a fault */ | ||
| *((uint32_t *)NULL) = 0; |
There was a problem hiding this comment.
is this guaranteed to fault for all architectures including the ones we might support? Is address 0 always going to remain unmodifiable? perhaps we need to use an abstraction here. die()?
There was a problem hiding this comment.
Well, it was that or an unusable opcode.
There was a problem hiding this comment.
Good spot @rgrover ... it is legal (AFAIK, but definitely not recommended) to put RAM at 0x0, in which case this would set the initial SP to an invalid value (and cause the next reset exception to escalate to a fault of some kind... hard fault?).
|
Comments? |
|
The nRF5x platform needs a soft-device specific critical section API; __disable_irq() is not an option. I believe critical section and atomic-ops may need to be target-specific. |
|
@rgrover Is that problem duplicated on any other platforms? It might be more appropriate to add a linker override to the nRF5x platform, rather than making all atomic ops target-specific. On most platforms, they will be generic operations. |
|
currently only the nRF5x requires platform-dependent atomics; but the same would be true for the forthcoming POSIX platform. Perhaps @hugovincent can share some thoughts. I believe we shouldn't base critical-section on disabling interrupts. |
|
Should we introduce a platform_enter_critical(&state) [https://github.com/ARMmbed/mbed-6lowpan-eventloop-adaptor/blob/master/source/events.cpp#L32] to handle platform specific implementations? |
|
@rgrover, that seems to be the way to go, yes. |
|
Is this PR still relevant? |
|
It is, but I'd say not for our feature freeze. We can deal with it later. |
| /* Generate a fault */ | ||
| ((void(*)(void))&invalid)(); | ||
| } | ||
| interruptEnableCounter++; |
There was a problem hiding this comment.
This should probably use atomic_incr
|
We should check for a race between |
| #include "mbed-util/critical.h" | ||
|
|
||
| static volatile uint32_t interruptEnableCounter = 0; | ||
| static volatile uint32_t critical_primask = 0; |
There was a problem hiding this comment.
This variable is not required.
|
I believe this one can be closed, resolved in #88 |
Implements the API discussed in #16