Author Topic: Moteino & W5100 ethernet SPI support / SPI_HAS_TRANSACTION  (Read 37035 times)

kiwisincebirth

  • Jr. Member
  • **
  • Posts: 69
Moteino & W5100 ethernet SPI support / SPI_HAS_TRANSACTION
« on: January 07, 2015, 06:47:30 AM »
Hi Tom, What is the likelihood that the RFM69 library, be updated to support the new SPI API's ?

My understanding (see thread) is that Arduino 1.5.8 includes the new SPI capability, and the ethernet libraries have been updated with the new SPI functions, so updating the RFM69 libraries would

http://forum.arduino.cc/index.php?topic=273369.0

I would like to setup a W5100 ethernet RFM69 gateway, on a single moteino. I don't mind contributing to the effort / testing anyway I can.

Kiwi

« Last Edit: February 29, 2016, 05:27:08 PM by Felix »

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Moteino & W5100 ethernet SPI support
« Reply #1 on: January 07, 2015, 08:49:26 AM »
Hi Tom, What is the likelihood that the RFM69 library, be updated to support the new SPI API's ?

My understanding (see thread) is that Arduino 1.5.8 includes the new SPI capability, and the ethernet libraries have been updated with the new SPI functions, so updating the RFM69 libraries would

http://forum.arduino.cc/index.php?topic=273369.0

I would like to setup a W5100 ethernet RFM69 gateway, on a single moteino. I don't mind contributing to the effort / testing anyway I can.

Kiwi
Hi Kiwi!
We should probably move this to a new thread, however, I've skimmed the current code in the proposed SPI library and I'm not seeing that it really adds anything more to the way that Felix has set up the select()/unselect() functions.  I'll look at it in more detail when I get a chance, but I'm sort of caught up in my own code at this point.

BTW, I'm currently using Arduino IDE 1.5.7 so I'm not sure what the issue would be running under 1.5.8 other than changing the RFM69 library to the new SPI library and that doesn't seem like it would be a big effort.  I'd be glad to help but I probably can't add much time this week.  Like you, I wouldn't mind having an Ethernet option, but I'm also wary of running out of memory even with the code I have.  I keep getting 'low memory' warnings on my builds and have to tweak to make them go away...
« Last Edit: January 07, 2015, 04:42:14 PM by Felix »

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #2 on: January 07, 2015, 07:14:53 PM »
Kiwi,
I'll get the 1.5.8 IDE and check out the new functions.  In principal the beginTransaction() and endTransaction() methods should be a direct replacement for select() and unselect() although the referenced object is different. And we'll need to add a method call to setup the new SPI configuration settings, but that should be easy to fold in.

Tom

kiwisincebirth

  • Jr. Member
  • **
  • Posts: 69
Re: Moteino & W5100 ethernet SPI support
« Reply #3 on: January 08, 2015, 12:36:45 AM »
Kiwi,
I'll get the 1.5.8 IDE and check out the new functions.  In principal the beginTransaction() and endTransaction() methods should be a direct replacement for select() and unselect() although the referenced object is different. And we'll need to add a method call to setup the new SPI configuration settings, but that should be easy to fold in.

Tom
I would agree it doesn't seem like a big change, but it would make the library play nice with the Ethernet, and other such libraries that adopt the SPI library. However I think there are some gotcha's, the receiveDone() method is a bit unusual.

The issue isn't with the RFM library (as Felix disable interrupts), it is with other libraries that may not do the same and risk being interrupted (while actively doing SPI) by and RFM69 interrupt.

I typically have the latest 1.0.x and 1.5.x Arduino applications installed, with suitable renaming of the Application Package (on OS X) I don't have an issue. The application package bundles all the components, except your own contributed libraries an code. Not sure how windows handles it.

Kiwi

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #4 on: January 08, 2015, 08:57:07 AM »
<...snip>
I would agree it doesn't seem like a big change, but it would make the library play nice with the Ethernet, and other such libraries that adopt the SPI library. However I think there are some gotcha's, the receiveDone() method is a bit unusual.
I agree and think we should pursue this.  The only question is when...  I'm too tied up right now, but I'm willing to review anything you do if you want to start a fork.
The issue isn't with the RFM library (as Felix disable interrupts), it is with other libraries that may not do the same and risk being interrupted (while actively doing SPI) by and RFM69 interrupt.
Yeah, this was the part I found confusing about the transaction proposal.  It only masked its own interrupt, leaving the transactions interruptable by other SPI devices (unless I misunderstood the code I admittedly just skimmed).
I typically have the latest 1.0.x and 1.5.x Arduino applications installed, with suitable renaming of the Application Package (on OS X) I don't have an issue. The application package bundles all the components, except your own contributed libraries an code. Not sure how windows handles it.

