Pages: [1]   Go Down
Author Topic: Ethernet software bug - SPI access & ext ints  (Read 2568 times)
0 Members and 1 Guest are viewing this topic.
0
Offline Offline
Newbie
*
Karma: 0
Posts: 4
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Hi guys.
I found a bug in the Ethernet software. The SPI read and write accesses are not protected from interrupts. When the Wiz5100 chip is being accessed and an external interrupt occurs from another device that uses the SPI, both SPI selects are going to be on resulting in a data collision.

The fix is to disable interrupts before the SPI select goes active and enable interrupts again after the SPI becomes inactive. This prevents two devices from having their SPI active at the same time.

Here is an example from w5100.cpp:

Code:
uint8_t W5100Class::read(uint16_t _addr)
{
  // DISABLE INTERRUPTS
  cli();

  setSS();  
  SPI.transfer(0x0F);
  SPI.transfer(_addr >> 8);
  SPI.transfer(_addr & 0xFF);
  uint8_t _data = SPI.transfer(0);
  resetSS();

  // ENABLE INTERRUPTS
  sei();

  return _data;
}

Akiba
FreakLabs
Logged

0
Offline Offline
Faraday Member
**
Karma: 7
Posts: 2526
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

The bug is not in the ethernet library - the bugs are in your ISR.

bug 1 - an ISR that seizes the SPI bus without checking to see if it's already in use.  That's just insane.

bug 2 - doing something in an ISR that's as complex as an SPI conversation.  Depending on actual complexity and bus speed, this may or may not be an issue.

Turning off interrupts is not something to be done lightly.

-j
Logged

0
Offline Offline
Newbie
*
Karma: 0
Posts: 4
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Heh heh...

>> bug 1 - an ISR that seizes the SPI bus without checking to see if it's already in use.  That's just insane.

The problem doesn't arise from the use of the SPI bus. The problem arises because the SPI select is active. There is no way to check for this unless  every possible SPI select pin that each shield uses is scanned.  

>> bug 2 - doing something in an ISR that's as complex as an SPI conversation.  Depending on actual complexity and bus speed, this may or may not be an issue.

SPI accesses are quite common in ISRs. You usually need to read the interrupt register to figure out where the interrupt source is, set a flag, and then disable it. There's no way to read the interrupt register and disable the interrupt source unless you do an SPI access.
Logged

Forum Administrator
Cambridge, MA
Offline Offline
Faraday Member
*****
Karma: 9
Posts: 3538
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Can you open a Google Code issue for this: http://code.google.com/p/arduino/issues/list?

Although I'm wondering if this needs to be in the library.  If most interrupt handlers don't use the SPI bus, should interrupts instead be disabled in the user's sketch before calling the conflicting functions?  Or do we need to disable interrupts before doing anything in the core (so they don't conflict with someone's ISR)?  I'm not saying this shouldn't be fixed, just trying understand when it's appropriate to disable interrupts in the core and when not.
Logged

0
Offline Offline
Newbie
*
Karma: 0
Posts: 4
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks. I added it to the Google code issue list.

Disabling interrupts in the user's sketch or in the user library would not fix the issue. There are two problems with the current code:

1) SPI accesses aren't atomic so if an SPI access to the Wiznet is going on and an interrupt occurs after the SPI data register is written to but before its read back, and there is an SPI access to a different IC inside the ISR, then the data read back after the ISR exits will be different than what it should be.

2) The Wiznet chip does not release MISO properly. If the Wiznet is being accessed and an interrupt occurs in which the SPI bus needs to be accessed for a different IC, the fact that the Wiznet is selected and that it holds MISO will create a data collision.

I've already experienced this and seen it hang my system. I've added the fix in the Ethernet library on my PC and it is now working well.
Logged

0
Offline Offline
Newbie
*
Karma: 0
Posts: 6
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Turning off interrupts is not something to be done lightly.

Could you explain this? I'm writing code to log RPM's read using interrupts to an SD card. If I shouldn't disable interrupts, what are the reasons?
Logged

0
Offline Offline
Faraday Member
**
Karma: 7
Posts: 2526
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

It depends entirely on who else is using the interrupts, and how long interrupts are disabled.

The timers use interrupts; if you depend heavily on precise timing, you are screwing up your clock while interrupts are disabled.  The code than increments the "clocks" for the arduino core is an ISR.

If interrupts are disabled for very long, you can lose data on the UART. An ISR moves data from the UART buffer into the Serial buffer.

NewSoftSerial depends on ISRs.  I'm sure there are others.

My comment was made partly with the wider Arduino audience in mind.  If you're a microcontroller developer you may be well aware of what the consequences are already.  If you are new to the whole microcontroller thing, you should be aware that there are consequences to disabling interrupts.


