Moteino & W5100 ethernet SPI support / SPI_HAS_TRANSACTION

[TomWS]

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

@Kiwi,
is the W5100 shield you're using have the Interrupt jumper (a little solder pad) bridged or open?  Do you know if the ethernet library is making the SPI.usingInterrupt() call (and which interrupt number it's using)?

Tom

[kiwisincebirth]

@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

My belief is the receive done method needs to be changed. Either the interrupts() removed, or balanced as I have done in GitHub

The reason it has to change is that the endtransaction() method in SpI library do not reenable  interrupts. you can see more clearly what is happening in the begintransaction() spi method. This method is design to mask interrupts for the interrupt pins that have been registered via usinginterrupts(), but not disable interrupts entirely. This allows timer0 millis() to continue to function.

Thus if we rely on end transaction to reenable interrupts then this won't happen.

The approach that I have taken, assumes that the blocking of interrupts is necessary for the atomic locking to occur, if not it should be removed.

Since the disabling of interrupts and masking of interrupts i think are  done on different registers there shouldn't be an issue. Also so long as the two mechanisms happen in matching pairs, don't overlap instead one surrounds the other we should be ok,

I.e.  Spi end transaction is called and restores the interrupt state when begintransaction was called, then the receive done method enables the interrupts

Hope the makes sense.

Ps I am away from home for a couple of days, so can't check Ethernet board

[TomWS]

<...snip>My belief is the receive done method needs to be changed. Either the interrupts() removed, or balanced as I have done in GitHub

The reason it has to change is that the endtransaction() method in SpI library do not reenable  interrupts. you can see more clearly what is happening in the begintransaction() spi method. This method is design to mask interrupts for the interrupt pins that have been registered via usinginterrupts(), but not disable interrupts entirely. This allows timer0 millis() to continue to function.

Thus if we rely on end transaction to reenable interrupts then this won't happen.

The approach that I have taken, assumes that the blocking of interrupts is necessary for the atomic locking to occur, if not it should be removed.

Since the disabling of interrupts and masking of interrupts i think are  done on different registers there shouldn't be an issue. Also so long as the two mechanisms happen in matching pairs, don't overlap instead one surrounds the other we should be ok,

I.e.  Spi end transaction is called and restores the interrupt state when begintransaction was called, then the receive done method enables the interrupts

Hope the makes sense.

Ps I am away from home for a couple of days, so can't check Ethernet board

Your explanation is very clear.  I understand your point.  I'm hoping to have some time tomorrow to look at it and will reply once I have a sense of what is there (or more questions).

Thanks for answering so thoroughly.

Tom

[kiwisincebirth]

@Kiwi,
is the W5100 shield you're using have the Interrupt jumper (a little solder pad) bridged or open?  Do you know if the ethernet library is making the SPI.usingInterrupt() call (and which interrupt number it's using)?

Tom

I have just read some forum posts, and delved into the Arduino library itself. And NO interrupts are not supported by the standard ethernet library, the bridging pin you refer to could be used ONLY with a redeveloped library. 

[TomWS]

@Kiwi,
is the W5100 shield you're using have the Interrupt jumper (a little solder pad) bridged or open?  Do you know if the ethernet library is making the SPI.usingInterrupt() call (and which interrupt number it's using)?

Tom

I have just read some forum posts, and delved into the Arduino library itself. And NO interrupts are not supported by the standard ethernet library, the bridging pin you refer to could be used ONLY with a redeveloped library.

Great!  Thanks for looking into this.  I should be able to work on the transaction conversion of RFM69 today.

Tom

[kiwisincebirth]

Hi Tom, Have you made any progress ?

[TomWS]

@Kiwi, sorry, no I haven't.  I've been travelling this week.  Hopefully I can get to it on my Sunday...

Tom

[TomWS]

@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

My belief is the receive done method needs to be changed. Either the interrupts() removed, or balanced as I have done in GitHub

The reason it has to change is that the endtransaction() method in SpI library do not reenable  interrupts. you can see more clearly what is happening in the begintransaction() spi method. This method is design to mask interrupts for the interrupt pins that have been registered via usinginterrupts(), but not disable interrupts entirely. This allows timer0 millis() to continue to function.

Thus if we rely on end transaction to reenable interrupts then this won't happen.

The approach that I have taken, assumes that the blocking of interrupts is necessary for the atomic locking to occur, if not it should be removed.

