Lora-net / LoRaMac-node

Reference implementation and documentation of a LoRa network node.
Other
1.87k stars 1.09k forks source link

SX126x: DIO1 IRQ not handled #444

Closed ygaklk closed 6 years ago

ygaklk commented 6 years ago

About the sx126x radio code (src/radio/sx126x/radio.c), the function RadioIrqProcess() is never called (same thing for Radio.RadioIrqProcess() ) in the latest implementation (develop branch).

Without this call, it is not possible to have a full LoRaWAN communication. Because, I suppose that the correct behavior is that the DIO1 IRQ calls the RadioOnDioIrq() function, in order to set the IrqFire flag to true, to schedule the RadioIrqProcess() outisde the ISR context. But I can't find any reference to RadioIrqProcess() in the code.

On my implementation, the RadioOnDioIrq() is called outside the ISR context, so I rewrite this function to:

void RadioOnDioIrq( void )
{
    IrqFired = true;
    RadioIrqProcess();
}

And it works like a charm (Uplink+Downlink LoRaWAN messages).

By reading the Functions in ISR context issue, I solved this by posting events in a queue during IRQ occurences, and treat them later in classic context. My upper code has the same philosophy to the current stack, so should I purpose a merge request for this fix ?

Extra question: my pull request is still open, without any response/activity. Do I need to make something more ? Open an associated issue ?

Thank you !

mluis1 commented 6 years ago

If you look at the Nucleo boards examples you will notice that the Radio.IrqProcess() function is called.

https://github.com/Lora-net/LoRaMac-node/blob/b2720ba1a06f381f7b6fa00093859e0ee5b1adde/src/apps/LoRaMac/classA/NucleoL073/main.c#L875

https://github.com/Lora-net/LoRaMac-node/blob/b2720ba1a06f381f7b6fa00093859e0ee5b1adde/src/apps/LoRaMac/classA/NucleoL152/main.c#L875

The Nucleo boards examples are the only ones supporting the SX126x radios.

ygaklk commented 6 years ago

Thank you for your reply. And my apologize, you are right about Radio.IrqProcess() inside main.c, but it is strange to have a complete different behavior on the IRQs management process for the sx126x. Any reason to not put the IRQ calback directly on the DIO1 handler, as my upper example code ?

And about my pull request, do I need to perform an action ? Thanks!

mluis1 commented 6 years ago

If you look at the SX126x datasheet it is stated somewhere that the interrupt handling must not be performed inside the interrupt handlers.

Generally speaking it is better to make an interrupt handler as small as possible and to make the processing in the main context. (At the beginning of this project we had other constraints that are no more valid).

Currently the SX127x drivers do handle a lot inside the interrupt handler but, we will refactor those drivers in order to have the same behavior as for the SX126x.