search

ArmDeveloperEcosystem / lorawan-library-for-pico

Enable LoRaWAN communications on your Raspberry Pi Pico or any RP2040 based board. 📡
BSD 3-Clause "New" or "Revised" License
125 stars 47 forks source link

Don't cancel alarm if last_rtc_alarm_id hasn't been set #1

Closed keichan34 closed 3 years ago

keichan34 commented 3 years ago

Hi, thanks for this! I was trying to get this working with a Pi Pico and a RFM95W, and for some reason I was getting an assertion error in the pico-sdk:

https://github.com/raspberrypi/pico-sdk/blob/master/src/common/pico_util/include/pico/util/pheap.h#L105

Changing the alarm_pool_create max_timers attribute in this PR seemed to fix the error, but I'm not sure why.. Do you have any ideas why this would be happening? Perhaps I'm using an incompatible version of the SDK?

Please don't hesitate to close this PR if it isn't appropriate. Thanks!

sandeepmistry commented 3 years ago

Hi @keichan34,

Thank you for the pull request!

Do you have any ideas why this would be happening? Perhaps I'm using an incompatible version of the SDK?

Do you have some steps to reproduce this? Was it with the built-in examples or using some custom code?

For SDK, I was using the v1.1.2 tag, which version are you using?

keichan34 commented 3 years ago

I was able to reproduce this with the hello_otaa and otaa_temperature_led examples. I was using the v1.1.2 tag as well...

I'm also cross compiling from Mac, using arm-none-eabi-gcc-9.2.1. I'm going to try cross compiling from a Linux environment next.

keichan34 commented 3 years ago

@sandeepmistry I updated the PR with a better fix -- it looks like passing in -1 as the last_rtc_alarm_id wrapped it around to 255. In alarm_pool_cancel_alarm, alarm_id is typecast to pheap_node_id_t, which is a uint8_t for me.

(in the meantime I also updated the compiler to arm-none-eabi-gcc-10.2.1)

sandeepmistry commented 3 years ago

@keichan34 awesome work!

The revised version in https://github.com/sandeepmistry/pico-lorawan/pull/1/commits/0bd0b0339c1466a14496aa4b9e1f5126688b5ae2 makes sense, thanks again for this pull request!