Device-os: SOS 14 flashes on Device OS v1.2.1-rc.3 with PietteTech_DHT library

[rickschrader]

Bug Report

Expected Behavior

Calling DHT.acquireAndWait() from the PietteTech_DHT library should function as expected (and as it did on prior Device OS versions).

Observed Behavior

DHT.acquireAndWait() is causing issues on an Argon and a Xenon with Device OS v1.2.1-rc.3. I was able to reproduce the issue even with the DHT_simple example from the library. Whenever this line is executed, the device begins an SOS red LED sequence with 14 flashes following the SOS, which should be indicating a semaphore lock timeout.

Steps to Reproduce

Update to Device OS v1.2.1-rc.3 on an Argon or Xenon device. Flash the DHT_simple example from the PietteTech_DHT library. Reset the device. Wait for the device to connect and execute the acquireAndWait() line.
The library has no issues on v1.1.0-rc.2, nor with versions prior to that. I have other devices running the same firmware successfully on those older Device OS versions. Because of this, it does not appear to be an issue with the library.

Test App

DHT_simple example from PietteTech_DHT library

References

https://community.particle.io/t/piettetech-dht-causing-sos-14-flashes-on-v1-2-1-rc-3/50583/2

[avtolstoy]

Thanks for reporting the issue. We've added additional safety checks in 1.2.x line (https://github.com/particle-iot/device-os/pull/1761) to ensure that no heap allocations or frees happen from an ISR and panic code 14 now has a more generic meaning: HeapError.

detachInterrupt() when called from an ISR will attempt to free potentially allocated std::function-based handler, causing this particular panic: https://github.com/particle-iot/device-os/blob/develop/wiring/src/spark_wiring_interrupts.cpp#L116

I suggest removing detachInterrupt() calls from the ISR handler and doing that outside of the interrupt context instead. Going forward we will most likely allow detachInterrupt() to be used for raw (non-std::function-based) handlers from an ISR, but this will require changes in the library as well: https://github.com/eliteio/PietteTech_DHT/blob/master/src/PietteTech_DHT.h#L105 will need to become static. cc @ScruffR

[ScruffR]

That may prove difficult as it would mean you couldn't have multiple instances of that class for independent sensors.
For a lambda function I'd understand why detachInterrupt() may need to free the function, but as long the object maintaining the non-static function doesn't get freed I'd not see a need nor the authority of detachInterrupt() to free that function.
IMO this would also render the existence of attachInterrupt(pin, &class::method, object, trigger) greatly useless as you'd never be able to use non-static class members inside that ISR.

What am I missing? :confused:

Can we have a ISR save version of detachInterrupt()?
I can imagine multiple cases where detachInterrupt() will logically be best placed in the ISR and no fitting place to do it otherwise makes sense (e.g. ParticleSoftSerial TX interrupt after the last byte has been clocked out)

[avtolstoy]

That may prove difficult as it would mean you couldn't have multiple instances of that class for independent sensors.

The class itself can have some pin-to-instance based mapping for example. Alternatively as I've mentioned, detachInterrupt() may simply be moved out of the interrupt context in PietteTech_DHT.

For a lambda function I'd understand why detachInterrupt() may need to free the function, but as long the object maintaining the non-static function doesn't get freed I'd not see a need nor the authority of detachInterrupt() to free that function.

The function itself is not freed. Whenever you pass something that isn't a non-capturing lambda or a function matching raw_interrupt_handler_t, attachInterrupt() allocates an internal std::function that holds all of that extra info (e.g. lambda itself with capture variables or instance pointer + member function pointer for class methods).

Can we have a ISR save version of detachInterrupt()?

Potentially, yes, however the current implementation relies on heap in both attachInterrupt() and detachInterrupt() for non-trivial handlers and will require some overhaul. We can easily fix the current detachInterrupt() issue for raw_interrupt_handler_t cases though.