allow multiple LoRa modules to use onReceive

[paidforby]

Based on discussion in #279, this library currently does not support the use of a local LoRa object as a receiver. This prevents the use of multiple LoRa modules in which more than one module is able to receive packets.

Problem

The function that handles the onReceive callback, onDio0Rise, contains a reference to the global LoRa object when it calls handleDio0Rise, see LoRa.cpp line 715. Regardless of the LoRaClass object on which you call the onReceive function, the global LoRa object is always the object on which handleDio0Rise is called. If you then tried to read from a local LoRa object, the buffer for that object would never be properly cleared and reset because handleDio0Rise is never called on that object.

I imagine that the onDio0Rise function was written this way because it is a callback and, therefore, cannot contain references to this or other non-static members, so a quick fix was to use the global object.

Solution

The "right" way to handle this non-static member inside of a callback is to pass the LoRaClass object down to onDio0Rise from where the callback is set in onReceive here.

I suppose something like this should work,

attachInterrupt(digitalPinToInterrupt(_dio0), LoRaClass::onDio0Rise(this), RISING);

and then rewrite onDio0Rise like so,

ISR_PREFIX void LoRaClass::onDio0Rise(LoRaClass *lora)
{
  lora->handleDio0Rise();
}

I haven't tested to see if this will actually work, but it seems like the best solution since it should keep the syntax of onReceive the same and only actually makes a difference when you call it on a local LoRa object. I'll test and report back.

[paidforby]

Hmm, I'm finding information that says that attachInterrupt cannot take functions with arguments, so that's annoying. See here under ISR, https://www.arduino.cc/reference/en/language/functions/external-interrupts/attachinterrupt/, any ideas of a work around?

[paidforby]

Trying to think through possible work-arounds. The best thing I can think of is to keep a global key-value map of all created LoRa objects keyed by the interrupt pin they are attached to, you should then be able to get that interrupt pin during ISR and select the correct object on which to call handleDio0Rise. I'm beginning to see why y'all just used the global LoRa object, haha.

I also saw one idea to use a macro to define a function that takes takes a variable, but acts like it doesn't. See this forum discussion, https://forum.arduino.cc/index.php?topic=267380.0

[morganrallen]

I saw a similar error while trying a variation this morning.

key-value map

There has to be something in the realm of C++ that provides this kind of functionality. I'm going to read up on static functions in C++ and see if that turns anything up.

[paidforby]

@morganrallen you are correct that there is a key-value map option in C++. We use it in disaster radio for handling multiple websocket connections, it's called an unordered map. See this line in our WebsocketClient code. We map a pointer to the WebsocketClient object to a hash that corresponds to a client id. I'm imagining something similar could be done with LoRaClass objects mapped to interrupt pins. I'll hack a bit on it myself in the next few days to see if I can come up with anything.

[IoTThinks]

