clean up pwn and analog

[hathach]

• replace built-in atomic with freeRTOS critical functions
• mostly clean up and (re)move thing around

It may take a couple more follow-up PR, but I will switch to other task. Therefore this one is good for a review.

[hathach]

Thank you very much for your review,

1. I strongly recommend against removal of the atomic operation. The existing code works, while yours changes add bugs and make the code more complex.

There should be no race condition, the mutex is ensured by critical API(). There is nothing could take the control of CPU between Enter/Exit critical except for BLE softdevice since it has highest priority, higher than kernel's one.

2. If the serial port buffer is full, and noTone() is called from an ISR, what result? (will it hang waiting for space in serial output buffer?)

I have attempted to comment on each change that appeared to cause bugs.

User must not call tone() or noTone() within ISR. Like my current BLE API, anything API got called within ISR will mostly like to hanged. There is way to defer function to thread-context.

[henrygab]

There should be no race condition, the mutex is ensured by critical API(). There is nothing could take the control of CPU between Enter/Exit critical except for BLE softdevice since it has highest priority, higher than kernel's one.

Yes, there is a race condition. I give details above. READ IT.

User must not call tone() or noTone() within ISR. Like my current BLE API, anything API got called within ISR will mostly like to hanged. There is way to defer function to thread-context.

The BSP itself called noTone() from its own ISR. Therefore, user sketches were allowed to do so. In other words, your own code is what made it OK for sketches to call noTone() from ISR. You will now break stuff.

Question

Why are you changing working, bug-free code, after telling me so many times that you only make changes when someone complains of a bug?

Summary

• [ ] I strongly recommend against the removal of the atomic operation.
• [ ] I recommend against making it illegal to call noTone() from ISR.

I cannot spend hours trying to teach you anymore. I have tried my best to warn you away. If you resolve the issues because you cannot see the issue, I will not stop you anymore.

[hathach]

@henrygab thanks again for more review, please check out the taskENTER_CRITICAL() to see how it provide the mutex, the C++ atomic in the other way may not provide the same level of race condition guarding in efficient way. As mentioned multiple time before I suspect it uses cspie which will also disable the Softdevice task that handles timing-sensitive radio event. I would be happy if you could tell me more on the implementation of the C++ atomic.

For noTone(), the previous code is not mine, and it is now obsolete. The API should not be called within ISR like lots of others API within the core, I don't see why this should be an exception.

Update: update the code to fix possible race condition, compare before assign

[hathach]

Why are you changing working, bug-free code, after telling me so many times that you only make changes when someone complains of a bug?

Yeah, I only want to add code when people is complaining, though I would like to maintain an clean code that I could easily read and understand as well.

[hathach]

I still strongly recommend against removal of the use of the atomic. You say you don't understand it's use.... but you can use a search engine.

"raymond chen compare_exchange_strong"

Regardless, it's cleaner because it's OS-independent. It never disables interrupts, and it has no effect on the softdevice. Your code does.

Ah thanks, I just google to that article here https://devblogs.microsoft.com/oldnewthing/20180329-00/?p=98375 . This discussion also explain how it is interrupt-safe as well https://community.arm.com/developer/ip-products/processors/f/cortex-m-forum/10361/ldrex-strex-on-the-m3-m4-m7 . It looks like the atomic implementation uses the LDREX/STREX on ARM cortex which is indeed 100% fit for this ownership purpose. You should really point me to the article earlier, I am so skeptical of the implementation. On other platform there may not be an equivalent instruction and may ended up disable global interrupt. I should have done some research instead of just simply asking though (in my defense it is hard to get over the laziness :smile: ). Thank you very much for your passion, I have learned something new today :100:

[hathach]

@henrygab I am persuaded by your persistence with the takeOwnership/releaseOwnership regarding the ISR. They should really not be called within ISR, any user that write that should really need to rewrite the sketch. Normally I would just either ignore that or just simply drop the LOG message. However, since these API are new and there are potentially issue with the implementation, having the debug log is indeed useful. Thank you for your review so far.

[hathach]

