sandeepmistry / arduino-LoRa

An Arduino library for sending and receiving data using LoRa radios.
MIT License
1.67k stars 634 forks source link

Crash: ISR not in IRAM when #254

Closed sbrl closed 5 years ago

sbrl commented 5 years ago

When I try to use continuous receive mode with a Wemos D1 R2, I get the following crash:

Abort called

>>>stack>>>

ctx: cont
sp: 3ffffda0 end: 3fffffc0 offset: 01b0
3fffff50:  000000c0 00000000 3ffee290 4020154f  
3fffff60:  007a1200 00000001 0000000f 3ffee340  
3fffff70:  3fffdad0 3ffee294 3ffee294 4010044a  
3fffff80:  3fffdad0 3ffee294 3ffee294 40201834  
3fffff90:  3fffdad0 3ffee294 3ffee2e8 4020108d  
3fffffa0:  feefeffe 00000000 3ffee310 40202448  
3fffffb0:  feefeffe feefeffe 3ffe84f4 401007c5  
<<<stack<<<
?܆⸮⸮D⸮N⸮LoRa Receiver
ISR not in IRAM!

Here's a link to the sketch I'm using: https://privatebin.net/?f92cda1dbecb73ee#J6+bp9JBfSdUhJ5S7X/u1YsOrPxE4rQddfJgH736wpg=

torntrousers commented 5 years ago

Which release of the Arduino ESP8266 core are you using? I think in the latest core releases all the ISR's must use ICACHE_RAM_ATTR so we probably need to update this LoRa library for that. Can you try adding ICACHE_RAM_ATTR to the start of this line to see if it fixes it?

sbrl commented 5 years ago

Hey, thanks so much for the lightning fast reply @torntrousers! :zap:

Changing that line to this does indeed fix the issue:

ICACHE_RAM_ATTR void LoRaClass::onDio0Rise()

However, this change causes compilation to fail for the Arduino Uno (I also have 2 of these - 1 with a Dragino LoRa Shield that I'm borring from University until ~August, and another which has an RFM95 connected via level shifter).

Looking in the boards manager reveals that I'm using v2.5.2 of the esp8266 package, if that's the one you're talking about?

torntrousers commented 5 years ago

There is a issue about this, I think its a release from 2.5.1 - https://github.com/esp8266/Arduino/issues/6127

I guess that should be surrounded by a conditional, something like - #ifdef ESP32 || ESP8266, https://community.platformio.org/t/how-change-define-device-platform/4251

morganrallen commented 5 years ago

I feel like adding more conditionals is a bad idea. The code base has been having an increasing number of these recently, and I believe they're starting to cause problems. These will be address in a future issue.

So instead of a conditional block around the function declaration, I feel like a macro prefix, with a board specific conditional define elsewhere is a better idea.

Something like....

#ifdef BOARD_ESP8266
    #define ISR_PREFIX ICACHE_RAM_ATTR 
#elif BOARD_SOMEOTHER
    #define ISR_PREFIX FAST_MEM_LOCATION
#else 
    #define ISR_PREFIX
#endif 

Then the function declaration would look like

ISR_PREFIX void LoRaClass::onDio0Rise()

And FWIW, this probably isn't the best C++ solution

torntrousers commented 5 years ago

Try this and see if it fixes it? https://github.com/sandeepmistry/arduino-LoRa/pull/257/files

sbrl commented 5 years ago

It sure does, @torntrousers :smiley_cat:

yheynema commented 5 years ago

Good point, worked for me with RFM69 library. But please have a look at the comment I left about it at: https://github.com/esp8266/Arduino/issues/6127

sbrl commented 5 years ago

Should this be closed? Or should it wait until the next release

morganrallen commented 5 years ago

0.6.0 has been release. Give it a go and reopen if you have further issues.