search

pstolarz / OneWireNg

Arduino 1-wire service library. OneWire compatible. Dallas thermometers support.
BSD 2-Clause "Simplified" License
85 stars 19 forks source link

Bug? Continuous crashing on ESP8266 (not on ESP32) when OneWire::begin(pin) tries to run #15

Closed ba58smith closed 3 years ago

ba58smith commented 3 years ago

Working with https://github.com/SignalK/SensESP, which used the https://github.com/PaulStoffregen/OneWire library for a couple of years without issue. Recently switched to pstolarz/OneWireNg and pstolarz/Arduino-Temperature-Control-Library, and now several users are getting continuous crashes when using it with ESP8266, but not with ESP32. I've traced it down to somewhere inside the begin(pin) method.

The first line of the error from PlatformIO (VS Code) says Exception Cause: 9 [LoadStoreAlignmentCause: Load or store to an unaligned address]. I noticed that the following line in the begin(pin) method is trying to store the new OneWireNg_CurrentPlatform in a specific spot in memory: _ow = new (_ow_buf) OneWireNg_CurrentPlatform

I removed the (_ow_buf) from that line, and the crash stopped. Unfortunately, I'm away from the rest of my equipment that consumes the data from this program, so I can't test it all the way through. But could this be the cause of the crash?

pstolarz commented 3 years ago

Thanks for pointing this out. It looks like memory alignment issue. Could you check the following 2 alternatives and let me know about results (if it crashes or no):

  1. Relplace lines 157-160 in OneWire.h

    private:
bool _srch_done;
uint8_t _ow_buf[sizeof(OneWireNg_CurrentPlatform)];
OneWireNg *_ow;

    to

    private:
uint8_t _ow_buf[sizeof(OneWireNg_CurrentPlatform)];
bool _srch_done;
OneWireNg *_ow;

  2. Similar to previous with with alignas usage.

    private:
alignas(size_t) uint8_t _ow_buf[sizeof(OneWireNg_CurrentPlatform)];
bool _srch_done;
OneWireNg *_ow;
ba58smith commented 3 years ago

I changed line 69 back to _ow = new (_ow_buf) OneWireNg_CurrentPlatform(, and then tried each of your two alternatives. Each of them worked.

pstolarz commented 3 years ago

Cool, I'll push new version with the 2nd alt. (as more generic one) to fix this issue. Thanks for reporting this problem.

ba58smith commented 3 years ago

Happy to help! Thanks for your quick response and updated version.

pstolarz commented 3 years ago

Thanks for using the lib and constructive feedback.

pstolarz commented 3 years ago

Fixed by #16 (already merged to master).

mairas commented 3 years ago

Yay, it's great to see that this bug got unearthed and fixed! It's been a showstopper for SensESP ESP8266 users for quite some time but I just haven't had the time to investigate...

If I may make a kind request - could you make a new release of OneWireNg now that this issue has been fixed? TIA!

pstolarz commented 3 years ago

@mairas Do you confirm the resent fix works on your setups?

mairas commented 3 years ago

Tried with ESP8266, that works now. But ESP32 is broken due to the recent ESP32-C3 commit. I made a separate issue for that: #17.

I'm digressing here, but that ESP32-C3 is actually a pretty great little chip. The cheapest MCU with an integrated CAN controller you can get... I'm planning on using the bare chip in some projects. Sure, you'll need an oscillator and a flash memory, but still some pretty great peripherals at very little cost.