PaulStoffregen / OneWire

Library for Dallas/Maxim 1-Wire Chips
http://www.pjrc.com/teensy/td_libs_OneWire.html
579 stars 382 forks source link

merge stickbreaker's ESP32 fixes #93

Closed garyd9 closed 6 months ago

garyd9 commented 3 years ago

cleanly merge ESP32 fixes as authored here: https://github.com/stickbreaker/OneWire (with minor style changes.) This should resolve issue #90

Please feel free to change "CRIT_TIMING" to something better. I tried to make it some text that generically identifies it's purpose.

blazoncek commented 1 year ago

I am not sure about redefining Interrupts()/noInterrupts() but IRAM_ATTR does the trick for me. And the solution provided here seems clean and nonbreaking.

I am in favor of this PR.

softhack007 commented 1 year ago

With this PR merged, I get compiler warnings due to the #undef lines at the end of OneWire.cpp:

.pio/libdeps/esp32_4MB_M_debug/OneWire/OneWire.cpp:599:22: warning: extra tokens at end of #undef directive
 #  undef noInterrupts() {portMUX_TYPE mux = portMUX_INITIALIZER_UNLOCKED;portENTER_CRITICAL(&mux)
                      ^
.pio/libdeps/esp32_4MB_M_debug/OneWire/OneWire.cpp:600:20: warning: extra tokens at end of #undef directive
 #  undef interrupts() portEXIT_CRITICAL(&mux);}
                    ^

I think these #undef are not necessary, as its the last lines of the .cpp file so the only use case would be that someone does include <OneWire.cpp> in his own source code, which seems a very unlikely case.

softhack007 commented 1 year ago

As a matter of coding style, I would suggest to rename the macros noInterrupts and interrupts to something that does not clash with standard arduino framework functions.

For example:

#ifdef ARDUINO_ARCH_ESP32
// due to the dual core esp32, a critical section works better than disabling interrupts
#  define START_MY_CRIT_SECTION() {  static portMUX_TYPE mux = portMUX_INITIALIZER_UNLOCKED; portENTER_CRITICAL(&mux) 
#  define END_MY_CRIT_SECTION()      portEXIT_CRITICAL(&mux); }
// for info on this, search "IRAM_ATTR" at https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/general-notes.html 
#  define CRIT_TIMING IRAM_ATTR
#else
#  define START_MY_CRIT_SECTION() noInterrupts()
#  define END_MY_CRIT_SECTION() interrupts()
#  define CRIT_TIMING 
#endif

And then slightly refactor the code to use these macros instead of noInterrupts() / interrupts().