How you specify SS of each LoRa in this case for one 1 SPI?
Or you use two SPI (SPI / VSPI for each LoRa?

Thanks a lot.

[paidforby]

You would specify the CS pins using the setPins function of the LoRaClass before calling begin() on the LoRa object, with something like this it should be possible to initialize two LoRa objects,

LoRaClass LoRaIn;
LoRaClass LoRaOut;
LoRaIn.setPins(10,9,2);
LoRaOut.setPins(8,4,7);
LoRaIn.begin(915E6);
LoRaOut.begin(915E6);

However, setting this chip select is irrelevant to the onReceive callback because it will always attach onDio0Rise to the interrupt (as the ISR) and onDio0Rise contains a call to the global LoRa object, not the dynamic, locally created objects. Even if you set the callback like this, LoRaIn.onReceive(myReceiveFunction);, the onDio0Rise function we still be the ISR and still calls handleDio0Rise on the wrong LoRaClass object.

Really this is a flaw in the design of this library. I got distracted and started trying to use RadioLib to solve my problem. It is able to initialize two LoRa modules successfully because it does not rely on a global class object, instead it forces users to create their own module objects. Their solution to the attachInterrupt issue is to allow the users to pass the ISR to the object. The ISR function is intended to only set a flag. That flag is then checked during the loop() function. You then can write your own dual LoRa wrapper that sets different flags for the two modules. This is the "right" way and the more Arduino way to do perform asynchronous tasks. The arduino-LoRa library instead wraps the ISR function for you and makes it seem more asynchronous, but disguises the fact that it is using a global variable to accomplish this asynchronicity.

I'm not sure of a good solution that isn't just a hack, Arduino isn't meant for async tasks, and the more things you add to the ISR, the more likely it is to crash your microcontroller.

[IoTThinks]

@paidforby Thanks for your comprehensive reply.

Now I realize about the global LoRa object.
Hope we will find a way to improve this issue.

Setting two CSs may not be sufficient.

Let me look at RadioLib for any hint to move on.

Thanks a lot again.

[IoTThinks]

I believe the only thing to solve is the global LoRa object.

The callback in this library can be use to set flag only too.
Then can process the tx or rx in loop.

This library has the beauty of simplicity.
Simpility means lightweight and less maintenance work.

[paidforby]

The onReceive callback technically can be used to set a flag, but that doesn't change the ISR function that is called, that is what needs to be changed.

I agree that simplicity is of this library is beautiful, but it is also deceiving.

However, I'm thinking that I may be able to solve my complaint by avoiding the onReceive function completely and instead just attempting to use parsePacket in the loop. I'm not sure if there are any down sides to this, but it's worth a shot. It looks like parsePacket does something similar to onDio0Rise, but does it synchronously, which is fine with me because the async aspect of onReceive is kind of a hack anyway.

[IoTThinks]

parsePacket is to use Single RX mode.
Callback is to use Continous RX mode.

The mode is in the datasheet.
If we dont call parsePacket fast enough, we may miss the receiving packet.
Yes, I believe parsePacket is a blocking / sync action.

[paidforby]

Got it. Thanks for explaining that. So using parsePacket probably isn't a viable solution. It seems to me that the best solution would be to change the suggested usage of the library and have a user check an interrupt flag during loop with something like this,

if(receiveInterrupt){
    packetSize = LoRaIn.handleDio0Rise();
    for (int i = 0; i < packetSize; i++) {
        Serial.print((char)LoRaIn.read());
    }
   receiveInterrupt = false;
}

and to use onReceive to set that receiveInterrupt flag.

With this the only changes that need to be made is for handleDio0Rise to be made public and to have it return the packetSize. My only issue is that this leaves in place the questionable onDio0Rise function, which could still be used with the global LoRa object.

[IoTThinks]

Can we have two or more global LoRa objects instead? Haha

By right, it should be simple to fix to LoRaClass.this or make this object private to the instance.

I also dont know how one SPI takes turn to Low/high two SSs of two LoRa modules.
As the library sets the CS whenever it needs.
Currently, I have to make each LoRa to sleep() in turn.

[IoTThinks]

@morganrallen Any idea on @paidforby recommendation?

[IoTThinks]

@paidforby After trying to fix here and there a bit.
I realized why there was global LoRa object. Hehehe.

The main problem is with this line.
This line will create issues with static vs. non-static function and we can not pass in paramter into onDioRise() inside attachInterrupt
attachInterrupt(digitalPinToInterrupt(_dio0), LoRaClass::onDio0Rise, RISING);

So the limitation of attachInterrupt is the main root cause.

[paidforby]

Sure, I mentioned that in an earlier comment, #367 (comment). This is what I was saying about "technically" being able to set a flag with onReceive back in #367 (comment), because the function you pass to onReceive isn't actually the ISR, but a function that is eventually called by the ISR.

With this knowledge there are two ways to solve the problem,

A) Fix the attachInterrupt side by allowing the user to pass their own static ISR function into onReceive that is then attached to the interrupt. Then the onDio0Rise could become a user-defined code containing user-defined LoRaClass objects.

OR

B) Fix the onDio0Rise with a work around that selects the correct LoRaClass object from a key-value map and then calls handleDio0Rise on that object.

I'm in favor of option A because it is more honest about how the library is operating and is the more correct usage of an ISR function. Option B seems messy and hacky for only a small pay off.

I haven't pursued either solution very far because I am currently satisfied with my RadioLib solution. If anyone is interested in taking a look at how I have used RadioLib to do this, you can see the LoRaLayer2 library portion in Layer1_SX1276.cpp and the relevant main.ino code, here and here. Apologies if it is a little messy, I haven't had time to clean it up and add comments.

[IoTThinks]

I checked RadioLib.
The code is quite modular.
InvertIQ is not supported yet.

Will be useful if two LoRa modules are put into two different SPI interfaces.

[paidforby]

See the commit in which I referenced this issue. paidforby@92177e0

In it, I implemented Option A. The only added syntax is that the user must execute handleDio0Rise() on a LoRaClass object of their choosing before reading from the object, typically this object will be just LoRa, but it doesn't have to be. I updated the ReceiverCallback example such that it should work with this change. I also tested with the disaster radio code and it seems to be working.

I left onDio0Rise in the library because it is also used by onTxDone. I have never used the txDone functionality and am not sure how to test it properly.

If @morganrallen and/or @sandeepmistry think this change is worth adding to the library, I'm happy to open a merge request.

[IoTThinks]

Multiple LoRa modules can not use other interrupt functions such as OnTxDone too.
Same issue.

Thinking to merge @paidforby 's code into my own Repository.

[paidforby]

Go for it! I have not done anything for onTxDone, but since it involves that global Lora object I'm guessing it would have the same problem as onReceive and probably can be solved in a similar fashion.

[IoTThinks]

This library seems less active recently.

But nevermind, let me try based on your work.