I should also repeat what I've said before: an ISR should be as small and fast as possible.  IMO the best way to use an ISR when the interrupt triggers a lot of code is to use the ISR to set a flag, and let the main body of code act on the flag.

-j
« Last Edit: October 22, 2010, 07:57:27 am by kg4wsv » Logged

Forum Administrator
Cambridge, MA
Offline Offline
Faraday Member
*****
Karma: 9
Posts: 3538
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

freaklabs: if you disabled interrupts before calling functions that accessed the Wiznet, wouldn't that prevent the conflicts (since an ISR couldn't fire while the Ethernet library was accessing the SPI bus or the Wiznet chip had control of MISO)?
Logged

0
Offline Offline
Newbie
*
Karma: 0
Posts: 4
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Although this is one solution, the issue then becomes one of interrupt latency. The question would then become how long the interrupts would remain off and it would depend on how the function is written. If this is the route taken, it would probably be best to include in the documentation somewhere that interrupts need to be turned off at the application level (in the user sketch) if you're using another interrupt driven SPI device. Otherwise, its quite difficult to trace the cause of the hang.

The main issue that I saw was that since the SPI accesses to the Wiznet were not protected from external interrupts, it basically means that the ethernet shield can't share the SPI bus with interrupt driven SPI devices.

In most embedded designs, the interrupts would be turned off only during the actual IC accesses since this would usually be short. This prevents locking out other peripherals that need interrupt servicing for long periods of time.

I can understand that there's probably some resistance to modifying the code to fix this issue. However this problem will become much more pronounced if the Arduino moves to integrate the Wiznet Ethernet IC on the actual MCU board. Then, it won't be a matter of shield compatibility but a matter of the Arduino Ethernet board being compatible with other devices.

So my point was that a lot of this can be avoided by simply protecting the SPI accesses to the Wiznet from external interrupts. My advice would be the same for other interrupt driven SPI devices, too. The SPI bus is meant to be shared and so everyone needs to play nicely on it.
Logged

Forum Administrator
Cambridge, MA
Offline Offline
Faraday Member
*****
Karma: 9
Posts: 3538
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

It's really resistance, just trying to understand the implications.  There's a lot of stuff in the various core functions and libraries that can interfere with certain things in interrupt handlers.  Which may mean we just need to disable interrupts in a lot of places in the core / libraries.  Which could have its own interrupt latency implications.  But probably it's best just to go ahead and do this.  
Logged

Offline Offline
Edison Member
*
Karma: 3
Posts: 1001
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Mellis wrote:
Quote
But probably it's best just to go ahead and do this.
I don't really think this example justifies the proposed modification. Disabling interrupts for the duration of an SPI read/write operation is likely to cause loss of millis, serial and other interrupts. As such this may very well cause more issues than it solves.

Also I am not convinced that accessing SPI hardware directly from within an interrupt handler is a good idea for any application and even less so for Arduino to encourage or support this as a general practice. A better approach might be to extend the SPI “library” and add access arbitration at this level. Extensions could include multiple CS lines, buffered input output, interrupt driven read/write. This would make SPI more like HardwareSerial and I2C/Wire and even allow read/write from within interrupt handlers (through the driver rather than directly).
Logged

0
Offline Offline
Shannon Member
****
Karma: 161
Posts: 10442
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Its actually quite a complicated issue since SPI hardware is shared between unrelated devices and the SPI handling itself might use the SPI interrupt (say one library is for a slow chip and selects a slow SCK setting, it might try and be friendly by being written in an asychronous style and have interrupt-driven SPI completion.)

Then its really difficult to disable interrupts because that library has effectively claimed the SPI hardware between the asynchronous setup call and the completion of the SPI transfer...  That really would upset timing and lose urgent interrupts.

So I guess the moral is don't use interrupt-driven SPI and try not to use SPI devices with only v. slow clock rate IF MIXING SPI DEVICES.  For slow devices use unique pins and bit bang them (could even setup a timer interrupt to schedule these if you want asynchronous library).

With a fast SPI (8MHz) the transfer is around 1us per byte, its OK to make this uninterruptible - you are v. unlucky to lose interrupts.  Even at 1MHz SPI clock its unlikely to raise issues. However see my last paragraph...

But the bottom line is that there are always going to be issues of resource-clashes between libraries because of the small finite number of hardware resources.

Another little side issue is the slowness of digitalWrite() for asserting/releasing the chip select pin for an SPI transfer - for speed you want direct pin access via PORTn registers, but this then binds the library to a fixed CS pin.
Logged

[ I won't respond to messages, use the forum please ]

Pages: [1]   Go Up
Jump to: