fix for esp8266 and esp32 when LMIC_USE_INTERRUPTS is used

[d-a-v]

No description provided.

[terrillmoore]

Thanks for this. It's about to become relevant, because the pending changes for time and interrupt handling will make all this work much better.

What does ICACHE_RAM_ATTR expand to? Other function attributes (on other compilers) generally appear after the type, before the function, and may need to be on the prototype. We may want to use this in a way that's portable across compilers. (Some times you need annotation on the prototype, sometimes on the definition, sometimes both... this is why LMIC_DECLARE_FUNCTION_WEAK() is done the way it is.)

Also, this should be done via the lmic_env.h file, and documented similarly to LMIC_ABI_STD or LMIC_DECLARE_FUNCTION_WEAK().

[d-a-v]

Thanks for the review.
This is the current definition on esp8266, mainly an __attribute__((section("some-section"))) with salt and pepper:

#define ICACHE_RAM_ATTR __attribute__ \
    ((section("\".iram.text." __FILE__ "." __ICACHE_STRINGIZE(__LINE__) "."  \
              __ICACHE_STRINGIZE(__COUNTER__) "\"")))

This attribute can indeed be placed elsewhere in the declaration.
I'll come back trying to comply at best with your review.

[terrillmoore]

Thanks, Before you spend too much time, let me take a look at how TCM is handled on ARM with a couple of other compilers; that's the same kind of attribute. I think it wants to be on the prototype. With static in this file, there isn't a prototype, but of course that's easy to change. (It's often convenient to wrap this kind of thing in a typedef; that can also make function roles more explicit.) I'll come back shortly.

[terrillmoore]

Yeah, as I thought, placements on some compilers show up at the end of a declaration -- i.e., the way to declare this would be:

static void isr0(void) LMIC_PLACEMENT_ATTRIBUTE_ISR;

static void isr0(void) {
// body
}

Since some compilers may want different orders, if I were doing a general notation, I probably should use something like LMIC_DELARE_FUNCTION_WEAK(). Ugh, this use is static. Hm, so something like this:

#ifndef LMIC_DECLARE_FUNCTION_PLACEMENT
# define LMIC_DECLARE_FUNCTION_PLACEMENT(a_StorageClass, a_Placement, a_ReturnType, a_FunctionName, a_Params) \
    a_StorageClass a_ReturnType a_Placement a_ReturnType a_FunctionName a_Params
#endif

Then we'd write:

LMIC_DECLARE_FUNCTION_PLACEMENT(static, LMIC_PLACEMENT_ISR, void, hal_isrPin0, (void));
LMIC_DECLARE_FUNCTION_PLACEMENT(static, LMIC_PLACEMENT_ISR, void, hal_isrPin1, (void));
LMIC_DECLARE_FUNCTION_PLACEMENT(static, LMIC_PLACEMENT_ISR, void, hal_isrPin2, (void));

But maybe that's overkill for this. We could have (in lmic_env.h):

#ifndef LMIC_DECLARE_ISR
# if defined(/* whatever for ESP32 */)
#  define LMIC_DECLARE_ISR(a_StorageClass, a_Name)   a_StorageClass ICACHE_RAM_ATTR a_Name(void)
# else
#  define LMIC_DECLARE_ISR(a_StorageClass, a_Name)  a_StorageClass a_Name(void)
# endif
#endif // ndef LMIC_DECLARE_ISR

Then we'd write (in hal.cpp):

LMIC_DECLARE_ISR(static, hal_isrPin0);
LMIC_DECLARE_ISR(static, hal_isrPin1);
LMIC_DECLARE_ISR(static, hal_isrPin2);

I think the last is best, because it's easy to understand, and we're not likely to need other placement strategies.

[cyberman54]

@terrillmoore I ran into the same problem using MCCI LMIC on ESP32 with arduino-esp32 framework. Would be great if this could be merged soon, to make it safe using interrupt DIOs on ESP32.

04:46:33.474 > Guru Meditation Error: Core  1 panic'ed (Cache disabled but cached memory region accessed)
04:46:34.591 > Core 1 was running in ISR context:
04:46:34.591 > Backtrace: 0x400df2f0:0x3ffbe960 0x40088331:0x3ffbe980 0x4008a792:0x3ffba190 0x4008672f:0x3ffba1b0 0x40091edd:0x3ffba1d0
04:46:34.591 >   #0  0x400df2f0:0x3ffbe960 in hal_isrPin0() at .pio\libdeps\usb\MCCI LoRaWAN LMIC library\src\hal/hal.cpp:366
04:46:34.591 >   #1  0x40088331:0x3ffbe980 in _xt_lowint1 at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/freertos/xtensa_vectors.S:1154
04:46:34.591 >   #2  0x4008a792:0x3ffba190 in spi_flash_op_block_func at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/spi_flash/cache_utils.c:81
04:46:34.591 >   #3  0x4008672f:0x3ffba1b0 in ipc_task at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/esp32/ipc.c:62
04:46:34.591 >   #4  0x40091edd:0x3ffba1d0 in vPortTaskWrapper at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/freertos/port.c:355 (discriminator 1)

[cyberman54]

@d-a-v Note: as far as i unterstand, for ESP32 it's IRAM_ATTR, for ESP8266 it's ICACHE_RAM_ATTR.
But there is a #define ICACHE_RAM_ATTR IRAM_ATTR in esp8266-compat.h, so ICACHE_RAM_ATTR should do the job for ESP32, too.

[d-a-v]

IRAM_ATTR also exists for esp8266 ! :) (I added it a while ago)
I'll update with @terrillmoore recommendations asap

[cyberman54]

@d-a-v Don't we need some more ATTR in hal.cpp for ESP32?

• hal_isrPinX() calls os_getTime(), which calls hal_ticks(), so i guess we also need IRAM_ATTR for both os_getTime() and hal_ticks()
• SPI communication is suggested by Espressif docs to use static buffers in DRAM. Not sure if this is necessary while hal.cpp is based on SPI.cpp of arduino esp32 framework