Hi,
I tried to get mmc_spi with FATFS in ChibiOS master (3b1848e on github) running on an Arduino UNO, but it didn't work. First I discovered that the card wouldn't connect because the AVR SPI implementation sends 0 as a dummy variable when it receives and ignores data, whereas SD cards want to hear a 0xFF value. I defined a dummy send value, but maybe it's a better idea if the value of the dummy variable becomes a field in the SPI_config structure? This allowed the card to connect, but still, writing to the card failed. I couldn't exactly pinpoint the problem without a debugger, but I have a strong suspicion that the existing spi interrupt routine corrupts memory by writing to buffers outside of their range. The routine is also needlessly complex, so I rewrote it to fix the problem. I also replaced the send, receive and ignore functions with macros which saves a few bytes of program memory. Attached is a patch with the changes to the AVR/hal_spi_lld.* files and a test project for your review.
Kind regards, Marcello.
bugs in AVR/hal_spi_lld.* Topic is solved
-
- Posts: 9
- Joined: Tue Apr 05, 2016 2:52 pm
- Location: Torgon, Switzerland
- Been thanked: 1 time
bugs in AVR/hal_spi_lld.* Topic is solved
- Attachments
-
- ChibiOS.patch.zip
- (9.55 KiB) Downloaded 260 times
- Giovanni
- Site Admin
- Posts: 14455
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
- Giovanni
- Site Admin
- Posts: 14455
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: bugs in AVR/hal_spi_lld.*
I have no problems using the SPI driver so far so the modifications to the interrupt routine are not needed IMO. I checked the SPI drivers for the STM32 port and they send 0xFF as dummy byte. I think the AVR port should to this as well.
Marco
Marco
- Giovanni
- Site Admin
- Posts: 14455
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: bugs in AVR/hal_spi_lld.*
Just to know if I should mark it as "solved", there is an entry of this topic also in the bug reports forum.
Giovanni
Giovanni
-
- Posts: 9
- Joined: Tue Apr 05, 2016 2:52 pm
- Location: Torgon, Switzerland
- Been thanked: 1 time
Re: bugs in AVR/hal_spi_lld.*
Hurray! Somebody finally took the time to have a look at my solution to a bug which I posted 8 months ago. You took your time guys!
And yes, I think my initial post is still relevant:
Apart from (unknowingly) solving the "Write Collision" bug that Marco discovered, the solution I propose has some other advantages:
* It solves another bug: spiIgnore() always sends out 1 byte too much (this causes problems in mmc_spi).
* the code of the interrupt routine is considerably simpler, smaller and therefore faster. I also think it's easier to read and understand.
* the driver uses slightly less RAM memory.
* overall the code is slightly smaller; also because spiRead(), spiIgnore() and spiSend() are now macros that are translated into different invocations of spiExchange()
With regard to the dummy variable, I think it needs to become a mandatory config field. Different spi-devices might have different requirements regarding the dummy variable. If you need to receive an array of 200 bytes from a particular device and the dummy variable is not correct, you end up doing a spiExchange() with a txBuf of 200 dummy values.
I included the hal_spi_lld files once again. Let me know if you want to use them, than I will clean up the code and fix comments where necessary.
Kind regards,
Marcello
And yes, I think my initial post is still relevant:
Apart from (unknowingly) solving the "Write Collision" bug that Marco discovered, the solution I propose has some other advantages:
* It solves another bug: spiIgnore() always sends out 1 byte too much (this causes problems in mmc_spi).
* the code of the interrupt routine is considerably simpler, smaller and therefore faster. I also think it's easier to read and understand.
* the driver uses slightly less RAM memory.
* overall the code is slightly smaller; also because spiRead(), spiIgnore() and spiSend() are now macros that are translated into different invocations of spiExchange()
With regard to the dummy variable, I think it needs to become a mandatory config field. Different spi-devices might have different requirements regarding the dummy variable. If you need to receive an array of 200 bytes from a particular device and the dummy variable is not correct, you end up doing a spiExchange() with a txBuf of 200 dummy values.
I included the hal_spi_lld files once again. Let me know if you want to use them, than I will clean up the code and fix comments where necessary.
Kind regards,
Marcello
- Attachments
-
- AVR_hall_spi_lld.zip
- (4.91 KiB) Downloaded 235 times
- Giovanni
- Site Admin
- Posts: 14455
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: bugs in AVR/hal_spi_lld.*
Hi Marcello,
I am not directly involved with the AVR port and the maintainer are volunteering their time for the AVR sub-project, you can't expect an immediate reaction but bug reports usually don't get lost.
Giovanni
I am not directly involved with the AVR port and the maintainer are volunteering their time for the AVR sub-project, you can't expect an immediate reaction but bug reports usually don't get lost.
Giovanni
-
- Posts: 359
- Joined: Sat Jan 07, 2012 6:22 pm
- Location: Brazil
- Has thanked: 1 time
- Been thanked: 20 times
- Contact:
Re: bugs in AVR/hal_spi_lld.*
I applied the patch with some style changes. I have no AVR board to test it right now but the patch looks ok. If you can re-test and post updates, please do.
Fabio Utzig
Fabio Utzig
-
- Posts: 9
- Joined: Tue Apr 05, 2016 2:52 pm
- Location: Torgon, Switzerland
- Been thanked: 1 time
Re: bugs in AVR/hal_spi_lld.*
Hi Fabio,
Thanks for applying the patch! I have re-tested it and it works fine.
Kind regards, Marcello
Thanks for applying the patch! I have re-tested it and it works fine.
Kind regards, Marcello
Who is online
Users browsing this forum: No registered users and 17 guests