Moteino & W5100 ethernet SPI support / SPI_HAS_TRANSACTION

[scott216]

You are correct sir. You move one of the pins to something else

I'm confused.  The wiring from pin to the RFM module would be a copper trace on the Moteino PCB.  I don't see how you would change anything.

[Felix]

Cut, resolder to another pin using thin hookup wire, add 1 line of code, done.

[scott216]

Cut, resolder to another pin using thin hookup wire, add 1 line of code, done.

ok, I see.  I was thinking there might be a less invasive option.

[kiwisincebirth]

ok, I see.  I was thinking there might be a less invasive option.

I have followed your approach and moved the Ethernet CS Pin to a different location, and downloaded the updated libraries you referenced.

Kiwi

[TomWS]

<...snip>
I question the motivation for excluding the extra line of code, All this to save a few extra bytes in flash, and clock cycles at runtime, I know this is a constrained platform, but really...

I come from a professional Java coding background, in a large scale system this sort of thing wouldn't be accepted because across many 10's or 100's of thousands of line it is impossible to understand manage.

Kiwi.

The motivation wasn't saving code, but an effort to try to keep the calls to noInterrupts()/interrupts() balanced.  If these ever permit nested hierarchical calls, then an unbalance isn't a good thing.  I'm with you on struggling with the dependency, but, as you say, one step at a time...

Thanks for your work.  Hopefully I'll be able to join in the testing soon, right now I'm butt deep in my gateway code...

Thanks for the W5100 info.  I've ordered some of the 'Mini Shields' (for my gateway) and will be able to test once the 'slow boat' arrives...

Tom

[kiwisincebirth]

Hi kiwisincebirth!

As it is not done in the implementation of beginTransaction() and endTransaction() I would suggest placing the lines which save and restore the current SPI settings in front of the #ifdefs to not interfere with other code that accesses SPI and relies on a previous configuration.

I have made this change. I am not sure why the the new SPI library doesn't do this in the begin/end transaction, I am guessing the assumption is that SPI will be configured before each access to SPI

You have a typo - "doest" but i don't know if you mean does or doesn't as beginTransaction() saves and endTransaction() restores the SREG and therefore the interrupt bit in SREG is the same as before an SPI access.
In the comments it says that "New SPI Library doesn't disable interrupts" but they are enabled here and here?

I have updated the code to more correctly document the change in the receiveDone() method. However it is important to note that the interrupts are disabled in this method to gain an atomic lock in the code, not specifically about locking SPI access. It just so happened that the completion of SPI calls interrupts are re-enabled by unselect(), thus nointerrupts() didn't need to be called. But with the new SPI library this is not guaranteed to be true, hence noInterrupts() needs to be explicitly called.

RE SPI beginTransaction(). The first couple of lines of code save SREG (to local variable) and disable interrupts. But you should also note that the SREG is restored (a few lines latter) if EIMSK can be used. Thus interrupts are not disabled for the duration of the SPI transaction. endTranaction() is similar in function creating a atomic lock, then restoring SREG.

And just delete that unnecessary unselect entirely.

Done.

The motivation wasn't saving code, but an effort to try to keep the calls to noInterrupts()/interrupts() balanced.  If these ever permit nested hierarchical calls, then an unbalance isn't a good thing.  I'm with you on struggling with the dependency, but, as you say, one step at a time...

To that end I have removed the #ifdefs out of my receiveDone(), and always call interrupts() to balance the noInterrupts() at the the top of this method. This archives matching calls, and removes the dependancy on the select() and unselect() entirely.

Note: Previously it wasn't balanced i.e. recieveDone() calls noInterupts(); select() calls noInterrupts(); unselect calls interrupts(); so 2 x Off and 1 x On.

ALSO. I think the best improvement would be simply to save and restore SREG, where interrupts are enabled and disabled in the code. e.g. select() and unselect()

This would solve a potential problem in interruptHandler(). When interruptHandler() calls a method that requires SPI, that method calls select() does it work then calls unselect(), unselect() then calls interrupts(). Which I assume means for the remainder of the interruptHandler() interrupts can occur.

I am not sure if this is a bug, and easily fixed by saving and restoring SREG in select() unselect(), OR it could be a FEATURE, meaning that other interrupts can occur (e.g. Timer0) during the rest of the interrupt, I don't know how long interruptHandler() takes to execute.

