search

eliteio / PietteTech_DHT

DHT Sensor Library for Spark Core
Other
9 stars 6 forks source link

Don't call detachInterrupt() inside ISR since it internally calls heap functions #2

Closed rgiese closed 4 years ago

rgiese commented 5 years ago

Resolves #1 (DeviceOS 1.2.1-rc.3 safety checks break library), also tracked under https://github.com/particle-iot/device-os/issues/1835.

I've created a flag _detachISR that the ISR code can use to request that main thread code detach the ISR. I'm not thrilled with how I baked in the check for _detachISR (in DHT_CHECK_STATE and getStatus) but I think this just creates the least amount of churn.

For completeness' sake it may be worth investing in a destructor that calls detachISRIfRequested to make it feel more complete, though I bet that nobody in real life actually has a transient object for any of this...

Regardless, happy to make any changes you'd like to see.

Sniff-tested on Photon with DeviceOS 1.2.1.

ScruffR commented 5 years ago

I'd like to see how the underlying bug will be fixed.

The original implementation made sure the ISR stays hooked up as short as absolutely needed, I'm not (yet) sure how the extended "lifetime" of the ISR would impact existing code. AFAICT, with this the interrupt gets detached only when explicitly calling detachISRIfRequested() (an entirely new function) or getStatus() (which isn't compulsory for acquire() - acquireAndWait() calls it when done, which is good).

rgiese commented 5 years ago

Fair enough, it's a bit hard to see unless you fully expand the delta.

If the user is calling acquireAndWait(), then it's taken care of (as you said) by virtue of acquireAndWait() calling getStatus() - that was the closest I felt I could magic this into, though calling it explicitly from within acquireAndWait might make this clearer.

If the user is calling acquire(), they'll eventually call getCelsius() and friends, which invoke DHT_CHECK_STATE, which I expanded to also call detachISRIfRequested() - the closest magic for this case I could come up with that felt reliable (whereas, conversely, someone might skip calling acquiring() after an acquire() in favor of just running a delay, so I didn't put it into acquiring()).

I'd like to think this is a temporary fix because I think the Particle SDK's choice to make it impossible to disconnect future interrupts inside an ISR is quite unwise and should be remedied...

Hope that helps.

ScruffR commented 4 years ago

I give in 😉 Since the underlying https://github.com/particle-iot/device-os/issues/1835 doesn't seem to be fixed anytime soon I have incorporated the workaround and wrapped it in #if (SYSTEM_VERSION < SYSTEM_VERSION_v121RC3) ... #else ... #endif to cater for buggy device OS and "bug-free" the appropriate way.