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

Make SPICONFIG configurable #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rfestag
Copy link

@rfestag rfestag commented Mar 21, 2019

In order to support ESP32, we need to be able to change the max speed from the SPICONFIG. This pull request is related to #57.

@PaulStoffregen
Copy link
Owner

Any idea if turning this from a constant to a variable has a performance impact on other boards?

@rfestag
Copy link
Author

rfestag commented Mar 21, 2019

I honestly don't know for certain, and I don't have anything else to test with for confirmation. It's been a while since I've delved too deeply into C++, but I would think that the variable form would perform better than the old define at least, and may be negligibly worse than a "static const" version.

As I understand, because #define is a macro, it is replacing all intsances of SPICONFIG with SPISettings(...). Presumably that means it is creating a new SPISettings object each time it is used, as opposed to re-using an existing one. I could be misunderstanding that though.

Unfortunately, I can't seem to find any meaningful information on whether "const" vs. a mutable variable declaration perform any better. Anywhere it is discussed, it simply says the compiler can optimize away the fact that you may not have used "const" (i.e., if it sees you never re-assign, it will just treat it as const). The advantages of const seem to be more maintainability, but obviously if you need to allow for changing a value, any optimizations related to immutable values go out the window.

@rfestag
Copy link
Author

rfestag commented Mar 23, 2019

@PaulStoffregen - Any further thoughts on this PR, or is there anything else I can do to make you more comfortable with any potential performance differences?

@rnbokade
Copy link

I would suggest putting the clock in terms of clock_div so that this is compatible with even the lower clock speed Arduinos... automatically by adjusting the clock speed according to the board on which the thing is running on

@intensite
Copy link

Any news on this merge request?

On ESP32 the default SPICONFIG doesn't seem to work
#define SPICONFIG SPISettings(50000000, MSBFIRST, SPI_MODE0)

However, If I change it to
#define SPICONFIG SPISettings(20000000, MSBFIRST, SPI_MODE0)
It works perfectly.

Also, since I use the hardware SPI (HSPI) which uses different pins

SPI MOSI MISO CLK CS
VSPI GPIO 23 GPIO 19 GPIO 18 GPIO 5
HSPI GPIO 13 GPIO 12 GPIO 14 GPIO 15

none of the example code work out of the box unless I define a SPIClass variable like:

const int FlashChipSelect = 15; // digital pin for flash chip CS pin
SPIClass SPI1(HSPI);

Then use the SerialFlash.begin like so:
SerialFlash.begin( SPI1,FlashChipSelect );

So far I successfully tested the RawHardwareTest and EraseEverything samples with the above modifications.

@dlyckelid
Copy link

Any news on this?
I have to change it manually now to 26MHz make it work. Would be great to have this pull request approved!

@sambartle
Copy link

I think there's a good chance the issue I raised #79 will also need this change to resolve as a change in the core related to SPI frequency is causing that issue.

I don't know enough about SPI to be sure.

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

Successfully merging this pull request may close these issues.

6 participants