-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Implement support for HSPI overlap mode #3189
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3189 +/- ##
======================================
Coverage 27.6% 27.6%
======================================
Files 20 20
Lines 3655 3655
Branches 678 678
======================================
Hits 1009 1009
Misses 2468 2468
Partials 178 178 Continue to review full report at Codecov.
|
libraries/SPI/SPI.cpp
Outdated
pinMode(SCK, SPECIAL); ///< GPIO14 | ||
pinMode(MISO, SPECIAL); ///< GPIO12 | ||
pinMode(MOSI, SPECIAL); ///< GPIO13 | ||
begin(SPI_PINS_HSPI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file used to be formatted with 4 spaces, could you please adjust indentation in your changes?
libraries/SPI/SPI.h
Outdated
@@ -54,6 +60,7 @@ class SPIClass { | |||
public: | |||
SPIClass(); | |||
void begin(); | |||
void begin(uint8_t pinSet, uint8_t miso = 0, uint8_t mosi = 0, uint8_t sck = 0, uint8_t ss = 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather suggest keeping just miso/miso/sck/ss arguments. You can determine _pinSet value internally.
Another point, do Arduino libraries typically call SPI.begin internally? If so, maybe adding an independent .pins method would be a more portable choice. That way, one could first call SPI.pins in the sketch and then call .begin method of another library which would do SPI.begin internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If going with begin method, suggest at least making it take the same arguments as it does in the ESP32 library:
https://github.com/espressif/arduino-esp32/blob/master/libraries/SPI/src/SPI.h
I think I submitted the updated changes (I suck at git). I like your idea of adding an interface to setup the SPI before the call to SPI.being, as you say most libraries just call it internally. I'm kind of wary of this SPI::pins interface as, in absence of SoftwareSPI there's just two magical set of pins that work. And even if we implement SoftSPI all three modes are different enough that I'd rather the caller set explicitly where they want normal HSPI, overlapped HSPI or SoftwareSPI. There are a lot of caveats in overlap mode that I'm in doubt that any library designed for Arduino(avr) is going to work out of the box. First and foremost you can't use any random GPIO for the SS line in overlap mode, you MUST use GPI0 as Hardware CS, otherwise you would likely be asserting your GPIO CC when the flash is using the pins. Now since you are using hardware CS it will deassert your device after every write/transfer. Since most arduino libraries are only doing single byte read/transfers the devices will be constantly asserted/deasserted, but at least the 23LC1024 I used to test this expects CS to be LOW for the entire transfer of a whole address/write so you need to convert existing libraries to use SPI.writeBytes/SPI.transferBytes otherwise they will not work. Let me know what you think. |
Also I didn't updated the documentation my in latest changes. I would do that once we agree on an interface. |
Okay, this version with |
Done, I removed the tabs, updated the doc and squashed all the commits back into one. |
Hey this implement Issue #1062. Allows activation of HSPI overlap mode. I tested this with a 23LC1024 memory and it worked fine. But couldn't make it work with an SD card. The SD card was getting confused by the sharing of the SPI bus.