New SdFat library (with and without DMA)

A simple analysis:
SPI.cpp
original function :

void SPIClass::beginTransaction(uint8_t pin, SPISettings settings)
{
#ifdef SPI_DEBUG
Serial.println("SPIClass::beginTransaction");
#endif
//_SSPin=pin;
//pinMode(_SSPin,OUTPUT);
//digitalWrite(_SSPin,LOW);
setBitOrder(settings.bitOrder);
setDataMode(settings.dataMode);
setClockDivider(determine_baud_rate(spi_d, settings.clock));
begin();
#if 0
// code from SAM core
uint8_t mode = interruptMode;
if (mode > 0) {
if (mode < 16) {
if (mode & 1) PIOA->PIO_IDR = interruptMask[0];
if (mode & 2) PIOB->PIO_IDR = interruptMask[1];
if (mode & 4) PIOC->PIO_IDR = interruptMask[2];
if (mode & 8) PIOD->PIO_IDR = interruptMask[3];
} else {
interruptSave = interruptsStatus();
noInterrupts();
}
}
uint32_t ch = BOARD_PIN_TO_SPI_CHANNEL(pin);
bitOrder[ch] = settings.border;
SPI_ConfigureNPCS(spi, ch, settings.config);
//setBitOrder(pin, settings.border);
//setDataMode(pin, settings.datamode);
//setClockDivider(pin, settings.clockdiv);
#endif
}

Steve it seems that putting them in that order makes sense.
I don’t think anyone has put much effort in rewriting examples except if they didn’t work. There is loads of code that’s really old and difficult to keep track on who did what and why.

I would suggest that in the examples that you have corrected and verified, open a new thread to comment, in case other people can test them to have more reassurance, and then send a pull request to Roger.
We have Roger always busy doing changes, while he still needs to tend his actual obligations, so anything we can do to save his time is good.
Sending a pull request saves him from having to go to those files and rewrite them personally.

About the sdFat library, Greyman has it as a work in progress in github, and has commented that there is loads of old code he needs to go thru. For changes that improve his library stability, I would suggest you tell him, so he is aware, and he can even add them to the library if he sees the benefit.

My copy was meant to be only temporary, I actually need to send him a new pull request as the one I initially sent him had the wrong files by accident.

Now, about removing begin() from those 3 functions, the question is, does the official arduino library call begin on those?
if it does, it is very likely that removing it in our core will break compatibility with other libraries that may not call begin in the order you suggest, because they rely on those functions calling it.
As Roger commented, those functions in our core set a parameter, but do not apply it to the hardware, so unless begin is called by either the function or the code using it, the setting would not be effectively applied.
If you want to remove begin() from those, I would suggest that you check what part of begin() applies each of those settings to the hardware, and modify those functions so they actually apply the setting besides saving the parameter.

OK, so now it looks that the things are not so simple as I thought.
I wanted to be conform to and therefore read about the SPI reference on Arduino reference homepage. A total mess…
There is written:
setClockDivider()
Description
This function should not be used in new projects. Use SPISettings with SPI.beginTransaction() to configure SPI parameters.

Now, should we keep backwards compatibility to older libs with these three small extra functions or not?

In my opinion we should adapt the new interface to SPI lib, via beginTransaction() and SPISettings.
The users could always easily adapt their sketches to be conform to latest SPI changes, but not the custom libs.
However, I think most custom libs have already switched to the transaction-based API, so it seems normal to follow that way.