Kiwi
Cool!  Thanks!  I'll give it a try.

Tom

kiwisincebirth

  • Jr. Member
  • **
  • Posts: 69
Re: Moteino & W5100 ethernet SPI support
« Reply #5 on: January 08, 2015, 05:35:17 PM »
Yeah, this was the part I found confusing about the transaction proposal.  It only masked its own interrupt, leaving the transactions interruptable by other SPI devices (unless I misunderstood the code I admittedly just skimmed).
I haven't read the new SPI code, I assumed that whenever a beginTransaction() was called that interrupts would be masked for any/all other PINS that have been registered, to prevent the interrupt (from the other SPI device) occurring until after then endTransaction() is called.

YES I agree time is a problem, If I get to it will fork (on GItHub) and post link back to this forum

Kiwi

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #6 on: January 08, 2015, 09:01:58 PM »
Yeah, this was the part I found confusing about the transaction proposal.  It only masked its own interrupt, leaving the transactions interruptable by other SPI devices (unless I misunderstood the code I admittedly just skimmed).
I haven't read the new SPI code, I assumed that whenever a beginTransaction() was called that interrupts would be masked for any/all other PINS that have been registered, to prevent the interrupt (from the other SPI device) occurring until after then endTransaction() is called.

YES I agree time is a problem, If I get to it will fork (on GItHub) and post link back to this forum

Kiwi
Don't 'break your back on this.  I had a major setback on my current project today and may now actually have some time to work on this.  I'll know more tomorrow.

Tom

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #7 on: January 10, 2015, 08:07:53 AM »
@Kiwi,
before we go forking ourselves (pun intended), I think we should establish the scope of the project.

Clearly the motivation is to align RFM69 with the new SPI changes, but there are a few other things that need to be established before we start on this:

  • First and foremost, the new library should be backward compatible with existing IDEs.  There should be only one library distributed and the code within the library should deal with IDE version differences, not the user of the library.
  • There are some clean up items that should be done:
    • Certainly cleaning out the unnecessary static declarations would be worthwhile (whether anyone would have multiple instances or not, it just makes for cleaner code)
    • IMO, there are a significant number of non-volatile registers that are accessed unnecessarily when modifying bit fields.  This creates inefficient SPI accesses as well as increasing the amount of interrupt mask/unmask operations.  Using shadow registers within the radio object would eliminate this overhead/risk.
    • The recent deadlock changes need to be reviewed, especially from the point of view of SPI transaction changes.  I think this is important to get right and we might recruit some of the guys who worked on that.
    • I've made some changes to the library to automatically adjust transmit power levels but I think we should keep those out of this project for the time being.  I'll manage those changes separately.


That's all I could think of at the moment, but let's refine and agree on this list before we start attacking the code...
Tom

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #8 on: January 10, 2015, 06:42:06 PM »
@Kiwi,
I don't know whether to be embarrassed, happy, or what, but I just discovered that I've been using version 1.5.8 on my Linux laptop already!  I'm looking at the SPI library now and see that transaction support is there AND, it appears at first glance, that the library DOES collate all the interrupts into a single mask. I won't be surprised to see that they do mask on every 'transaction', whether it's 'your' interrupt or not.  Will let you know after I dig a bit further.

Maybe I should use my Linux laptop BEFORE I have my Scotch... (or Sauvignon Blanc as the case may be)...

Tom

kiwisincebirth

  • Jr. Member
  • **
  • Posts: 69
Re: Moteino & W5100 ethernet SPI support
« Reply #9 on: January 11, 2015, 05:51:46 AM »
Hi Tom, I got my Moteinos a couple of days ago, soldered and connected to a standard ethernet shield, so in a position to move forward. Where are you up to?

I would agree with your statement about backward compatibility. Note spi.h defines #define SPI_HAS_TRANSACTION 1 - for exactly that purpose. In terms of code cleanup:
- Static Declarations is an isolated fix, it has no bearing on the functionality in the code, so YES agreed.
- Non Volatiles. While I agree in principle it is a functional change, if this is changed IMHO it may introduce subtle bugs, my preference would be to exclude this
- Deadlock. I don't know about this issue, but if the resolution involved SPI locking/unlocking then YES should be addressed.

... AND, it appears at first glance, that the library DOES collate all the interrupts into a single mask. I won't be surprised to see that they do mask on every 'transaction', whether it's 'your' interrupt or not.  ...
I haven't looked at the library but would expect ALL SPI registered interrupts would be masked on beginTransaction. Interrupts are non-predictable...

