rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.01k stars 202 forks source link

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

Open vpochapuis opened 5 months ago

vpochapuis commented 5 months ago

What

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

Resolves https://github.com/rust-embedded/embedded-hal/issues/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 https://github.com/rust-embedded/embedded-hal/issues/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

Dirbaio commented 5 months ago

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

vpochapuis commented 5 months ago

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](https://github.com/rust-embedded/embedded-hal/assets/75721408/58752290-b5c4-4a80-8909-dddc1f8474a8) Maybe related to https://github.com/embassy-rs/embassy/issues/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 commented 5 months ago

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

vpochapuis commented 4 months ago

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

Issue created and solve at https://github.com/embassy-rs/embassy/issues/3039