But going one step further, the site https://www.arduino.cc/en/Reference/DueExtendedSPI suggest a special usage form, without beginTransaction(), by simply using the pin number as parameter for transferring data. Hmmm.
On the top, the beginTransaction() example (https://www.arduino.cc/en/Tutorial/SPITransaction) uses again another approach, suggesting the user to manage the settings by its own.

Now, which example should we take as reference? All of them should be supported?
This is important to decide because the development depends on this decision.

Now, should we keep backwards compatibility to older libs with these three small extra functions or not?

There are so many legacy libraries and so much code out there which uses the old functions, we can’t simply not support them.

Re: Due Extended functionality

I think this is very SAM specific, where they can move the hardware CS pins to alternates.

We can’t even use the hardware CS because of issues with the way ST think it should work, i.e as has been posted in this thread, most people have a problem with the way the hardware CS on the STMF103 etc work, and end up just using GPIO for CS

So there is no point in attempting to mimic Due specific stuff..

IMHO, the Due never caught on. I have 2 of them, and have never used either. As far as I can tell not many people use the Due.
The new Arduino Zero (which also uses a SAM type MCU) may be a different story, only time will tell
But I’m not sure if it supports the Due Extended SPI either.. As its a SAMD chip, so may not have the same capabilities as the Due.

Re: Transactional SPI

I made a stab at adding the transactional SPI code (as it wasn’t in the original LeafLabs code)
But I’ve never thoroughly tested it, because not many libs use it.

I just did a google search to see how may places SPI.begintransaction( occurs e.g.

Search google for

"SPI.beginTransaction("

… in terms of what people are likely to want. The old style far outweighs the transactional.

The entire transactional approach was implemented by Paul for Teensy 3.x Now, we hashed this once regarding the STM32duino months ago and while many libs (Adafruit’s and Teensy3.x) were updated, most non-commercially distributed were not. So, we have a legitimate concern. If we change the core approach … things will break. “Working vs Proper but broken” is little use to noobies. I think this is one of those situations where “improvements” would bite someone in the arse.

I have not looked at Paul’s stuff in ages…
http://www.pjrc.com/teensy/td_libs_SPI.html

Ray

OK, so let’s go the conservative way, at least with regard to the SPI lib, which is used in many custom libs.
Following Roger’s idea to toggle spi_master_enable/disable, it seems that this works. The glitches are still there but in very reduced form, so it seems that they are not disturbing anymore.

Regarding SPI 2, I reported already that the data reading was working but the writing not. Well, it seems to be a bug in SdSpiSMT32F1.cpp.
https://github.com/victorpv/SdFat/blob/ … TM32F1.cpp
In function “void SdSpi::send(const uint8_t* buf , size_t n)”, lines 92-94, there is SPI 1 device used as fixed parameter, so there is no chance for SPI 2 to work.
if (spi_is_rx_nonempty(SPI1)) {
uint8_t b = spi_rx_reg(SPI1);
}


i suppose the classic way would be to fork it, or as in the title newSPI and NewSd* or add _STM as per Adafruit_GFX

i can’t see many libraries being ported back to AVR, but if required as multi-platform we already test
for _STM32F1_ etc.

stephen

Regarding those 3 lines, I don’t completely remember anymore.
They were in the version for the SAM, and maybe in the version for the Arduino. I am sure I tested with and without them, but left them just to avoid deviating much from the SAM version.

All it does is just empty the last byte from the RX buffer.
I would have to go and read the SPI library and the datasheet to see how the actual transfer function works, but I dont have time.

Now this is what I remember, although may be wrong. When we do a transfer, the MCU will be reading incoming bytes to the RX buffer.
Once we write the last byte to the TX buffer and the peripheral sends it out, it still reads one more byte to the RX buffer.
If the SPI library write function manages that correctly so that last incoming byte does not go as the first read value the next time you do a transfer, all is good. But if the byte is instead left waiting in the RX buffer, and the SPI functions read and return it as if it was the first read value, then it needs to be discarded.

I believe at some point I took it out, then I added it again because of some issues, that most likely were not really caused by that, but could be. You can either test with and without it, or check the SPI library to make sure a byte read in the last previous transfer is not going to be provided as the first read in the next transfer.

EDIT:
The fact that those lines using SPI1 prevent the write to function correctly in SPI2, seems to indicate those lines are actually necessary. They just need to use the correct port thought and not just read from SPI1 always.

EDIT2:
Just had a look at the write function in SPI.cpp
It goes to read from the RX buffer if there is anything waiting on it.
I believe the datasheet explains that when you send a byte, the MCU will always read what’s coming from the MISO line. In which case any read or transfer may leave a byte waiting in the RX buffer which could wrongly be picked up by the next read. I think it would be better to actually modify the functions that send data out to make sure they leave an empty RX buffer on exit.

I see I did that with the dmaTransfer functions, but did not add it to the existing ones. I would think it should be added to the write and transfer functions that do not have it yet.

The fact that those lines using SPI1 prevent the write to function correctly in SPI2, seems to indicate those lines are actually necessary. They just need to use the correct port thought and not just read from SPI1 always.
Exactly my opinion.
Victor, I would suggest you update SPI.cpp and SPI.h in your github repo.

@ Roger,
I made two pull requests regarding the SPI.cpp, please disregard the first one, it is wrong, the change should have been inserted into line 349.
The second one is to solve the glitches before the SCK real pulses.

@Victor, I made a pull request to solve the bugfix regarding SPI 2 writing problem.
And another pull request to change the calling sequence between init() and begin().

steve

ok I will close the first PR and test the second one.

Steve I have been away from the forums for a while due to lack of time. I believe you sent some pull request with changes to the sdFat library.
Is it all tested and stable now, or do you need to add any other change before I merge?

Hi Victor,
I have tested it as much as I could, used the RawWrite example to record analog data sampled in dual channel simultaneous mode.
So far everything looks fine.
Of course, it would be better when more people could be involved in testing, but i still would apply the PR.

In this page https://github.com/rogerclarkmelbourne/ … /Libraries

Latest SdFat, modified version from https://github.com/victorpv/SdFat seems to work fine. If you want the best performance for large transfers, can use DMA mode, but that requires the latest SPI Library from https://github.com/victorpv/Arduino_STM … raries/SPI . Does not need the updated SPI files if you don’t use DMA mode. DMA can be enabled or disabled editing SdSpiSTM32F1.cpp, read the file to know what to modify.

Is that info still correct?

If I download https://github.com/rogerclarkmelbourne/ … master.zip I see that the files are from Nov 21. Would that contain all the fixes discussed in this thread?

PM @stevestrong

He was the last person to have a good look at this.

I’m sure he will be able to tell you the best lib to use

I think the part Does not need the updated SPI files if you don’t use DMA mode. is wrong.
The new files are needed for a smooth SPI operation, at least on SPI 2, check out my latest PR.
However, I only tested w/o DMA. AS Victor said that between w/ and w/o DMA the difference is not that huge, I keep using the standard non-DMA way.

Hello Steve, what version of the libraries and core files have you used to make everything work?

Is the info I posted (below) correct?

Latest SdFat, modified version from https://github.com/victorpv/SdFat seems to work fine. If you want the best performance for large transfers, can use DMA mode, but that requires the latest SPI Library from https://github.com/victorpv/Arduino_STM … raries/SPI . Does not need the updated SPI files if you don’t use DMA mode. DMA can be enabled or disabled editing SdSpiSTM32F1.cpp, read the file to know what to modify.

Hi Roger, honestly, I’m not quite sure about the core version.
I use Github for Windows, I synchronized, and I could see all the PRs from your development and master branch, and the Github GUI tells me that it would be the STM32 master branch… So I only can hope that it is indeed the STM32 master branch, since all my “Github” knowledge reduces to commit and sync…and sometimes, by chance, to a real PR
Can you see what I have changed? Only 2 files are involved.
The SdFat version is from Victor’s repo + my PR from there.

Regarding the info, as I told, didn’ test the DMA. And for the “normal” SPI2 (w/o DMA) to work, and to make the glitches gone, my changes would be mandatory.

I generally use the git shell

then CD into the Arduino_STM32 folder and type

git checkout development

Finally got this to work on my Olimexino with its integrated sd card on SPI 2. The library is about twice as fast as the old Sdfat ported to libmaple.
Did a few of the examples and not got any errors, DMA seams like a worthwhile boost specially in read with a 85% increase.

The bench example with card on full speed DMA on.
Free RAM: 11783
Type is FAT32
Card size: 31.91 GB (GB = 1E9 bytes)

Manufacturer ID: 0X3
OEM ID: SD
Product: SL32G
Version: 8.0
Serial number: 0X55BA300
Manufacturing date: 2/2015

File size 5 MB
Buffer size 512 bytes
Starting write test, please wait.

write speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
255.46,75601,1523,2002
262.07,61258,1518,1951
257.48,67833,1524,1986
256.38,72433,1526,1995
257.68,60078,1525,1985
258.06,81216,1510,1982
251.27,69162,1535,2036
257.68,68508,1525,1985
252.89,77578,1525,2022
256.22,73986,1518,1996

Starting read test, please wait.

read speed and latency
speed,max,min,avg
KB/Sec,usec,usec,usec
1060.15,1281,477,482
1059.70,1281,477,482
1059.70,1277,478,482
1060.15,1278,477,481
1059.70,1276,478,482

Done