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

Revisions to SPI classes #2130

Merged
merged 11 commits into from Dec 31, 2020
Merged

Revisions to SPI classes #2130

merged 11 commits into from Dec 31, 2020

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Oct 28, 2020

Refactor, tidy and implement SPIClass for ESP32.

External interfaces haven't changed but internal ones have.
Tested with ScreenTFT_ILI9340-ILI9341, SDCard and Radio_nRF24L01 samples.

bugfix: correct CPU multipler, should be 1M (was 10M)

Naming / comments / style

  • Remove extraneous debug statement in setClock()
  • Get rid of redundant/duplicate comments
  • Don't repeat method name in brief
  • Use cstdint types

SPI code revisions to simplify and improve performance

  • SPISettings passed by reference. SPIClass no longer keeps a copy of these, no point.
  • Clock register value is pre-calculated and stored in settings. Code simplified for readability.
  • bugfix: exception (division by zero) raised in some cases, e.g. freq = 27M.
  • BeginTransaction() and prepare() just set up registers anyway as there is now minimal overhead in doing this.

Implement SPIClass for Esp32

Has three available SPI modules:

  • VSPI works reliably
  • HSPI works with SPI display but not with SDCard (even at low clocks) but it does work with remapped pins (e.g. SPIClass(SpiBus::HSPI, SpiPins{18, 19, 23})). Suspect involvement with RTC/IO but as yet unable to pin this down.
  • FSPI crashes: needs more work (it's shared with Flash).

@slaff slaff added this to the 4.2.0 milestone Oct 29, 2020
@mikee47 mikee47 closed this Oct 29, 2020
@mikee47 mikee47 deleted the fix/spi-classes branch October 29, 2020 21:47
@mikee47 mikee47 restored the fix/spi-classes branch October 29, 2020 22:57
@mikee47 mikee47 reopened this Oct 29, 2020
@mikee47 mikee47 force-pushed the fix/spi-classes branch 2 times, most recently from 39d1aa3 to 7f0f182 Compare October 29, 2020 23:08
@slaff
Copy link
Contributor

slaff commented Nov 26, 2020

Fix Esp32 code (not yet working - just copied Esp8266 version)

Just bought one of these LILYGO ESP32 watches. Hope to be able to run Sming on it soon :-)

@slaff slaff mentioned this pull request Dec 19, 2020
4 tasks
@mikee47
Copy link
Contributor Author

mikee47 commented Dec 19, 2020

Re. #2171 (comment) I'll get this PR tested so we can merge it, then start on updating the HardwareSPI library with ESP32 support.

As for the Esp32 Arduino SPI interface I'm not sure what to do about that. Looking at the Esp32 arduino code it does the same thing as the Esp8266 stuff and just mucks about with hardware registers directly. It's generally a poor model (I don't use it).

@slaff
Copy link
Contributor

slaff commented Dec 19, 2020

@mikee47 Do you think you can add also I2C support for ESP32?

@mikee47
Copy link
Contributor Author

mikee47 commented Dec 20, 2020

@slaff I'll certainly take a look at I2C at some point. Hardware support for I2C on ESP32 warrants a separate HardwareI2C library, even more important to make that asynchronous given the slow bus speeds.

@slaff
Copy link
Contributor

slaff commented Dec 26, 2020

@mikee47 what is the status of this PR? Did you have a chance to test it on Esp32 device?

@mikee47
Copy link
Contributor Author

mikee47 commented Dec 26, 2020

@slaff Works fine on ESP8266, ESP32 is WIP

@mikee47 mikee47 changed the title [WIP] Revisions to SPI classes Revisions to SPI classes Dec 28, 2020
@mikee47 mikee47 force-pushed the fix/spi-classes branch 2 times, most recently from a092adb to 00eaef3 Compare December 28, 2020 15:41
@mikee47
Copy link
Contributor Author

mikee47 commented Dec 29, 2020

@slaff OK ready for review/testing.

@slaff
Copy link
Contributor

slaff commented Dec 29, 2020

@kmihaylov can you try using this PR and one or two of your devices that use SPI?

@mikee47 mikee47 force-pushed the fix/spi-classes branch 3 times, most recently from 1bab882 to 2216941 Compare December 30, 2020 21:51
@kmihaylov
Copy link
Contributor

Wow too much activity on the SPI class. Should I wait a bit or give it a go?

@slaff
Copy link
Contributor

slaff commented Dec 31, 2020

Should I wait a bit or give it a go?

@kmihaylov Go for it. Please test both ESP8266 and ESP32.

@slaff slaff removed the 3 - Review label Dec 31, 2020
@slaff
Copy link
Contributor

slaff commented Dec 31, 2020

@mikee47 PR #2188 is now merged. You can rebase on latest develop and check on your microcontrollers.

Also remove extraneous debug statement in setClock()
Change 'lenght' -> 'length'
SPISettings passed by reference. SPIClass no longer keeps a copy of these, no point.
Clock register value is pre-calculated and stored in settings. Code simplified for readability.
bugfix: exception (division by zero) raised in some cases, e.g. freq = 27M.
BeginTransaction() and prepare() just set up registers anyway as there is now minimal overhead in doing this.
Duplicate method comment blocks removed, should only be in header.
Get rid of redundant/duplicate comments
Don't repeat method name in brief
Use cstdint types
…code

Clock has to be recalculated on every call. With non-const version, it is cached.
@slaff slaff merged commit 6d7455c into SmingHub:develop Dec 31, 2020
@mikee47 mikee47 deleted the fix/spi-classes branch December 31, 2020 18:02
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.

None yet

3 participants