feature: expose C api `cs_pre_trans_delay` in `SpiDriver` #266

vpochapuis · 2023-06-19T06:49:50Z

Dear esp-idf-hal maintainers,

I was trying to use the different Spi Drivers provided by the crate in order to communicate with an industrial Spi chip (AnalogDevices LTC-6813.
When I was doing so in C, i was using the spi_device_interface_config_t struct field cs_ena_pretrans in order to successfully communicate with the chip. It seems that without a delay between the CS activation and the 1st clock tick, the slave IC doesnt understand the transaction.
I found that that the SpiDriver exposed in the crate didn't expose this parameter, resulting in the communication with the chip to fail (less than 1us delay between the CS activation and the first clock tick).
However, the SoftwareCs Driver, seemed to use it, but in fact was simulating it using its own Delay function. I tried it, but even without any delay added with the given function, the CS delay is at least 20us which is too long.

Hence, in order to let users take advantage of the esp-idf already existing API, I propose to expose 2 parameters: cs_ena_pretrans and cs_ena_posttrans with this Merge Request.

Please let me know if there is any issue with this approach.

Thank you for your attention.

Vollbrecht

Thanks for the contribution ❤️

Vollbrecht · 2023-06-19T12:49:40Z

src/spi.rs

+        /// Add an aditional Amount of SPI bit-cycles the cs should be activated before the transmission (0-16).
+        /// This only works on half-duplex transactions.
+        #[must_use]
+        pub fn cs_pre_delay_us(mut self, delay_us: u16) -> Self {


How does it behave when not in half-duplex mode? Do we get a runtime EspError or does it just get eaten silently? If you are not sure could you test out the cases and report how it acts running?

That's a very funny thing in fact. I only use the FullDuplex mode.
I am using the AI-Thinker NodeMcu ESP32-S.
And it does make a difference (small one, but there is). However it only seems to take in account the fact that its different from 0.
When I do not set it or put 0, we have no time gap between CS pull-down and the first clock (and you can see the turquoise MISO channel doesn't change its state):

0

When I set it to 1/16/1000, I get the same small time gap between the CS pull-down and the first clock (and MISO answers):

1

16

1000

This is all using FullDuplex.

They say in the C API driver include that its not supported, but somewhere else if we look closely we see:

In full-duplex mode, only support cs pretrans delay = 1 and without address_bits and command_bits"

So, at least from what I see its that, even though some part of the C doc seems to say that in FullDuplex mode it doesn't work. In reality its more like a on-off switch for a 1us pre-delay in FullDuplex mode, but it does make a big difference for some SPI chips.

Also, as you can see, setting a value higher than 16 (here 1000), doesn't change the behavior compared to 16.

(And sorry for the bad pictures instead of the oscilloscope screen capture, I was lazy to remember the functionality, but I think its still is enough to demonstrate)

Further reading of the doc says:

cs_ena_pretrans is not compatible with the Command and Address phases of full-duplex transactions.

But on the field itself:

Amount of SPI bit-cycles the cs should be activated before the transmission (0-16). This only works on half-duplex transactions.

Seems like its not as clear as it could be in the doc.

Thanks for testing it out!
We are not using the Command & Address part currently in the driver. So that bit doesn't matter.
Did you also test the post delay? Docs don't indicate that this isn't only working in Half-Duplex.

What version of esp-idf are you using? We could annotate your finding's that a delay is happening in Full-Duplex at least in your tested esp-idf version, but with a warning that it might not be a "stable" option to do so.

This week will be busy at work for reports etc... I will see if I have time, if not it will be next week!

Its your PR -> your pacing 😄
i have no stress here

Vollbrecht · 2023-06-19T12:54:11Z

src/spi.rs

@@ -233,6 +233,10 @@ pub mod config {
        pub duplex: Duplex,
        pub bit_order: BitOrder,
        pub cs_active_high: bool,
+        ///< Amount of SPI bit-cycles the cs should be activated before the transmission (0-16). This only works on half-duplex transactions.


If you specify more than 16 'bit cycles', how does the api react? does it take 16 than? If not sure can you test it and report back?

It doesn't change anything given my test (0/1/16/1000), at least in FullDuplex Mode. It behaves like if the value was the highest available.
Further testing on other boards and configs could be interesting.

Could you run a short test with half-duplex on your board and just write something and check if the delay is working overall as intended?

I am doing some further testing at home on another oscilloscope and another ESP32 (its still a NodeMcu but instead of having written ESP32S on top its written ESP32-WROOM-32E). It behaves the same as the other one, but for scientific purpose I will redo all the tests.
Just to note that DMA doesn't work on Half-Duplex (I think this is normal and documented) (which I was activating using Fullduplex in my previous tests and no error was shown about it). I will post the results once I finish.

thanks for further looking into it. If you want to check out the C driver path. We are using the somewhat stable driver/spi C api, though it itself goes down a bit deeper and is somewhat splitted.

In C land they have the high level drivers for General Purpose (gpspi) and one for communicating with the internal Flash memory (sdspi). This is what lives in https://github.com/espressif/esp-idf/blob/master/components/driver/spi/include/driver/spi_master.h and neighborhood.

This drivers are written against an C hal living in https://github.com/espressif/esp-idf/blob/master/components/hal/include/hal/spi_hal.h

The hal itself acts on a "low level" api that is somewhat simmilar accros all chips. This is the abstraction over the register level. Here we can also find more infos about the delays itself. For example if we look at the codepath for an esp32 can be found here. https://github.com/espressif/esp-idf/blob/master/components/hal/esp32c3/include/hal/spi_ll.h#L795 . There is an interesting command that suggest the delay should not work in Full-Duplex if command and address features are utilized, but we are not using them. So by that comment it should work in Full-Duplex. Though your test seems to suggest that is not really working fully just "partially"?

vpochapuis · 2023-06-20T12:05:43Z

It seems the compilation on the CI fails for some platforms. Wouldn't it need to change the toolchain to esp instead of nightly for non-RISC-V chips? (I am not sure, its a wild guess after quickly looking)

Vollbrecht · 2023-06-20T12:55:34Z

It seems the compilation on the CI fails for some platforms. Wouldn't it need to change the toolchain to esp instead of nightly for non-RISC-V chips? (I am not sure, its a wild guess after quickly looking)

yeah its a known compiler issue currently, a fix is in the working. IF the S2 and risc-v are passing for you here its fine CI wise.

Vollbrecht · 2023-06-20T13:02:46Z

we might revisit the complete delay topic shortly, when rust-embedded/embedded-hal#462 lands . It's currently not fully flashed out but i think the delay would be flexible to not only allow delays between transaction, it would presumably be allowing to add an delay as a first transaction.

vpochapuis · 2023-06-20T13:26:46Z

we might revisit the complete delay topic shortly, when rust-embedded/embedded-hal#462 lands . It's currently not fully flashed out but i think the delay would be flexible to not only allow delays between transaction, it would presumably be allowing to add an delay as a first transaction.

I was gonna say that they are 2 totally different things while only reading their discussion, but reading the actual code changes and the datasheet of device in example, you are right.
Even though the "mindset" and "workflow" would be totally different (instead of having a SPI configuration, need to add to each Transaction? Or would it be possible to add a config that transparently adds the Operation at the start of each transaction?), it apparently could work.
By adding a delay operation at the start, from what I see in the datasheet the delay as the CS pulled down and the clock isn't ticking, so it would in theory simulate the same behavior.
I just hope it could be used as a configuration too (maybe I am too much used to the API of esp-idf haha).

(Also unrelated to this technical issue, how do you keep track of other projects pull request like this and get a good overview of the progress, status etc... I am a bit curious and even though I often use Open-Source, I didn't get the chance and time to contribute much, I would love some advice)

Vollbrecht · 2023-06-21T12:02:55Z

I just hope it could be used as a configuration too (maybe I am too much used to the API of esp-idf haha).

We have to see how the Operation pattern itself changes the landscape. Though we always will have some esp specific api, that most likly will be different from the embedded-hal api.

(Also unrelated to this technical issue, how do you keep track of other projects pull request like this and get a good overview of the progress, status etc... I am a bit curious and even though I often use Open-Source, I didn't get the chance and time to contribute much, I would love some advice)

Well everyone and every project is different, so i can only speak how its around here. There is not a magical switch though it comes with hanging around and doing stuff. To be a bit more concrete

The esp-rs matrix channel is a good feedback channel what is currently happening, there are also a related of other matrix channels around the rust working group and other project. It helps to read them.
Every Wednesday there is a public embedded rust wg meeting inside the matrix channel. Every two weeks usually on Thursday we have a esp-rs community meeting in Google Meet.
Subscribing to repository's you are interested in helping on GH helps alot - Software is build on more software -> If you also subscribe to relevant dependency's it helps a lot.

vpochapuis · 2023-06-23T14:41:33Z

I just hope it could be used as a configuration too (maybe I am too much used to the API of esp-idf haha).

We have to see how the Operation pattern itself changes the landscape. Though we always will have some esp specific api, that most likly will be different from the embedded-hal api.

(Also unrelated to this technical issue, how do you keep track of other projects pull request like this and get a good overview of the progress, status etc... I am a bit curious and even though I often use Open-Source, I didn't get the chance and time to contribute much, I would love some advice)

Well everyone and every project is different, so i can only speak how its around here. There is not a magical switch though it comes with hanging around and doing stuff. To be a bit more concrete

The esp-rs matrix channel is a good feedback channel what is currently happening, there are also a related of other matrix channels around the rust working group and other project. It helps to read them.

Every Wednesday there is a public embedded rust wg meeting inside the matrix channel. Every two weeks usually on Thursday we have a esp-rs community meeting in Google Meet.

Subscribing to repository's you are interested in helping on GH helps alot - Software is build on more software -> If you also subscribe to relevant dependency's it helps a lot.

Thanks a lot for the advice !

vpochapuis · 2023-06-23T15:52:36Z

@Vollbrecht Here is my further testing:

My understanding about the SPI pre-transaction timing is that:

In FullDuplex whether DMA is activated or not doesn't have an impact.
In FullDuplex, the cs_ena_pretrans is only an ON/OFF switch for an extra 1us of delay between the CS pull-down and the first CLK tick
In HalfDuplex, DMA doesn't work
In HalfDuplex the cs_ena_pretrans value is the number of added microsecond to the delay between the CS pull-down and the first CLK tick. This value is modulo 16. (Hence for a value of 100 we get 4, 200 we get 8, ...)

Vollbrecht · 2023-07-03T17:48:33Z

@Vollbrecht Here is my further testing:

My understanding about the SPI pre-transaction timing is that:

* In FullDuplex whether DMA is activated or not doesn't have an impact.

* In FullDuplex, the `cs_ena_pretrans` is only an ON/OFF switch for an extra 1us of delay between the CS pull-down and the first CLK tick

* In HalfDuplex, DMA doesn't work

* In HalfDuplex the `cs_ena_pretrans` value is the number of added microsecond to the delay between the CS pull-down and the first CLK tick. This value is modulo 16. (Hence for a value of 100 we get 4, 200 we get 8, ...)

Ty for all the testing and sorry for the delay!
I am currently a bit busy, but as soon as the work on DelayUs for SPI starts i hope we get something more refined here in the future.
I think its at least a good stop gap measure to expose the official api here a bit, even if its usefulness is limited. If you add the conclusion into some comments into the code so that other people can profit from the information, i think we are good to go here.

…lays

…-spi-driver

Vollbrecht · 2023-11-11T16:38:40Z

@vpochapuis did you have the chance to try out our new driver after the update. Because of the new transaction iter thing we currently can only delay between transfers. For Soft CS drivers this issue is trivial to fix but the other case would still relight on this underlying idf method.

vpochapuis · 2023-11-11T16:47:50Z

@vpochapuis did you have the chance to try out our new driver after the update. Because of the new transaction iter thing we currently can only delay between transfers. For Soft CS drivers this issue is trivial to fix but the other case would still relight on this underlying idf method.

Dear @Vollbrecht I didn't have time to try it out yet!
Is there specific things you would like me to try out?
(Also I might soon switch to ESP32-C3 or other for our project as having a separate toolchain setup is quite difficult to integrate in our CI).

…elay-in-spi-driver

Vollbrecht · 2023-11-15T12:22:07Z

No just make sure that it sill does the things you want it to do, technically the last big API changes on our site should not influenced this PR to much, just to verify it works with the current API, then we can merge it.

vpochapuis · 2023-11-15T15:45:16Z

No just make sure that it sill does the things you want it to do, technically the last big API changes on our site should not influenced this PR to much, just to verify it works with the current API, then we can merge it.

Thanks for telling me this!
I just took out the whole testing bench again, wired it up and... there was a missing part!
I fixed it and after modifications its working as intended!

…elay-in-spi-driver

Vollbrecht · 2024-03-06T16:22:54Z

@ivmarkov we can also merge this. @vpochapuis sorry for keeping this dormant so long thanks for keeping it up to date.

vpochapuis · 2024-03-06T16:27:43Z

@ivmarkov we can also merge this. @vpochapuis sorry for keeping this dormant so long thanks for keeping it up to date.

Its ok! thanks a lot for getting back on it!

vpochapuis · 2024-03-06T16:28:02Z

I saw the CI didn't pass some checks, is it normal? Or should i do anything about it?

Vollbrecht · 2024-03-06T16:28:35Z

its fixed already on master, it was just some nightly clippy things

…elay-in-spi-driver

vpochapuis · 2024-03-06T16:29:16Z

its fixed already on master, it was just some nightly clippy things

Oh ok, I synced my branch just now, just in case

…elay-in-spi-driver

ivmarkov · 2024-05-01T11:20:48Z

@Vollbrecht Don't you have merge perms in the meantime?

leonzchang · 2024-05-06T05:22:49Z

This is what I need, any update?

vpochapuis force-pushed the feature/expose-c-api-cs-pre-trans-delay-in-spi-driver branch from c2d527d to f78f342 Compare June 19, 2023 08:40

feature: expose C api cs_pre_trans_delay in SpiDriver

0412e3b

vpochapuis force-pushed the feature/expose-c-api-cs-pre-trans-delay-in-spi-driver branch from f78f342 to 0412e3b Compare June 19, 2023 08:54

Vollbrecht reviewed Jun 19, 2023

View reviewed changes

docs: add explanation on Duplex specifities for cs pre-transaction de…

0e2807b

…lays

vpochapuis requested a review from Vollbrecht August 8, 2023 06:32

Merge branch 'master' into feature/expose-c-api-cs-pre-trans-delay-in…

5b1899b

…-spi-driver

Merge branch 'esp-rs:master' into feature/expose-c-api-cs-pre-trans-d…

4432eec

…elay-in-spi-driver

fix(spi): add missing pre transaction parameters for C config struct

3d918c5

vpochapuis added 2 commits December 28, 2023 17:22

Merge branch 'esp-rs:master' into feature/expose-c-api-cs-pre-trans-d…

3fc68ff

…elay-in-spi-driver

Merge branch 'esp-rs:master' into feature/expose-c-api-cs-pre-trans-d…

0dd6ba6

…elay-in-spi-driver

Vollbrecht approved these changes Mar 6, 2024

View reviewed changes

Merge branch 'esp-rs:master' into feature/expose-c-api-cs-pre-trans-d…

164389e

…elay-in-spi-driver

vpochapuis added 2 commits March 29, 2024 11:26

Merge branch 'esp-rs:master' into feature/expose-c-api-cs-pre-trans-d…

f047c62

…elay-in-spi-driver

Merge branch 'esp-rs:master' into feature/expose-c-api-cs-pre-trans-d…

c1b1464

…elay-in-spi-driver

ivmarkov merged commit a3371b3 into esp-rs:master May 6, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: expose C api `cs_pre_trans_delay` in `SpiDriver` #266

feature: expose C api `cs_pre_trans_delay` in `SpiDriver` #266

vpochapuis commented Jun 19, 2023

Vollbrecht left a comment

Vollbrecht Jun 19, 2023

vpochapuis Jun 20, 2023

vpochapuis Jun 20, 2023 •

edited

Loading

Vollbrecht Jun 20, 2023

vpochapuis Jun 20, 2023

Vollbrecht Jun 21, 2023

Vollbrecht Jun 19, 2023

vpochapuis Jun 20, 2023

Vollbrecht Jun 20, 2023

vpochapuis Jun 23, 2023 •

edited

Loading

Vollbrecht Jun 23, 2023

vpochapuis commented Jun 20, 2023

Vollbrecht commented Jun 20, 2023 •

edited

Loading

Vollbrecht commented Jun 20, 2023

vpochapuis commented Jun 20, 2023

Vollbrecht commented Jun 21, 2023

vpochapuis commented Jun 23, 2023

vpochapuis commented Jun 23, 2023

Vollbrecht commented Jul 3, 2023

Vollbrecht commented Nov 11, 2023

vpochapuis commented Nov 11, 2023

Vollbrecht commented Nov 15, 2023

vpochapuis commented Nov 15, 2023

Vollbrecht commented Mar 6, 2024

vpochapuis commented Mar 6, 2024

vpochapuis commented Mar 6, 2024

Vollbrecht commented Mar 6, 2024

vpochapuis commented Mar 6, 2024

ivmarkov commented May 1, 2024

leonzchang commented May 6, 2024

feature: expose C api cs_pre_trans_delay in SpiDriver #266

feature: expose C api cs_pre_trans_delay in SpiDriver #266

Conversation

vpochapuis commented Jun 19, 2023

Vollbrecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpochapuis Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpochapuis Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpochapuis commented Jun 20, 2023

Vollbrecht commented Jun 20, 2023 • edited Loading

Vollbrecht commented Jun 20, 2023

vpochapuis commented Jun 20, 2023

Vollbrecht commented Jun 21, 2023

vpochapuis commented Jun 23, 2023

vpochapuis commented Jun 23, 2023

Vollbrecht commented Jul 3, 2023

Vollbrecht commented Nov 11, 2023

vpochapuis commented Nov 11, 2023

Vollbrecht commented Nov 15, 2023

vpochapuis commented Nov 15, 2023

Vollbrecht commented Mar 6, 2024

vpochapuis commented Mar 6, 2024

vpochapuis commented Mar 6, 2024

Vollbrecht commented Mar 6, 2024

vpochapuis commented Mar 6, 2024

ivmarkov commented May 1, 2024

leonzchang commented May 6, 2024

feature: expose C api `cs_pre_trans_delay` in `SpiDriver` #266

feature: expose C api `cs_pre_trans_delay` in `SpiDriver` #266

vpochapuis Jun 20, 2023 •

edited

Loading

vpochapuis Jun 23, 2023 •

edited

Loading

Vollbrecht commented Jun 20, 2023 •

edited

Loading