thank you very much for the in-depth review, I am glad that you are pushing me to learn new thing. I will be using the C11 atomic for cortex M mcu from now when possible. On other platform there maybe no equivalent of instruction, therefore an appropriate research should be done for each platform. But it is hard to find an MCU that is not ARM nowsaday.

[dhalbert]

Sadly these are not available on the M0.

[henrygab]

Really? I thought that they were defined for M0 as something similar to:

click to expand / collapse code

```C int __atomic_fetch_add_4(int *d, int val, int mem) { uint32_t primask = __get_PRIMASK(); __disable_irq(); *d += val; __set_PRIMASK(primask); return *d; } int __atomic_fetch_sub_4(int *d, int val, int mem) { uint32_t primask = __get_PRIMASK(); __disable_irq(); *d -= val; __set_PRIMASK(primask); return *d; } ```

Granted, this disables interrupts for a few instructions ... but are there synchronization primitives that don't disable interrupts on M0?

Or, were you saying that the LDREX/STREX instructions were not available on M0?

If the lack of those instructions is a concern, then unless there's the wierdness that nrf52 imposes via the softdevice, would the above read-modify-write with interrupts disabled be problematic on an M0? (obviously, the question is in comparison with a mutex or other synchronization primitive, each of which would appear to need to ensure an atomic compare-and-swap....)

[dhalbert]

Or, were you saying that the LDREX/STREX instructions were not available on M0?

That's what's missing.

The SoftDevice has its own primitives, which don't block interrupts levels used by the SoftDevice, but block all other levels. It also provides mutexes.

__STATIC_INLINE uint32_t sd_nvic_critical_region_enter(uint8_t * p_is_nested_critical_region)
{
  int was_masked = __sd_nvic_irq_disable();
  if (!nrf_nvic_state.__cr_flag)
  {
    nrf_nvic_state.__cr_flag = 1;
    nrf_nvic_state.__irq_masks[0] = ( NVIC->ICER[0] & __NRF_NVIC_APP_IRQS_0 );
    NVIC->ICER[0] = __NRF_NVIC_APP_IRQS_0;
    nrf_nvic_state.__irq_masks[1] = ( NVIC->ICER[1] & __NRF_NVIC_APP_IRQS_1 );
    NVIC->ICER[1] = __NRF_NVIC_APP_IRQS_1;
    *p_is_nested_critical_region = 0;
  }
  else
  {
    *p_is_nested_critical_region = 1;
  }
  if (!was_masked)
  {
    __sd_nvic_irq_enable();
  }
  return NRF_SUCCESS;
}

__STATIC_INLINE uint32_t sd_nvic_critical_region_exit(uint8_t is_nested_critical_region)
{
  if (nrf_nvic_state.__cr_flag && (is_nested_critical_region == 0))
  {
    int was_masked = __sd_nvic_irq_disable();
    NVIC->ISER[0] = nrf_nvic_state.__irq_masks[0];
    NVIC->ISER[1] = nrf_nvic_state.__irq_masks[1];
    nrf_nvic_state.__cr_flag = 0;
    if (!was_masked)
    {
      __sd_nvic_irq_enable();
    }
  }

  return NRF_SUCCESS;
}

[hathach]

yeah, the sd_nvic_critical_region_enter/sd_nvic_critical_region_exit is equivalent to freeRTOS crtiical enter and exit, it block interrupt except one used by Softdevice and pause the scheduler. Softdevice interrupt should be never blocked since yeah, it will cause ble disconnection in most case. I am still amazed with LDREX/STREX instruction on M3/M4 after working with the mcu for several years :), great feature.

If the lack of those instructions is a concern, then unless there's the wierdness that nrf52 imposes via the softdevice, would the above read-modify-write with interrupts disabled be problematic on an M0? (obviously, the question is in comparison with a mutex or other synchronization primitive, each of which would appear to need to ensure an atomic compare-and-swap...

nRF52 is M4 therefore the LDREX/STREX is supported, there is no need to worry about. @dhalbert is talking on a more generic critical section approach which he like to use on the circuitpython project which involved M0 SAMD21. So your atomic is perfect for this repo as it is. Thank you again.