Since the disabling of interrupts and masking of interrupts i think are  done on different registers there shouldn't be an issue. Also so long as the two mechanisms happen in matching pairs, don't overlap instead one surrounds the other we should be ok,

I.e.  Spi end transaction is called and restores the interrupt state when begintransaction was called, then the receive done method enables the interrupts

Hope the makes sense.

Ps I am away from home for a couple of days, so can't check Ethernet board

@Kiwi, I have to tell you that I scratched my head long and hard on this and, in the end, have come full circle.   I think you're completely right about using noInterrupts/interrupts() in receiveDone().   

I really struggled with this because strictly speaking, receiveDone() should only need to be atomic with respect to its own interrupt, which, in this case, is the same as the SPI interrupt.  Unfortunately there are two problems with this 'purist' view, the first being that a 'universal' receiveDone() would have to do the same work as SPI.usingInterrupt() to generate the appropriate mask and a quick look at the code needed to parse SPI.usingInterrupt() for all possible processors makes your head spin!

The second aspect is more a 'conservative' concern that it may not be JUST SPI interrupts we need to block in this case.  It may be that ANY interrupts used by the Moteino application may need to be blocked at this point and noInterrupts() does just that.  And, as you already concluded, since the setMode() and receiveBegin() don't restore global interrupt (if using SPI transactions) then the extra interrupts() calls are necessary in receiveDone().  However, since these are ONLY needed in the SPI transaction case, I think we should use a conditionally defined INTERRUPTS() (or conditionally execute interrupts()) in these two places.

Do you agree?

Once this is resolved, I think what you have is great and should be submitted to the 'committee' (AKA Felix) via pull request.

Tom

[kiwisincebirth]

...

I really struggled with this because strictly speaking, receiveDone() should only need to be atomic with respect to its own interrupt, which, in this case, is the same as the SPI interrupt.  Unfortunately there are two problems with this 'purist' view, the first being that a 'universal' receiveDone() would have to do the same work as SPI.usingInterrupt() to generate the appropriate mask and a quick look at the code needed to parse SPI.usingInterrupt() for all possible processors makes your head spin!

The second aspect is more a 'conservative' concern that it may not be JUST SPI interrupts we need to block in this case.  It may be that ANY interrupts used by the Moteino application may need to be blocked at this point and noInterrupts() does just that.  And, as you already concluded, since the setMode() and receiveBegin() don't restore global interrupt (if using SPI transactions) then the extra interrupts() calls are necessary in receiveDone().  However, since these are ONLY needed in the SPI transaction case, I think we should use a conditionally defined INTERRUPTS() (or conditionally execute interrupts()) in these two places.

Do you agree?

Once this is resolved, I think what you have is great and should be submitted to the 'committee' (AKA Felix) via pull request.

Tom

I have made the changes as you suggested, and created a pull request.

I did this because a non 1.5.8 user won't see any changes at compile time, It is the simplest step to get adoption of the change, i.e. minimal impact to existing users, is proven to work, and calls interrupts() once when completing receiveDone(). An I do appreciate the pragmatism of this approach.

One of my concerns is readability and future maintainability of the code. If you look at it now it may not be obvious why some times interrupts() is called and sometimes it is not, and without understanding how select() and unselect() work differently with and without 1.5.8 SPI this may not be obvious, and lead to mistakes.

The code as it stands creates a subtle dependancy between unselect() and receiveDone(). Always calling interrupts() in receiveDone() breaks this dependancy, and make the code easier to read and more reliable.

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.

[Felix]

Got the pull request, thanks.
These are high risk changes and they will need to be well tested before merging.

[damadmai]

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.

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?

And just delete that unnecessary unselect entirely.

Daniel

[scott216]

How are you guys dealing with the issue that the Moteino uses pin D10 for the radio SS and so does the stock Arduino Ethernet library?  I have a modified Ethernet.h/cpp and w5100.h/cpp files to handle this, but I didn't see any mention of it in the conversation and was curious how you handle it.

[Felix]

This is how:

void RFM69::setCS(uint8_t newSPISlaveSelect)

[scott216]

This is how:
void RFM69::setCS(uint8_t newSPISlaveSelect)

Nice.  I didn't know about that function.  But wouldn't Moteino's D10 pin be hard-wired to the CS pin on the RFM69 module.

[Felix]

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