Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ License along with NeoPixel. If not, see
#pragma once

#if defined(ARDUINO_ARCH_ESP32)
#if !defined(WLED_USE_SHARED_RMT) && !defined(__riscv) && (ESP_IDF_VERSION_MAJOR >= 4) // WLEDMM don't compile this file on unsupported platforms

// Use the NeoEspRmtSpeed types from the driver-based implementation
#include <NeoPixelBus.h>
Expand Down Expand Up @@ -485,3 +486,4 @@ typedef NeoEsp32RmtHI7Ws2805InvertedMethod NeoEsp32RmtHI7Ws2814InvertedMethod;
#endif // !defined(CONFIG_IDF_TARGET_ESP32C3)

#endif
#endif
2 changes: 2 additions & 0 deletions lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

#if defined(__XTENSA__) && defined(ESP32) && !defined(CONFIG_BTDM_CTRL_HLI)
#if !defined(WLED_USE_SHARED_RMT) && !defined(__riscv) && defined(ARDUINO_ARCH_ESP32) && (ESP_IDF_VERSION_MAJOR >= 4) // WLEDMM don't compile this file on unsupported platforms
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is the cause of your problems: the high priority interrupt assembly layer is being disabled here. Neither ARDUINO_ARCH_ESP32 or ESP_IDF_VERSION_MAJOR are available as they haven't been included, only command-line defines are available at this point in the file. (Also !defined(__riscv) is implied by defined(__XTENSA__)).

(FWIW the !defined(CONFIG_BTDM_CTRL_HLI) check there above is also wrong -- it can never be defined at that point in the file since sdkconfig.h hasn't been included yet.)

This change is also unnecessary: the linker will discard the object file if it's never referenced by the application code. (This is the same approach used by all the other unused drivers in NPB.)

The error isn't caught at build time because the NeoESPRmtHIMethod.cpp code that asks for it to be linked is playing some games with __attribute__((used)) in a way that cues the linker to include the file, but at no actual code space or runtime cost. However, if the assembly file is missing, the link will still succeed as the symbol it calls for is never in fact truly used. I'd prioritized runtime performance over developer convenience - sorry about that!

Anyways my recommendation is revert all the changes to lib/NeoESP32RmtHI from this patch. If bus_wrapper.h never calls for any RMTHI instances, the linker will discard the .cpp and .S object files, so it ultimately won't affect any build products.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're curious as to the derivation of the symptoms:

  • Tasmota ESP32 (no suffix) cores include CONFIG_BTDM_CTRL_HLI, so the HI driver piggybacks on the Bluetooth driver's handler (as there's only one high priority interrupt level available). This is why it worked on basic ESP32s, but not -S2s and -S3s.
  • Without the contents of the .S file from the library, the linker falls back to the default highint handler (a no-op). So as soon as an RMT interrupt was generated, the core in question looped forever, as the interrupt would never be cleared.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is the cause of your problems: the high priority interrupt assembly layer is being disabled here. Neither ARDUINO_ARCH_ESP32 or ESP_IDF_VERSION_MAJOR are available as they haven't been included, only command-line defines are available at this point in the file. (Also !defined(__riscv) is implied by defined(__XTENSA__)).

wow, that's unexpected, but it explains the problems. I still have a problem with my "emergency fallback" esp32dev_compat V3 buildenv, because its still on NPB 2.7.5.

Compiling .pio\build\esp32dev_compat\src\wled00.ino.cpp.o
Linking .pio\build\esp32dev_compat\firmware.elf
.pio\build\esp32dev_compat\libafb\NeoESP32RmtHI\NeoEsp32RmtHI.S.o:(.iram1.literal+0x14): undefined reference to `NeoEsp32RmtMethodIsr'
.pio\build\esp32dev_compat\libafb\NeoESP32RmtHI\NeoEsp32RmtHI.S.o: In function `_highint_stack_switch':
(.iram1+0x92): undefined reference to `NeoEsp32RmtMethodIsr'
collect2.exe: error: ld returned 1 exit status

--> i'll do some more experimenting, most likely I can still protect compiling the .s file with #if !defined(WLED_USE_SHARED_RMT) because this is a command line define.

😅 means another round of testing, but at least we now have an explanation that makes sense 👍

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try lib_ignore = NeoESP32RmtHI in the environment def for those using older NPB? It might be cleaner.


#include <freertos/xtensa_context.h>
#include "sdkconfig.h"
Expand Down Expand Up @@ -260,4 +261,5 @@ ld_include_hli_vectors_rmt:


#endif // CONFIG_BTDM_CTRL_HLI
#endif // WLEDMM
#endif // XTensa
2 changes: 2 additions & 0 deletions lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ License along with NeoPixel. If not, see
#include <Arduino.h>

#if defined(ARDUINO_ARCH_ESP32)
#if !defined(WLED_USE_SHARED_RMT) && !defined(__riscv) && (ESP_IDF_VERSION_MAJOR >= 4) // WLEDMM don't compile this file on unsupported platforms

#include <algorithm>
#include "esp_idf_version.h"
Expand Down Expand Up @@ -504,4 +505,5 @@ esp_err_t NeoEsp32RmtHiMethodDriver::WaitForTxDone(rmt_channel_t channel, TickTy
return rv;
}

#endif
#endif
2 changes: 1 addition & 1 deletion wled00/bus_wrapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ bool canUseSerial(void); // WLEDMM (wled_serial.cpp) returns true if Serial ca
#endif


// RMT driver selection
// RMT driver selection - only for Xtensa and ESP-IDF 4.x
#if !defined(WLED_USE_SHARED_RMT) && !defined(__riscv) && defined(ARDUINO_ARCH_ESP32) && (ESP_IDF_VERSION_MAJOR >= 4)

#include <NeoEsp32RmtHIMethod.h>
Expand Down