Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
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
489 changes: 489 additions & 0 deletions lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions lib/NeoESP32RmtHI/library.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "NeoESP32RmtHI",
"build": { "libArchive": false },
"platforms": ["espressif32"],
"dependencies": [
{
"owner": "makuna",
"name": "NeoPixelBus",
"version": "^2.7.9"
}
]
}
265 changes: 265 additions & 0 deletions lib/NeoESP32RmtHI/src/NeoEsp32RmtHI.S
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
/* RMT ISR shim
* Bridges from a high-level interrupt to the C++ code.
*
* This code is largely derived from Espressif's 'hli_vector.S' Bluetooth ISR.
*
*/

#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"
#include "soc/soc.h"

/* If the Bluetooth driver has hooked the high-priority interrupt, we piggyback on it and don't need this. */
#ifndef CONFIG_BTDM_CTRL_HLI

/*
Select interrupt based on system check level
- Base ESP32: could be 4 or 5, depends on platform config
- S2: 5
- S3: 5
*/

#if CONFIG_ESP_SYSTEM_CHECK_INT_LEVEL_5
/* Use level 4 */
#define RFI_X 4
#define xt_highintx xt_highint4
#else /* !CONFIG_ESP_SYSTEM_CHECK_INT_LEVEL_5 */
/* Use level 5 */
#define RFI_X 5
#define xt_highintx xt_highint5
#endif /* CONFIG_ESP_SYSTEM_CHECK_INT_LEVEL_5 */

// Register map, based on interrupt level
#define EPC_X (EPC + RFI_X)
#define EXCSAVE_X (EXCSAVE + RFI_X)

// The sp mnemonic is used all over in ESP's assembly, though I'm not sure where it's expected to be defined?
#define sp a1

/* Interrupt stack size, for C code. */
#define RMT_INTR_STACK_SIZE 512

/* Save area for the CPU state:
* - 64 words for the general purpose registers
* - 7 words for some of the special registers:
* - WINDOWBASE, WINDOWSTART — only WINDOWSTART is truly needed
* - SAR, LBEG, LEND, LCOUNT — since the C code might use these
* - EPC1 — since the C code might cause window overflow exceptions
* This is not laid out as standard exception frame structure
* for simplicity of the save/restore code.
*/
#define REG_FILE_SIZE (64 * 4)
#define SPECREG_OFFSET REG_FILE_SIZE
#define SPECREG_SIZE (7 * 4)
#define REG_SAVE_AREA_SIZE (SPECREG_OFFSET + SPECREG_SIZE)

.data
_rmt_intr_stack:
.space RMT_INTR_STACK_SIZE
_rmt_save_ctx:
.space REG_SAVE_AREA_SIZE

.section .iram1,"ax"
.global xt_highintx
.type xt_highintx,@function
.align 4

xt_highintx:

movi a0, _rmt_save_ctx
/* save 4 lower registers */
s32i a1, a0, 4
s32i a2, a0, 8
s32i a3, a0, 12
rsr a2, EXCSAVE_X /* holds the value of a0 */
s32i a2, a0, 0

/* Save special registers */
addi a0, a0, SPECREG_OFFSET
rsr a2, WINDOWBASE
s32i a2, a0, 0
rsr a2, WINDOWSTART
s32i a2, a0, 4
rsr a2, SAR
s32i a2, a0, 8
#if XCHAL_HAVE_LOOPS
rsr a2, LBEG
s32i a2, a0, 12
rsr a2, LEND
s32i a2, a0, 16
rsr a2, LCOUNT
s32i a2, a0, 20
#endif
rsr a2, EPC1
s32i a2, a0, 24

/* disable exception mode, window overflow */
movi a0, PS_INTLEVEL(RFI_X+1) | PS_EXCM
wsr a0, PS
rsync

/* Save the remaining physical registers.
* 4 registers are already saved, which leaves 60 registers to save.
* (FIXME: consider the case when the CPU is configured with physical 32 registers)
* These 60 registers are saved in 5 iterations, 12 registers at a time.
*/
movi a1, 5
movi a3, _rmt_save_ctx + 4 * 4

/* This is repeated 5 times, each time the window is shifted by 12 registers.
* We come here with a1 = downcounter, a3 = save pointer, a2 and a0 unused.
*/
1:
s32i a4, a3, 0
s32i a5, a3, 4
s32i a6, a3, 8
s32i a7, a3, 12
s32i a8, a3, 16
s32i a9, a3, 20
s32i a10, a3, 24
s32i a11, a3, 28
s32i a12, a3, 32
s32i a13, a3, 36
s32i a14, a3, 40
s32i a15, a3, 44

