Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SdSpiTeensy3.cpp - Could be faster and not need to use 512 bytes on the stack on each sector send. #506

Open
KurtE opened this issue Jan 11, 2025 · 4 comments

Comments

@KurtE
Copy link

KurtE commented Jan 11, 2025

@greiman @PaulStoffregen: While @mjs513 and myself have been playing with SdFat code on another board, I noticed that
the current versions of this file, use the option 1 like code as described in this version library SdFatConfig.h file:

/**
 * If USE_SPI_ARRAY_TRANSFER is one and the standard SPI library is
 * use, the array transfer function, transfer(buf, count), will be used.
 * This option will allocate a 512 byte temporary buffer for send.
 * This may be faster for some boards.  Do not use this with AVR boards.
 *
 * Warning: the next options are often fastest but only available for some
 * non-Arduino board packages.
 *
 * If USE_SPI_ARRAY_TRANSFER is two use transfer(nullptr, buf, count) for
 * receive and transfer(buf, nullptr, count) for send.
 *
 * If USE_SPI_ARRAY_TRANSFER is three use transfer(nullptr, buf, count) for
 * receive and transfer(buf, rxTmp, count) for send. Try this with Adafruit
 * SAMD51.
 *
 * If USE_SPI_ARRAY_TRANSFER is four use transfer(txTmp, buf, count) for
 * receive and transfer(buf, rxTmp, count) for send. Try this with STM32.
 */

This code on every multi byte (512 typically) send it uses a 512 byte array on the stack, that it copiesthe data
to be sent to and then uses the m_spi->transfer(tmp, count);
code to send it.

8 years ago we added the methods:

	static void setTransferWriteFill(uint8_t ch ) {_transferWriteFill = ch;}
	static void transfer(const void * buf, void * retbuf, uint32_t count);

to the Teensy SPI library.
so this code could simply be:

void SdSpiArduinoDriver::send(const uint8_t* buf, size_t count) {
    m_spi->transfer(buf, nullptr, count);
}

Likewise, currently the receive function does a memset(buf, 0xff, count);
before it calls: m_spi->transfer(buf, count);

Now if SD cards do require 0xff to be sent, you can specify what character should be sent when
you specify in the transfer, call that you are not passing it a buffer.

Something like:

uint8_t SdSpiArduinoDriver::receive(uint8_t* buf, size_t count) {
  m_spi->setTransferWriteFill(0xff);
  m_spi->transfer(nullptr, buf, count);
  return 0;
}
@greiman
Copy link
Owner

greiman commented Jan 11, 2025

Many board packages now support the following API:

void transfer(const uint8_t* txBuf, uint8_t* rxBuf);

Where txBuf or rxBuf may be nullptr and 0XFF is sent if txBuf is nullptr.

Transfer mode 2 can be used and no temp buffer will be needed.

Here is receive for mode 2 and here is send.

I will not add special cases for Teensy. I am removing special cases as more boards support the above API. There are now endless "Arduino compatible" boards so you must fork the library for one-of-a-kind APIs.

@KurtE
Copy link
Author

KurtE commented Jan 11, 2025

Thanks @greiman,

I more or less 100% agree with you. In fact that is why I included the comments about the different modes.
If this change was made and Teensy was set for mode 2, the only thing missing was the stuff for begin:

void SdSpiArduinoDriver::begin(SdSpiConfig spiConfig) {
  if (spiConfig.spiPort) {
    m_spi = spiConfig.spiPort;
#if defined(SDCARD_SPI) && defined(SDCARD_SS_PIN)
  } else if (spiConfig.csPin == SDCARD_SS_PIN) {
    m_spi = &SDCARD_SPI;
    m_spi->setMISO(SDCARD_MISO_PIN);
    m_spi->setMOSI(SDCARD_MOSI_PIN);
    m_spi->setSCK(SDCARD_SCK_PIN);
#endif  // defined(SDCARD_SPI) && defined(SDCARD_SS_PIN)
  } else {
    m_spi = &SPI;
  }
  m_spi->begin();
}

As far as I can tell, this is for if the user chooses the SPI interface for the built in SDCard on T3.5/6/T4.x and said use
the SDCARD_SS_PIN - it will automatically chose to use SPI and on 3.5/6 that is SPI1 and choose the MISO, MOSI, SCK and CS pin.

Never thought you could do that (Use SPI for it...) although should work. (I think). Not sure it is worth the trouble of special driver.

@PaulStoffregen - I don't believe the Teensy fork of this, has integrated this support for choosing the method of how read and write of multiple bytes has been done. Maybe something good for the next beta?

@facchinm as @greiman mentioned:

Many board packages now support the following API:

void transfer(const uint8_t* txBuf, uint8_t* rxBuf);

I really wish that Arduino would adopt this as well, especially for all ARM base boards. Any chance of this?
Thanks

@greiman
Copy link
Owner

greiman commented Jan 12, 2025

the only thing missing was the stuff for begin:

Many boards require a special begin call or combination of calls. SdFat supports a user call to setup the SPI port.

Here are SPI options:
Screenshot 2025-01-12 075654

Here is the form of the call

// Setup the SD_SPI port before this begin call.
sd.begin(SdSpiConfig(CS_PIN, USER_SPI_BEGIN | <other options>, &SD_SPI));

@greiman
Copy link
Owner

greiman commented Jan 13, 2025

One final SPI option. You can wrap any SPI driver with the SdFat API. Here is an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants