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

SPI pins are no longer optional #2133

Merged
merged 4 commits into from
Sep 12, 2024
Merged

SPI pins are no longer optional #2133

merged 4 commits into from
Sep 12, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Sep 10, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Since we already have DummyPin (now NoPin), let's replace the remaining uses of Option with it.

cc #1318
Closes #2006

@bugadani bugadani marked this pull request as draft September 10, 2024 15:32
@bugadani bugadani mentioned this pull request Sep 10, 2024
6 tasks
@bugadani bugadani marked this pull request as ready for review September 11, 2024 15:38
@Dominaezzz
Copy link
Collaborator

Not a blocker for this PR (This problem already exists with or without this PR anyway) but I thought it's worth mentioning that using NoPin for peripheral input is interesting since you have to choose between 0 or 1 internally. 0 makes sense for some signals and 1 makes sense for others.
I'm not sure how best to express this, maybe a big match table I guess.

Perhaps it's best to wait until someone complains that a driver doesn't work right when using NoPin. 😂

@bugadani
Copy link
Contributor Author

bugadani commented Sep 11, 2024

I guess we should direct users to prefer Level if they can - but NoPin (or something else) needs to implement both PeripheralInput and PeripheralOutput because some peripheral signals need both (e.g. half-duplex SPI data).

I think I would like to avoid a big table but the idea of mapping InputSignal to Level is a good worst case fallback. Do you happen to have an example peripheral/signal in mind? Or did I hit a jackpot with QSPI failures? :D :D

@Dominaezzz
Copy link
Collaborator

I guess we should direct users to prefer Level if they can

Yeah that's probably best.

Or did I hit a jackpot with QSPI failures? :D :D

😂

but NoPin (or something else) needs to implement both PeripheralInput and PeripheralOutput because some peripheral signals need both (e.g. half-duplex SPI data).

Ahhh, I had completely missed this. Fair enough

Do you happen to have an example peripheral/signal in mind?

SPI slave CS pin should probably have a default of 0, otherwise the peripheral is useless.
SD/MMC card detect pin should probably be 1 so the hardware doesn't complain about the card being missing.
SD/MMC write protect pin should probably be 0 since by default cards should be read/write.
LCD_CAM ctrl signals should be 1 by default otherwise the peripheral won't work. (A bit awkward if the user enables signal inversion in the peripheral but that's out of scope for this I think)
Same story I2S camera mode ctrl signals.

I can't think of more ATM but these are what I have in mind.

@bugadani
Copy link
Contributor Author

SPI slave CS pin should probably have a default of 0, otherwise the peripheral is useless.
SD/MMC card detect pin should probably be 1 so the hardware doesn't complain about the card being missing.
SD/MMC write protect pin should probably be 0 since by default cards should be read/write.
LCD_CAM ctrl signals should be 1 by default otherwise the peripheral won't work. (A bit awkward if the user enables signal inversion in the peripheral but that's out of scope for this I think)
Same story I2S camera mode ctrl signals.

Thanks. If any of these is bidirectional, that's a blocker and I don't see a good way around it that doesn't suck in some way.

@Dominaezzz
Copy link
Collaborator

They're all input only.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MabezDev MabezDev added this pull request to the merge queue Sep 12, 2024
Merged via the queue into esp-rs:main with commit 562c891 Sep 12, 2024
26 of 27 checks passed
@bugadani bugadani deleted the dummy branch September 12, 2024 11:44
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.

API consistency for GPIO types
4 participants