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

[vpochapuis]

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.

[vpochapuis]

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]

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]

we might revisit the complete delay topic shortly, when https://github.com/rust-embedded/embedded-hal/pull/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]

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]

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

1. 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.
2. 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.
3. 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]

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

1. 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.
2. 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.
3. 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]

@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]

@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.

[Vollbrecht]

@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]

@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).

[Vollbrecht]

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]

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!

[Vollbrecht]

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

[vpochapuis]

@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]

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

[Vollbrecht]

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

[vpochapuis]

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

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

[ivmarkov]

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

[leonzchang]

This is what I need, any update?