search

ARMmbed / core-util

DEPRECATED: Mbed 3 utilities library
Other
12 stars 17 forks source link

Fix CriticalSection lock for nrf51 targets. #77

Closed pan- closed 8 years ago

pan- commented 8 years ago

The previous implementation of CriticalSectionLock for nrf51 was only relying to softdevice primitives sd_nvic_critical_region_enter and sd_nvic_critical_region_exit . If the soft device is not enabled, these functions have no effect. This means that CriticalSectionLock was not locking anything if the softdevice was not enabled.

This fix allow the usage of CriticalSectionLock, even if the softdevice is not enabled. If the softdevice is not enabled, all irq are disabled until the destruction of the lock.

This PR should be merged with: https://github.com/ARMmbed/mbed-hal-nrf51822-mcu/pull/48

pan- commented 8 years ago

@rgrover @0xc0170 @emidttun could you review this please ?

rgrover commented 8 years ago

+1

emidttun commented 8 years ago

Looks correct to me, but out of curiosity: Why isn't there an IRQ enable in the destructor? (It was like that before the change.)

0xc0170 commented 8 years ago

Looks correct to me, but out of curiosity: Why isn't there an IRQ enable in the destructor? (It was like that before the change.)

Which line are you referencing ? It restores primask, as it was before.

emidttun commented 8 years ago

@0xc0170 So the __disable_irq() only writes to PRIMASK? Wasn't aware of it; then no further comments.

0xc0170 commented 8 years ago

LGTM, however it is lot of code for for critical section :-)

cc @bogdanm

bogdanm commented 8 years ago

That does look like a lot of code, thus quite a few opportunities for race conditions. I'm going to trust @rgrover on this one, since I'm not that familiar with nRF51 targets. So, +1.