/* We are about to rotate the window, so that a12-a15 will become the new a0-a3.
* Copy a0-a3 to a12-15 to still have access to these values.
* At the same time we can decrement the counter and adjust the save area pointer
*/

/* a0 is constant (_rmt_save_ctx), no need to copy */
addi a13, a1, -1 /* copy and decrement the downcounter */
/* a2 is scratch so no need to copy */
addi a15, a3, 48 /* copy and adjust the save area pointer */
beqz a13, 2f /* have saved all registers ? */
rotw 3 /* rotate the window and go back */
j 1b

/* the loop is complete */
2:
rotw 4 /* this brings us back to the original window */
/* a0 still points to _rmt_save_ctx */

/* Can clear WINDOWSTART now, all registers are saved */
rsr a2, WINDOWBASE
/* WINDOWSTART = (1 << WINDOWBASE) */
movi a3, 1
ssl a2
sll a3, a3
wsr a3, WINDOWSTART

_highint_stack_switch:
movi a0, 0
movi sp, _rmt_intr_stack + RMT_INTR_STACK_SIZE - 16
s32e a0, sp, -12 /* For GDB: set null SP */
s32e a0, sp, -16 /* For GDB: set null PC */
movi a0, _highint_stack_switch /* For GDB: cosmetics, for the frame where stack switch happened */

/* Set up PS for C, disable all interrupts except NMI and debug, and clear EXCM. */
movi a6, PS_INTLEVEL(RFI_X) | PS_UM | PS_WOE
wsr a6, PS
rsync

/* Call C handler */
mov a6, sp
call4 NeoEsp32RmtMethodIsr

l32e sp, sp, -12 /* switch back to the original stack */

/* Done with C handler; re-enable exception mode, disabling window overflow */
movi a2, PS_INTLEVEL(RFI_X+1) | PS_EXCM /* TOCHECK */
wsr a2, PS
rsync

/* Restore the special registers.
* WINDOWSTART will be restored near the end.
*/
movi a0, _rmt_save_ctx + SPECREG_OFFSET
l32i a2, a0, 8
wsr a2, SAR
#if XCHAL_HAVE_LOOPS
l32i a2, a0, 12
wsr a2, LBEG
l32i a2, a0, 16
wsr a2, LEND
l32i a2, a0, 20
wsr a2, LCOUNT
#endif
l32i a2, a0, 24
wsr a2, EPC1

/* Restoring the physical registers.
* This is the reverse to the saving process above.
*/

/* Rotate back to the final window, then start loading 12 registers at a time,
* in 5 iterations.
* Again, a1 is the downcounter and a3 is the save area pointer.
* After each rotation, a1 and a3 are copied from a13 and a15.
* To simplify the loop, we put the initial values into a13 and a15.
*/
rotw -4
movi a15, _rmt_save_ctx + 64 * 4 /* point to the end of the save area */
movi a13, 5

1:
/* Copy a1 and a3 from their previous location,
* at the same time decrementing and adjusting the save area pointer.
*/
addi a1, a13, -1
addi a3, a15, -48

/* Load 12 registers */
l32i a4, a3, 0
l32i a5, a3, 4
l32i a6, a3, 8
l32i a7, a3, 12
l32i a8, a3, 16
l32i a9, a3, 20
l32i a10, a3, 24
l32i a11, a3, 28 /* ensure PS and EPC written */
l32i a12, a3, 32
l32i a13, a3, 36
l32i a14, a3, 40
l32i a15, a3, 44

/* Done with the loop? */
beqz a1, 2f
/* If no, rotate the window and repeat */
rotw -3
j 1b

2:
/* Done with the loop. Only 4 registers (a0-a3 in the original window) remain
* to be restored. Also need to restore WINDOWSTART, since all the general
* registers are now in place.
*/
movi a0, _rmt_save_ctx

l32i a2, a0, SPECREG_OFFSET + 4
wsr a2, WINDOWSTART

l32i a1, a0, 4
l32i a2, a0, 8
l32i a3, a0, 12
rsr a0, EXCSAVE_X /* holds the value of a0 before the interrupt handler */

/* Return from the interrupt, restoring PS from EPS_X */
rfi RFI_X


/* The linker has no reason to link in this file; all symbols it exports are already defined
(weakly!) in the default int handler. Define a symbol here so we can use it to have the
linker inspect this anyway. */

.global ld_include_hli_vectors_rmt
ld_include_hli_vectors_rmt:


#endif // CONFIG_BTDM_CTRL_HLI
#endif // WLEDMM
#endif // XTensa
Loading