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

feat(embedded-hal-bus): Spi - Add support for cs to clock delay #605

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vpochapuis
Copy link

@vpochapuis vpochapuis commented May 31, 2024

What

Add support for cs to clock delay in embedded-hal-bus spi's device implementation.

Resolves #539

Why

This feature is defined at https://docs.rs/embedded-hal/latest/embedded_hal/spi/index.html#cs-to-clock-delays and issued at #539

Some chips that communicates through SPI necessitate this feature in order to correctly communicate.

How

Added a field in the device structs in order to store the cs_to_clock_delay, with its adequate constructor parameter.
The delay is applied in the shared transaction implementation, just after the CS is toggled.

Notes

  • I might have misunderstood where the clock is being controlled, I would need help if that's the case
  • This change is breaking as the API to create the device is different, should this be avoided?
  • I didn't see the usage of core::time::Duration in the code, I would like to know if its a bad practice in embedded system implementation or not and why

@vpochapuis vpochapuis requested a review from a team as a code owner May 31, 2024 03:41
@vpochapuis vpochapuis marked this pull request as draft May 31, 2024 03:42
@vpochapuis vpochapuis force-pushed the spi-add-settings-to-embedded-hal-bus-SpiDevice-impls-for-CS-to-clock-delays branch 2 times, most recently from 0bbd0f6 to e7fe105 Compare May 31, 2024 03:53
@vpochapuis vpochapuis marked this pull request as ready for review May 31, 2024 03:55
@Dirbaio
Copy link
Member

Dirbaio commented May 31, 2024

Thanks for the PR! This is a welcome addition, just a few notes:

  • Some SPI chips also need a delay after the data, between the last clock and deasserting CS. Could you add that too? I think it could be a separate setting so you can set them to different values.
  • Perhaps the delay could default to 0 and be set with a .set_cs_to_clock_delay_ns() setter instead. This has two advantages: it keeps the new() signature simple for the simple case when you don't need delays, and is backwards-compatible.

@vpochapuis
Copy link
Author

Thanks for the PR! This is a welcome addition, just a few notes:

* Some SPI chips also need a delay after the data, between the last clock and deasserting CS. Could you add that too? I think it could be a separate setting so you can set them to different values.

* Perhaps the delay could default to 0 and be set with a `.set_cs_to_clock_delay_ns()` setter instead. This has two advantages: it keeps the `new()` signature simple for the simple case when you don't need delays, and is backwards-compatible.

Thank you for the feedback!
I will do my best to apply those.

Note:
I have some issue testing this on STM32F439ZI, unrelated issue to this, about the SPI communication CLK pin behavior

If you ever encountered something similar and could give a quick tip

with the clock pin first bit floating for a while before starting normally under embassy_stm32:
image

Maybe related to embassy-rs/embassy#1094 but I actually have no idea (timer prescaler config issue?), maybe you saw this in the past and could have quick tip?

I will continue by testing on ESP32C3 awaiting I fix my issue on the STM32.

@Dirbaio
Copy link
Member

Dirbaio commented May 31, 2024

it might be a bug yes. please file an issue or join #embassy, we can discuss there.

@vpochapuis vpochapuis force-pushed the spi-add-settings-to-embedded-hal-bus-SpiDevice-impls-for-CS-to-clock-delays branch from e7fe105 to ab42bc6 Compare May 31, 2024 10:05
@vpochapuis vpochapuis marked this pull request as draft May 31, 2024 10:07
@vpochapuis vpochapuis force-pushed the spi-add-settings-to-embedded-hal-bus-SpiDevice-impls-for-CS-to-clock-delays branch from ab42bc6 to e0b7617 Compare May 31, 2024 10:08
@vpochapuis vpochapuis marked this pull request as ready for review May 31, 2024 10:09
Fix wrong variable name in spi shared delay driver
@vpochapuis
Copy link
Author

it might be a bug yes. please file an issue or join #embassy, we can discuss there.

Issue created and solve at embassy-rs/embassy#3039

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.

spi: add settings to embedded-hal-bus SpiDevice impls for CS-to-clock delays.
2 participants