Two RFM69 on One Moteino

[TomWS]

...
Another possibility is 'SRAM' is getting low, here is an article that describes problem, and provides solution https://learn.adafruit.com/memories-of-an-arduino/measuring-free-memory

...
Kiwi

Thanks for the link!  I've been wondering how to measure the amount of free SRAM and THERE it is!  Also, the article makes one wonder if it's worthwhile exploring 'garbage' collection methods.  On the other hand, 'holes' should only occur if you're using dynamically allocated storage and, as far as I know, none of the 'standard' Moteino libraries use malloc, etc.  Are you aware of any?  Maybe sprintf and a few others?

Tom

[donaldhwong]

Hello KiwiSinceBirth,

Thank you so very much for your continuous support of this very important project of mine.

I understand what you are saying and I read through your edits, though I still  am at function level.  But I am learning a step at a time.

In short, this time, it did not even go through the setup().  Meaning the following message was not displayed in the Serial Monitor.

sprintf(buff, "\nListening at %d Mhz...", FREQUENCY==RF69_433MHZ ? 433 : FREQUENCY==RF69_868MHZ ? 868 : 915);

I am thinking of the following.  When the Node transmits.  Both Gateway will receive and both will interrupt practically at the same time.  If we process the first one, will the second one continue to interrupt until it is being processed or not? If not, then it is okay for the project, if we need to alternately listen to one after the other.

Second, I slowed my Node to transmit at 1,000 millisecond, I still don't see any kind of interrupt from the second RFM69W.  Even just readRSSI. 

I have implemented RAM reporting.  There are still 1,316 bytes of Free RAM.

End of report.  Waiting for your further advise.

Thank you so much and Merry Christmas.

Donald

[kiwisincebirth]

Some more research:

http://www.dorkbotpdx.org/blog/paul/spi_transactions_in_arduino
http://www.pjrc.com/teensy/td_libs_SPI.html

[TomWS]

...

In short, this time, it did not even go through the setup().  Meaning the following message was not displayed in the Serial Monitor.

Code Select Expand

sprintf(buff, "\nListening at %d Mhz...", FREQUENCY==RF69_433MHZ ? 433 : FREQUENCY==RF69_868MHZ ? 868 : 915);

....

Donald

I have to ask: did you have a

Code Select Expand

Serial.println(buf);

after your sprintf function call above?

Tom

[donaldhwong]

Hi KiwiSinceBirth,

Thank you for SPI information.  I will study them.

Thank you Tom.

Yes, these were the original Felix's code.  There was a "char [buf]" defined.

Happy Holidays,
Donald
Christmas Eve.

[kiwisincebirth]

I have attached some updated libraries, (I have no way to test currently). I have put them back to a similar state as original, but left the select() unselect() nested and interrupt handling fixes. Please try them.

The reason I sent you the SPI articles is clearly there are issues with multiple devices on the same channel, especially when they are competing for interrupts and SPI at the same times.

My fixes were basically trying to gain some exclusivity of interrupts and SPI bus, which reading the links is basically the same goal. Really the best outcome would probably to re-write the RFM library using the new transactional features introduced into SPI libraries.

Kiwi.

[TomWS]

I have attached some updated libraries, (I have no way to test currently). I have put them back to a similar state as original, but left the select() unselect() nested and interrupt handling fixes. Please try them.

The reason I sent you the SPI articles is clearly there are issues with multiple devices on the same channel, especially when they are competing for interrupts and SPI at the same times.

My fixes were basically trying to gain some exclusivity of interrupts and SPI bus, which reading the links is basically the same goal. Really the best outcome would probably to re-write the RFM library using the new transactional features introduced into SPI libraries.

Kiwi.

Kiwi, are you suggesting that 'select' is called more than once before 'unselect' is called?  I'm not understanding why you have the counters in the code.

Is this an artifact of trying to get two radios working and wouldn't be a problem in a single radio scenario (ALL known cases but one)?

Tom

[donaldhwong]

Hello Again, KiwiSinceBirth,

So glad to know that you have not given up on me yet.  Very much appreciated indeed.

I tried the original Felix's code of Gateway and Node with your modified files.  TWICE.  But both times, the interrupt did not engage.  The Serial Monitor stopped at:

Listening at 433 Mhz...
SPI Flash Init OK. Unique MAC = [D7:64:44:24:63:5F:71:31:]

I thank you for taking the time on holidays trying to address this.

Thank you and Happy New Year,
Donald

[kiwisincebirth]

Kiwi, are you suggesting that 'select' is called more than once before 'unselect' is called?  I'm not understanding why you have the counters in the code.

Is this an artifact of trying to get two radios working and wouldn't be a problem in a single radio scenario (ALL known cases but one)?