Thanks for your work.  Hopefully I'll be able to join in the testing soon, right now I'm butt deep in my gateway code...

Thanks for the W5100 info.  I've ordered some of the 'Mini Shields' (for my gateway) and will be able to test once the 'slow boat' arrives...

Tom

I have committed my small changes back to my GitHub Fork, not sure if I need to do anything else to make them visible in the Pull request.

This is quite a long post, and I hope it makes sense. Tom Good luck with your gateway

Regards
Kiwi

[TomWS]

<...snip>

The motivation wasn't saving code, but an effort to try to keep the calls to noInterrupts()/interrupts() balanced.  If these ever permit nested hierarchical calls, then an unbalance isn't a good thing.  I'm with you on struggling with the dependency, but, as you say, one step at a time...

To that end I have removed the #ifdefs out of my receiveDone(), and always call interrupts() to balance the noInterrupts() at the the top of this method. This archives matching calls, and removes the dependancy on the select() and unselect() entirely.

Note: Previously it wasn't balanced i.e. recieveDone() calls noInterupts(); select() calls noInterrupts(); unselect calls interrupts(); so 2 x Off and 1 x On.

ALSO. I think the best improvement would be simply to save and restore SREG, where interrupts are enabled and disabled in the code. e.g. select() and unselect()

This would solve a potential problem in interruptHandler(). When interruptHandler() calls a method that requires SPI, that method calls select() does it work then calls unselect(), unselect() then calls interrupts(). Which I assume means for the remainder of the interruptHandler() interrupts can occur.

I am not sure if this is a bug, and easily fixed by saving and restoring SREG in select() unselect(), OR it could be a FEATURE, meaning that other interrupts can occur (e.g. Timer0) during the rest of the interrupt, I don't know how long interruptHandler() takes to execute.

<snip...>

Your comments make sense with respect to the already unbalanced nature of the noInterrupts()/interrupts() calls and I agree with the change.  I can't look at the code right now so can't comment on the SREG discussion although I will say I'm concerned about the possible re-entrance of interruptHandler().  I'll try to get to it later today. 

Tom

[damadmai]

Hi kiwisincebirth!

Your new commits are visible in the Pull Request.
Thank you for implementing my suggestions.

I have another suggestion regarding the current state.

https://github.com/damadmai/RFM69/commit/69620f280b5ba8eca991c107097f36a3d9d50d6b

