MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.18k stars 19.22k forks source link

SD card SPI conflicts #1226

Closed Grogyan closed 9 years ago

Grogyan commented 9 years ago

The SPI for the SD card assumes no other devices are connected to the same bus and so data conflicts occur because the code doesn't do a sanity check to ensure that the SPI bus to free to communicate over.

Noticeable with; RAMPS 1.4 RepRap discount full graphic display Max6675 thermocouple

boelle commented 9 years ago

related to https://github.com/MarlinFirmware/Marlin/issues/1227

boelle commented 9 years ago

one way to solve this one is to use the chip select pin.... if master do not use SPI it will not pull the chip select lines high... one line is needed for each device...

when master wants a temp reading it will pull high the chip select line a and talk to the thermocouple... when done it puts the line low...

http://en.wikipedia.org/wiki/Serial_Peripheral_Interface-bus

Grogyan commented 9 years ago

Yes, this can be done, however, it will need careful implementation to not screw up in progress communication. I did a very rough hack to see if it could resolve the issue, it did not, meaning that there are some underlying problems withing the SD card code implementation

Grogyan commented 9 years ago

http://elm-chan.org/docs/mmc/mmc_e.html Explains that during the initialization method, the SD card needs to be set to SPI MODE, default on power up is SD MODE. The above document also explains that this MODE selection is CS dependent during initialization. I don't know how it is implemented in Marlin, however, during power up, consideration for this step is essential.

barneycg commented 9 years ago

would this explain the times I've been seeing the sd card as showing files but when I try going into a sub directory it shows the files briefly and then they disappear ? Using a thermocouple hooked up with one of these http://reprap.org/wiki/ExtThermoCouple_1.0 boards

boelle commented 9 years ago

its a problem in general... SPI needs one line for each device that the master can use when it needs to talk to say the thermocouple... lets say it can talk to the LCD in general but then the thermocouple sends out data at the same time... why does this happen? because the CS (chipselect) line is not driven right

thinkyhead commented 9 years ago

If this is related to code in the SDFat library, we should have a look at the SDFat Beta repo and see if the latest library is doing the right thing, then incorporate those changes. (Assuming there is a "right thing" it can even do!)

Grogyan commented 9 years ago

I just got home, and doing a quick catch up, before I head out again in 20 minutes.

It's the whole SD card implementation, it's just a mess.

I just had a look at that repo, briefly, and looking through the code, it is doing some right thing I expect in SdSpiCard.cpp However, it does not check to see if any other devices are on the SPI bus. A fix could be applied to the function, bool SdSpiCard::isBusy() but this is just a guess.

I have been racking my head for a couple of weeks now, of some way, this/similar function could be adapted to read all the SPI devices automagically. I only know some very basic elements in code, not in any way would I consider myself adept at coding.

thinkyhead commented 9 years ago

The readme on SDFat beta seems optimistic with its mention of "Allow[ing] simultaneous use of hardware and software SPI with multiple cards. See the ThreeCard example." But I suppose it would be good to query the author @greiman and see what he thinks is possible.

daid commented 9 years ago

You need this patch to fix the MAX6675 code with SD cards: https://github.com/Ultimaker/Ultimaker2Marlin/commit/91c64e356348decee934b816269a25e7c42e8071

As this changes the MAX6675 read from interrupt to main loop. And as the SD is also called from the main loop the conflict is suddenly gone.

thinkyhead commented 9 years ago

I will roll a patch from that soon.

Grogyan commented 9 years ago

Still haven't got around to checking this merge out. We've just had a long weekend here in NZ and I happened to be away for it.

fmalpartida commented 9 years ago

Just a comment question. ISP devices assume that they can share a bus but there is a chip select (or Slave Select signal in the spec) that determines which device has been granted access to the bus. In general, the master controller arbitrates and uses the SS to use the different peripherals. In your configuration, do you have the same pin/signal driving the Chip Select of two devices? If so, this is something that you should review.

The SPI bus is not like the I2C where you can address devices in the bus, they require their unique CS signal.

fmalpartida commented 9 years ago

The Max6675 does not support being connected on a multi-slave shared CS configuration. As soon as the CS is asserted low and it sees a clock signal in the its SCK pin, it will start clocking data out. This will short anything on the MISO line.

Please indicate how the MAX6675 is connected and if it shares the CS with the SD card-reader.

boelle commented 9 years ago

yes i have said the same a few times.... i will try and find the schematics for the 2...

fmalpartida commented 9 years ago

In the patch shared by daid it is using a different pin for the chip select.

boelle commented 9 years ago

oh... yes... so this just needs testing then

boelle commented 9 years ago

@Grogyan had time to test if the included patch worked?

but basicly the issue here is that every SPI device must have an individual CS pin

AnHardt commented 9 years ago

@fmalpartida No. The patch does not use another pin. MAX code used to be in an interrupt - so could interrupt the communication with the sd-card an mess it up.. Now the Code for the MAX is execute in the main-loop. So they should interleave nicely. Yes. The MAX always had its own CS Pin.

fmalpartida commented 9 years ago

So we can close this one if someone can try it out. I don't have the needed setup.

boelle commented 9 years ago

@fmalpartida excatly... someone with the setup needs to test...

so if we dont see any activity in say 7 days i will close it

issues can always be reopened if the issue is still there

Grogyan commented 9 years ago

When I last checked, this problem was resolved. But there have been a number of changes which I will need to check out next weekend, probably, when I have time free. On 26 Apr 2015 11:21, "Bo Herrmannsen" notifications@github.com wrote:

@fmalpartida https://github.com/fmalpartida excatly... someone with the setup needs to test...

so if we dont see any activity in say 7 days i will close it

issues can always be reopened if the issue is still there

— Reply to this email directly or view it on GitHub https://github.com/MarlinFirmware/Marlin/issues/1226#issuecomment-96292695 .

boelle commented 9 years ago

will close this one since it has been resolved... we can always open a new issue if the the problem comes back

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.