Tom

Hi Tom.

This was an coding experiment on my behalf to try an get the RFM69 library to operate with two independent radios, as Donald is attempting to do. And yes clearly a single radio has been well tested, and works as per the published code, however I think there is one POTENTIAL issue (below), not sure.

My initial thought that simple removal of static declared variables would be all that was needed, but latter the issue of contention of interrupts (firing simultaneously), and the SPI bus are the main issues.

I introduced the counters in the select() unselect() so that I could call these methods throughout the code, to create larger blocks of exclusive-ness (without worrying about a sub-procedure using these functions also), and prevent a second instance (of the radio) from interfering with the first.

As I said so far this hasn't worked, but let me to one POTENTIAL issue; In the first line of the interrupt handler

Code Select Expand

if (_mode == RF69_MODE_RX && (readReg(REG_IRQFLAGS2) & RF_IRQFLAGS2_PAYLOADREADY))

A call is made to readReg() which uses select() and unselect(). select() calls nointerrupts() which is fine because interrupts are disabled anyway, but unselect() call's interrupts() which enables interrupts prior to going back to the RFM69 interrupt handler.

I think it leaves the RFM interrupt handler open to further interrupt, until another select() unselect() pair is encountered, so not a big issue but not totally ideal.

Hope this makes some sense.

Kiwi

[TomWS]

Hi Tom.

This was an coding experiment on my behalf to try an get the RFM69 library to operate with two independent radios, as Donald is attempting to do. And yes clearly a single radio has been well tested, and works as per the published code, however I think there is one POTENTIAL issue (below), not sure.

My initial thought that simple removal of static declared variables would be all that was needed, but latter the issue of contention of interrupts (firing simultaneously), and the SPI bus are the main issues.

On the two radio solution are you still using the same interrupt pin or did you move the second radio to another pin (like pin 3, for example)?

The tricky part with the interrupt handler is that the pointer to it MUST be to a static function (so it's always in the same place for the interrupt routine to call).  Since you now have two instances, each instance COULD be the same code (ie, located within the same static object) IF, and ONLY IF, all references within the code are pointer based references ('this' won't work since 'this' may reference the wrong object).  I would think that would require a substantial re-write of the interrupt handler. 
My inclination would be to try to externalize the interrupt handler(s) with a static function or functions and make the RFM69 Interrupt handler simply a method of radio (remove interrupt modifier from its declaration/definition).  In this way each externalized interrupt handler would be separately attachable and then would be able to call the appropriate instance of radio object (ie radio1.interruptProcessor(), radio2.interruptProcessor()).  When they exit, they will return to the interrupt handler that called them and its return would be a proper interrupt handler return.

I introduced the counters in the select() unselect() so that I could call these methods throughout the code, to create larger blocks of exclusive-ness (without worrying about a sub-procedure using these functions also), and prevent a second instance (of the radio) from interfering with the first.

As I said so far this hasn't worked, but let me to one POTENTIAL issue; In the first line of the interrupt handler

Code Select Expand

if (_mode == RF69_MODE_RX && (readReg(REG_IRQFLAGS2) & RF_IRQFLAGS2_PAYLOADREADY))

A call is made to readReg() which uses select() and unselect(). select() calls nointerrupts() which is fine because interrupts are disabled anyway, but unselect() call's interrupts() which enables interrupts prior to going back to the RFM69 interrupt handler.

I think it leaves the RFM interrupt handler open to further interrupt, until another select() unselect() pair is encountered, so not a big issue but not totally ideal.
Hope this makes some sense.

Kiwi

It does make sense, good catch!  I'll have to look at the code again to see what harm the original code would cause.

[kiwisincebirth]

On the two radio solution are you still using the same interrupt pin or did you move the second radio to another pin (like pin 3, for example)?

The tricky part with the interrupt handler is that the pointer to it MUST be to a static function (so it's always in the same place for the interrupt routine to call).  Since you now have two instances, each instance COULD be the same code (ie, located within the same static object) IF, and ONLY IF, all references within the code are pointer based references ('this' won't work since 'this' may reference the wrong object).  I would think that would require a substantial re-write of the interrupt handler. 
My inclination would be to try to externalize the interrupt handler(s) with a static function or functions and make the RFM69 Interrupt handler simply a method of radio (remove interrupt modifier from its declaration/definition).  In this way each externalized interrupt handler would be separately attachable and then would be able to call the appropriate instance of radio object (ie radio1.interruptProcessor(), radio2.interruptProcessor()).  When they exit, they will return to the interrupt handler that called them and its return would be a proper interrupt handler return.

The requirement/assumption I took was that the second radio, would be wired to Interrupt#1 ( Pin#3 on AtMega328 ). These values would need to be passed in the constructor of the second radio instance.

If you look at the code I posted (#35 above); I have introduced a second static isr1() routine and selfPointer1 variable. isr1() is coded to call the interruphandler() of the selfPointer1 instance. In the initialize() method the appropriate isr0() or isr1() is attached, and the selfPointer0 or selfPointer1 static variable set to this. The only other change is of course to remove the static declaration off all other variables not described above.

The above change I think is all that is need to isolate the two radio object instances, running on two different interrupts (0 and 1), and correctly route interrupts to the appropriate instance.

Kiwi

[TomWS]

The requirement/assumption I took was that the second radio, would be wired to Interrupt#1 ( Pin#3 on AtMega328 ). These values would need to be passed in the constructor of the second radio instance.

If you look at the code I posted (#35 above); I have introduced a second static isr1() routine and selfPointer1 variable. isr1() is coded to call the interruphandler() of the selfPointer1 instance. In the initialize() method the appropriate isr0() or isr1() is attached, and the selfPointer0 or selfPointer1 static variable set to this. The only other change is of course to remove the static declaration off all other variables not described above.

The above change I think is all that is need to isolate the two radio object instances, running on two different interrupts (0 and 1), and correctly route interrupts to the appropriate instance.

Kiwi

I'm not sure this is enough.  If the internal interrupt handler is declared as static, as it needs to be, then the only way to attach a distinct handler is if you externalize it - otherwise you end up attaching the SAME function as both handlers.  Without knowing the internals of what the compiler generates for the handler code, my suspicion is that the safest way is to convert the handler within each object so that they are truly unique (ie, NOT static).  So, create a separate function that is a static interrupt handler that calls the correct instance. 

I could be wrong about this, but I'm pretty sure this 'brute force' approach is safer and more likely to work...

Tom
PS: I'm still confused about the use case for two RFM69 radios in the same Mote, but, at least, this dialog is interesting and productive.

[TomWS]

Ok, I looked at your code posted at #35.   I agree that this MIGHT work, but, uh, it doesn't (from what you've said).  I think it's a tricky thing to contain (in a single object) and attach two ISRs to two different interrupt sources when the 'attach' point has to be a static function.  In this case what does 'this' refer to when you are setting up and when you're calling a respective ISR?  I don't know - it depends on how the compiler builds the code.

If, on the other hand, you attach your OWN code to be an ISR that calls radio1.interruptHandler or radio2.interruptHandler there is no ambiguity - for you or the the compiler...

Tom

[kiwisincebirth]

Thanks for the link!  I've been wondering how to measure the amount of free SRAM and THERE it is!  Also, the article makes one wonder if it's worthwhile exploring 'garbage' collection methods.  On the other hand, 'holes' should only occur if you're using dynamically allocated storage and, as far as I know, none of the 'standard' Moteino libraries use malloc, etc.  Are you aware of any?  Maybe sprintf and a few others?

Tom

YES The most common being the Sting Class, which uses dynamic memory allocation when the internal buffer is not large enough. Here is a good description of the problem, and fix. https://code.google.com/p/arduino/issues/detail?id=449

Also the first article I linked to is part of a series, with alot more information about memory. https://learn.adafruit.com/memories-of-an-arduino?view=all

Kiwi

[kiwisincebirth]

Ok, I looked at your code posted at #35.   I agree that this MIGHT work, but, uh, it doesn't (from what you've said).  I think it's a tricky thing to contain (in a single object) and attach two ISRs to two different interrupt sources when the 'attach' point has to be a static function.  In this case what does 'this' refer to when you are setting up and when you're calling a respective ISR?  I don't know - it depends on how the compiler builds the code.

If, on the other hand, you attach your OWN code to be an ISR that calls radio1.interruptHandler or radio2.interruptHandler there is no ambiguity - for you or the the compiler...

Tom

I agree with your assessment, but I wouldn't want a single radio user to have to write the ISR. Best to pass it as an optional argument in the constructor, with a default which is the internal ISR method, just an opinion, not sure if this is possible.

Not sure what the actual issue is to stop it from working, I suspect contention on interrupts, and SPI bus. To make this work would require some considered effort, and debugging time to get working. I personally don have a use for it, all my Motinos are in the field (more on order) so cannot even test a single radio works with modified code.

Have you read this article ?

http://www.dorkbotpdx.org/blog/paul/spi_transactions_in_arduino

Kiwi

PS. My main interest is improving the RFM69 code base. To that end; Could the last line of readAllRegs() "unselect();" be removed ? To me it looks like redundant code, since "unselect();" is called in the preceding loop.