As it might be possible that an interrupt occurs after saving the SPCR and SPSR I think it would be better if the call to noInterrupts() stays where it was like in select().
(and SREG needs only be saved in _SREG if we don't have SPI_HAS_TRANSACTION )

And as endTransaction doesn't change SPCR or SPSR unselect() could be written for the same reason as in my commit.

In the Header file it might be necessary to do some #ifdef

In this line with your changes the comment "enables interrupts" would not be true any more because unselect does not re-enable interrupts even if they were not enabled before!
https://github.com/LowPowerLab/RFM69/pull/25/files#diff-2d1040d890d26d4ac6877f9269b3e32fR367
So just please remove this comment.

Thats a comlex change. I just thought two hours about it...

Does anyone know if there is a better way on GitHub to suggest changes to an Pull Request like my manual approach?

Daniel

[TomWS]

Daniel, I've added a comment to your commit.  Net: I agree with your change and also added that I think a SPIsettings variable, _settings, should be conditionally added and initialized once, rather than calculate the value on each beginTransactions() call.

Tom

[TomWS]

I've updated the SPIFlash library to include the SPI Transaction support and submitted a pull request.

I've tested this on V1.5.8 and V1.6.0 and don't think I did anything bone headed like the last pull request 

My tests included the updated RFM69 library from kiwi... (with my own transmit power control mods added) as well as simultaneous use with the V1.6.0 SD library which also includes SPI Transaction support.   I've not seen a single hiccup after a lot of traffic, including multiple over the air program updates.  With today's updates to the RFM69 library, I think its good to go.

Tom

[Felix]

This is an ultra major potentially breaking change and it's very labor intensive to test it properly.
I'm not sure when I will get to this.
Also I am not sure package deal SPI transactions are the best thing either, as we've seen with other things bundled with Arduino. SPI "transactions" were already implemented in the library in a raw way.

[kiwisincebirth]

Hi kiwisincebirth!

Your new commits are visible in the Pull Request.
Thank you for implementing my suggestions.

I have another suggestion regarding the current state.

https://github.com/damadmai/RFM69/commit/69620f280b5ba8eca991c107097f36a3d9d50d6b

As it might be possible that an interrupt occurs after saving the SPCR and SPSR I think it would be better if the call to noInterrupts() stays where it was like in select().
(and SREG needs only be saved in _SREG if we don't have SPI_HAS_TRANSACTION )

And as endTransaction doesn't change SPCR or SPSR unselect() could be written for the same reason as in my commit.

In the Header file it might be necessary to do some #ifdef

In this line with your changes the comment "enables interrupts" would not be true any more because unselect does not re-enable interrupts even if they were not enabled before!
https://github.com/LowPowerLab/RFM69/pull/25/files#diff-2d1040d890d26d4ac6877f9269b3e32fR367
So just please remove this comment.

Thats a comlex change. I just thought two hours about it...

Does anyone know if there is a better way on GitHub to suggest changes to an Pull Request like my manual approach?

Daniel

I have corrected my branch, assume you can see this. Note: I simplified the code with a single #if compiler directive in each of the select() and unselect() methods. IMHO this make it more readable, since you can now clearly identify the two distinct blocks of code. The original code is now back in its unmodified (except fro SREG) state.

[kiwisincebirth]

Daniel, I've added a comment to your commit.  Net: I agree with your change and also added that I think a SPIsettings variable, _settings, should be conditionally added and initialized once, rather than calculate the value on each beginTransactions() call.

Tom

Hi Tom, From the Article

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

The new SPISettings is a special data type, just for describing SPI clock, data order and format.  For fixed settings, you can use beginTransaction(SPISettings(clock, order, format)), and the compiler will automatically inline your fixed settings with the most optimal code.  For user controlled settings, you can create a variable of SPISetting type and assign it based on user choice, which you don't know in advance.  This allows a very efficient beginTransaction(), because the non-const settings are converted to an efficient form ahead of time.

This statement in the article was the reason I put the SPISettings inside the call to beginTransaction()

Kiwi

[kiwisincebirth]

This is an ultra major potentially breaking change and it's very labor intensive to test it properly.
I'm not sure when I will get to this.
Also I am not sure package deal SPI transactions are the best thing either, as we've seen with other things bundled with Arduino. SPI "transactions" were already implemented in the library in a raw way.

I can understand your caution.

FYI I have been running my Gateway Node (Motino and Ethernet Shield) for the past 2-3 weeks. In that time I can say that the gateway hasn't crashed, but I can't confirm I havent lost a message. I have of course been re-flashing it with the latest libraries as changes are made, and with Arduino 1.6.0 when it came out.  The gateway doesn't transmit (it only receives) over RFM radio, and relays to MQTT via the ethernet hardware.

So from my perspective I am good I am fully working (have retired my RaspPi), hopefully in time will post a more complete description of my project to the forum.

I will leave it with you when and if you want to merge it back in, and keep it updated with other changes in you main branch, and suggestions from the community.

Stay tuned, a much more usable and more automatic interface for Moteino based stuff is coming from Low Power Lab. It will only require a few files.

Cant wait.

Kiwi.

[TomWS]

Daniel, I've added a comment to your commit.  Net: I agree with your change and also added that I think a SPIsettings variable, _settings, should be conditionally added and initialized once, rather than calculate the value on each beginTransactions() call.

Tom

Hi Tom, From the Article

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

The new SPISettings is a special data type, just for describing SPI clock, data order and format.  For fixed settings, you can use beginTransaction(SPISettings(clock, order, format)), and the compiler will automatically inline your fixed settings with the most optimal code.  For user controlled settings, you can create a variable of SPISetting type and assign it based on user choice, which you don't know in advance.  This allows a very efficient beginTransaction(), because the non-const settings are converted to an efficient form ahead of time.

This statement in the article was the reason I put the SPISettings inside the call to beginTransaction()

Kiwi

Ah, very good.  Thank you very much for pointing this out, I had not realized that - very clever indeed!  I used a variable in the SPIFlash update, perhaps I should change it?  On the other hand, I noticed that a variable was used in the IDE 1.6.0 SD library - maybe they don't trust the compiler as much as the writers of that comment?

Maybe some others should chime in on this.  I'll try to run some experiments to see I can look at the code produced to verify that they do, indeed, use constants as opposed to re-calculating the settings each time.

Regardless, I now understand your reasoning and withdraw my suggestion.  Thanks!

Tom