Kiwi.

EDIT: I just did a basic update of the RFM library to add beginTransaction() support, and will leave it running overnight with a gateway sketch just dumping my local RF network traffic (360 messages/hour), will report back if its still running in the morning.
« Last Edit: January 11, 2015, 07:24:00 AM by kiwisincebirth »

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #10 on: January 11, 2015, 08:23:39 AM »
Hi Tom, I got my Moteinos a couple of days ago, soldered and connected to a standard ethernet shield, so in a position to move forward. Where are you up to?

I would agree with your statement about backward compatibility. Note spi.h defines #define SPI_HAS_TRANSACTION 1 - for exactly that purpose. In terms of code cleanup:
- Static Declarations is an isolated fix, it has no bearing on the functionality in the code, so YES agreed.
- Non Volatiles. While I agree in principle it is a functional change, if this is changed IMHO it may introduce subtle bugs, my preference would be to exclude this
- Deadlock. I don't know about this issue, but if the resolution involved SPI locking/unlocking then YES should be addressed.

... AND, it appears at first glance, that the library DOES collate all the interrupts into a single mask. I won't be surprised to see that they do mask on every 'transaction', whether it's 'your' interrupt or not.  ...
I haven't looked at the library but would expect ALL SPI registered interrupts would be masked on beginTransaction. Interrupts are non-predictable...

Kiwi.

EDIT: I just did a basic update of the RFM library to add beginTransaction() support, and will leave it running overnight with a gateway sketch just dumping my local RF network traffic (360 messages/hour), will report back if its still running in the morning.
Kiwi,
I'm not in a position to test Ethernet, I don't have the adapter, although I probably should (which adapter are you using?), but it will be a while before I'd be able to incorporate into this project. 

The deadlock code is now incorporated into the RFM69-Master but I 'feel' that it needs a bit more scrutiny. While it didn't change the SPI calls, it did change the timing WHEN those calls were made, including calls from within the RFM69 interrupt handler.

Regarding availability, I'm available for code review and changes, but probably won't be in a position to test for a few weeks since I'm traveling next Friday and will need a couple of days to prep for that.  I should be returning in about 2 1/2 weeks.

Re your test, did you replace the noInterrupts()/interrupts() calls in the select/unselect methods or simply add the begin/endTransaction() methods?

Finally, I'm pretty passionate about removing the unnecessary SPI calls via shadow registers, but I accept your position on this WRT this project and will address this change independently.

Thanks,
Tom

kiwisincebirth

  • Jr. Member
  • **
  • Posts: 69
Re: Moteino & W5100 ethernet SPI support
« Reply #11 on: January 11, 2015, 05:07:01 PM »
Hi there Tom, I am using the standard arduino Ethernet shield with W5100 chipset. From a Moteino  perspective this board chip are 3V3 compatible, so can be directly connected to the moteino pins.

Good news, is my gateway test worked, 10 hours of operation without issue that I can see. Next step is to include some Ethernet running at same time.

Will post code for review tonight, most change is in select() unselect()

Kiwi

kiwisincebirth

  • Jr. Member
  • **
  • Posts: 69
Re: Moteino & W5100 ethernet SPI support
« Reply #12 on: January 12, 2015, 06:17:24 AM »

Hi Felix

Hi Tom

I have made a small improvement to the RFM69 library, as discussed in the above thread.

In summary

The purpose of this change is to support improvements to the core Arduino SPI libraries that have been first released in Arduino 1.5.8

In summary the SPI changes improve reliability when there are multiple slave devices on the same SPI bus. This is by creating exclusion zones (transactions) around SPI interaction, managing different SPI configuration, and preventing interrupts where the interrupt handler uses SPI.

The full description of the changed SPI libraries is here

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

I can confirm the Ethernet libraries in 1.5.8 have been enhanced with these changes, so by upgrading the RFM libraries, then these two should work more reliably together, this is the main use case I want to support.

My changed RFM69 library is on GitHub. It is backwards compatible with traditional (legacy) SPI libraries, by #ifdef compiler directive.

https://github.com/kiwisincebirth/RFM69.

The main change in the RFM library is in the select() and unselect() methods, with a further small change in the init() method, where there interrupt handler is attached.

There is a small fix in readAllRegs() method to remove an unnecessary (duplicated) unselect() method call.

Lastly in the receiveDone() method, a call the interrupts() method has been added before every return statement. This is to ensure interrupts are fully enabled before the function ends. Previously this was implicit via a sub-functon calling unselect(), but this cannot be relied upon in the amended unselect(). The new SPI libraries disable (mask) only the interrupts that it knows need to be masked, rather then globally disable all interrupts.

It is lastly this last change that needs the most review, as I am not across the reason for disabling interrupts globally in this way, with the new SPI libraries this may not even be needed.

I am still testing it, and invite others to do the same. I will confirm once I have a amount of “multiple device” testing done. But the library ran successfully for 20 hours, running a gateway sketch, handling 360 messages an hour.

I have resisted the urge to tidy up other areas of the code (removing static declarations), concentrating on Just what IS needed. I know Tom has other cleanups and improvements he wishes to do.

I would like to consider this to be included for inclusion in the main library.

Regards

Kiwi

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #13 on: January 12, 2015, 08:49:09 AM »

<...snip>

https://github.com/kiwisincebirth/RFM69.

The main change in the RFM library is in the select() and unselect() methods, with a further small change in the init() method, where there interrupt handler is attached.

There is a small fix in readAllRegs() method to remove an unnecessary (duplicated) unselect() method call.

Lastly in the receiveDone() method, a call the interrupts() method has been added before every return statement. This is to ensure interrupts are fully enabled before the function ends. Previously this was implicit via a sub-functon calling unselect(), but this cannot be relied upon in the amended unselect(). The new SPI libraries disable (mask) only the interrupts that it knows need to be masked, rather then globally disable all interrupts.

It is lastly this last change that needs the most review, as I am not across the reason for disabling interrupts globally in this way, with the new SPI libraries this may not even be needed.

<snip...>
@Kiwi,
I just donwloaded your updates and will review today.  Re the global interrupts() call, ISTM this is unnecessary and, in the 'wrong' circumstances(ie a processor with more interrupt sources/levels), could create problems.  It seems like it would be unnecessary since you do pair begin/endtransactions and this should keep the keep the proper interrupt levels unmasked (assuming the new SPI library does ;-)

Again, I'll get back to you later today.
Tom

TomWS

  • Hero Member
  • *****
  • Posts: 1930
Re: Moteino & W5100 ethernet SPI support
« Reply #14 on: January 12, 2015, 05:55:14 PM »

<...snip>

https://github.com/kiwisincebirth/RFM69.

The main change in the RFM library is in the select() and unselect() methods, with a further small change in the init() method, where there interrupt handler is attached.

There is a small fix in readAllRegs() method to remove an unnecessary (duplicated) unselect() method call.

Lastly in the receiveDone() method, a call the interrupts() method has been added before every return statement. This is to ensure interrupts are fully enabled before the function ends. Previously this was implicit via a sub-functon calling unselect(), but this cannot be relied upon in the amended unselect(). The new SPI libraries disable (mask) only the interrupts that it knows need to be masked, rather then globally disable all interrupts.

It is lastly this last change that needs the most review, as I am not across the reason for disabling interrupts globally in this way, with the new SPI libraries this may not even be needed.

<snip...>
@Kiwi,
I just donwloaded your updates and will review today.  Re the global interrupts() call, ISTM this is unnecessary and, in the 'wrong' circumstances(ie a processor with more interrupt sources/levels), could create problems.  It seems like it would be unnecessary since you do pair begin/endtransactions and this should keep the keep the proper interrupt levels unmasked (assuming the new SPI library does ;-)

Again, I'll get back to you later today.
Tom
@Kiwi,
I looked over the code and everything looks good EXCEPT the changes in receiveDone(), which I'll get to in a moment.   

The #ifdef approach works well without a lot of messiness.  Good job and good approach in select().

Re receiveDone, the problem I see is that the 'transaction' is now more than just a SPI sort of thing.  receiveDone() disables interrupts so that it can perform an atomic test and execute on the mode and payloadlen, but then relies on this to be 'fixed' at the end of the unselect() on either the setMode() or receiveBegin() calls.  This MIGHT be ok, if the logic in the begin/endtransaction isn't state dependent, ie, that interrupts are already disabled.  Like your counting approach, this could be a problem if the transaction logic simply restores the 'previous' state.  I'm not sure the current version does, probably due to the simplicity of the processor interrupt logic, it just seems 'unbalanced' to me. 

If we accept that the endtransaction will re-enable interrupts, then the comments in the code indicate that receiveBegin() will 'unselect' (and therefore re-enable interrupts), hence your addition here is not necessary.

What are your thoughts?


Tom
PS: I'm really glad you fixed the spelling error on line 204, that really bugged me!   :D  JK!!!