SPI: Implement more SPI traits from embedded-hal 1.0.0-alpha.8 #101

har7an · 2022-07-13T14:13:01Z

Hi,

as the title suggests, this PR implements the SpiBusWrite and SpiBus traits from the latest embedded-hal alpha release as documented here.

I'm putting this up here in case someone is interested in having a preliminary look. Things that I'd still like to implement:

Consistent naming
Currently we have send_bytes and write_bytes, both doing the same thing really, what should I do with that?
A generic SpiDevice trait so we can share the SpiBus when we want to
Ideally this goes in tandem with some sort of marker trait that prohibits creating a SpiDevice from a SpiBus that has a CS pin associated? Or maybe we make the SpiDevice the default and drop the CS from the bus entirely?
Proper error types
While the code is in fact infallible (apart from the unsafe parts of course), I think it would be nice to have e.g. a ReadOnly and WriteOnly that is returned when writing without mosi and reading without miso, respectively. I guess these could be enforced at compile time with more types and abstractions, but tbh these abstractions make me all fuzzy in the head... Or should we keep the current behavior where nothing happens?
Add more fun use cases to the example
Find out why there is a 2 ms gap between consecutive transfers when using SpiBus::transfer with buffers much bigger than the 64 bytes SPI FIFO
Fix transfer_in_place (currently broken)

I only have an ESP32 here for testing, so I'm looking forward to get some feedback from anyone who cares to test this on other ESP32 boards/chips. I have implemented a new test esp32-hal/examples/spi_eh1_loopback.rs that performs a very simple demo as of currently.

har7an · 2022-07-13T14:15:42Z

Oh and here's a proof from my logic analyzer that I didn't just cheat:

Did a performance test, and it seems that the current implementation, regardless of the SPI speed, always has a 2 ms gap between consecutive transfers when transferring multiples of 64 bytes (the SPI FIFO size). Investigating...

har7an · 2022-07-15T09:15:51Z

Hope you don't mind me documenting what I've done so far for myself. You can safely ignore this message.

Update

Fixed the transfer performance. Previously the FIFO was filled and read from in copies of 4 bytes size each. Now instead we use a core::ptr::copy_nonoverlapping, which is rusts equivalent to C's memcpy.

The implementation is broken since it only works when calling copy with a size that exactly fills the entire FIFO, even if the input is smaller than that. For some reasons giving it a size different from that will break the code. I suspect this may have something to do with how to properly implement integer division but I can't tell for sure yet.

Consecutive transfers now take ~370 us, down from the previous ~2ms in debug builds. For release builds, transfer time dropped from 48 us to 14.12 us.

Added an example that performs a big transfer of 256 bytes length.

Edit: See bold text.

bjoernQ · 2022-07-15T09:53:49Z

Regarding those ~2ms .... is that for release or debug builds?

har7an · 2022-07-15T09:57:19Z

@bjoernQ Debug builds, now that you mention it. Good point, I completely forgot about that...

har7an · 2022-07-15T10:43:15Z

@bjoernQ Would you prefer to have a separate example to demo the SPI loopback with the eh1 feature enabled, or should I integrate it into the existing example via conditional compilation?

bjoernQ · 2022-07-15T11:02:48Z

@bjoernQ Would you prefer to have a separate example to demo the SPI loopback with the eh1 feature enabled, or should I integrate it into the existing example via conditional compilation?

Probably it's better to separate them since they should be as less confusing as possible. However, be careful regarding CI since eh1 is not a default feature (for reasons). Also, after a clean checkout cargo build --examples should ideally just work for everyone.

har7an · 2022-07-15T11:16:57Z

@bjoernQ Understood. I made it a separate file now and wrapped main into a #[cfg(feature = "eh1")]. If the 'eh1' feature isn't present, it will instead compile a minimal main that complains via serial0 that this examples requires the 'eh1' feature. :)

bjoernQ · 2022-07-15T12:39:46Z

@bjoernQ Understood. I made it a separate file now and wrapped main into a #[cfg(feature = "eh1")]. If the 'eh1' feature isn't present, it will instead compile a minimal main that complains via serial0 that this examples requires the 'eh1' feature. :)

Another - maybe easier and probably better - solution would be to add something like this to the cargo.toml:

[[example]]
name = "myexample"
required-features = ["myfeature", "another"]

This would prevent the example to be built if the required features are not activated. TBH I never tried this myself

har7an · 2022-07-18T08:17:48Z

@bjoernQ I'm starting to think Cargo.toml deserves its own book...

This works, and I added it to the PR now. Thanks for sharing!

This leaves three TODOs from above, what's your opinion on these?

bjoernQ · 2022-07-18T10:32:56Z

Great!

For the send_bytes / write_bytes: looks like write_bytes just calls send_bytes so (if I'm not missing something here) we could get rid of write_bytes?

For the other things: @jessebraham what do you think? You are more into the EH1 things

jrmoulton · 2022-07-18T14:50:48Z

This leaves three TODOs from above

For the SpiDevice trait I have an open draft pull request. It's got a few things that need to be fixed but it is a working implementation

jessebraham · 2022-07-18T15:10:33Z

Sorry I'm not going to be able to get to this for a day or two probably. Will take a look ASAP.

har7an · 2022-07-19T06:27:18Z

For the SpiDevice trait I have an open draft pull request. It's got a few things that need to be fixed but it is a working implementation

Oh cool! Clever idea to split the SPI part into a separate SpiMaster. I'm also working on an implementation where the device directly takes the Mutex<RefCell<SpiBus>>, but that doesn't work due to funny issues with mutability and the Mutex trait implementation on the mutex types in xtensa_lt...

But how exactly does the CS line get handled? You're connecting it like that: self.cs.connect_peripheral_to_output(spi.spi.cs_signal());, but is this reversible or does it have a permanent effect?

har7an · 2022-07-19T06:29:55Z

For the send_bytes / write_bytes: looks like write_bytes just calls send_bytes so (if I'm not missing something here) we could get rid of write_bytes?

Given that send_bytes was introduced in this rather recent commit, maybe we drop send_bytes and use write_bytes instead so existing code directly calling that hopefully doesn't break?

jrmoulton · 2022-07-19T07:23:48Z

But how exactly does the CS line get handled? You're connecting it like that: self.cs.connect_peripheral_to_output(spi.spi.cs_signal());, but is this reversible or does it have a permanent effect?

It does have a permanent effect but I was just playing with reconnecting the cs pin to the GPIO peripheral after flushing the bus. I didn't get the chance to test it but I think that would fix the issue

jrmoulton · 2022-07-19T07:27:27Z

Given that send_bytes was introduced in this rather recent commit, maybe we drop send_bytes and use write_bytes instead

I wore the send bytes specifically because it doesn't read bytes after writing so a device that only needs to write doesn't spend time reading. But it is bad naming and quite a bit of duplication which was discussed in the PR

jessebraham · 2022-07-25T21:33:01Z

Sorry for taking so long to get to this. Hopefully this helps, if you have any additional questions let me know.

Consistent naming
Currently we have send_bytes and write_bytes, both doing the same thing really, what should I do with that?

No strong opinion here. I would choose write_bytes personally, but feel free to just go with your gut.

A generic SpiDevice trait so we can share the SpiBus when we want to
Ideally this goes in tandem with some sort of marker trait that prohibits creating a SpiDevice from a SpiBus that has a CS pin associated? Or maybe we make the SpiDevice the default and drop the CS from the bus entirely?

For the sake of pushing forward on this PR, maybe this could be done after in a subsequent PR? I'm frankly not super familiar with the newer traits (especially for SPI) so I really don't know the pros/cons of leaving this out at this point.

Proper error types
While the code is in fact infallible (apart from the unsafe parts of course), I think it would be nice to have e.g. a ReadOnly and WriteOnly that is returned when writing without mosi and reading without miso, respectively. I guess these could be enforced at compile time with more types and abstractions, but tbh these abstractions make me all fuzzy in the head... Or should we keep the current behavior where nothing happens?

If everything is Infallible then maybe this isn't a huge deal, could possibly be pushed off. I do like the idea of adding verification like you alluded to; compile-time would be preferable for obvious reasons, but runtime-checks are better than nothing IMO and we can always change that later.

har7an · 2022-08-02T12:36:57Z

For the sake of pushing forward on this PR, maybe this could be done after in a subsequent PR?

That's what I'm aiming for now anyway.

but runtime-checks are better than nothing IMO and we can always change that later.

That's true of course, but the current code doesn't even impose a runtime check. Tbh I have no idea what happens on a write without a MOSI line attached, I guess it does nothing?

I'll try to get the merge conflicts fixed, let me check what happened...

har7an · 2022-08-04T07:36:49Z

@jessebraham @bjoernQ CI passed, I have nothing more to add at this point. :)

jrmoulton · 2022-08-04T18:30:22Z

esp-hal-common/src/spi.rs

+    where
+        T: Instance,
+    {
+        /// See also: [`send_bytes`].


I guess this send_bytes comment can be removed/renamed

MabezDev · 2022-08-04T21:37:08Z

Thanks for the PR! Just testing this out, seems like the s2 & s3 examples don't work... probably not this PR because I can't get the normal loopback example to work on these chips, @bjoernQ, @jessebraham is this the case for you too?

bjoernQ · 2022-08-08T07:36:21Z

Also for me the normal loopback example doesn't work on S2 and S3 anymore on this branch while they do work fine on main

har7an · 2022-08-08T08:08:30Z

That's interesting... I only have the regular esp32 here, but I just double-checked: Both the regular loopback example and the one for embedded-hal 1 work fine. I also didn't touch the regular loopback example, only the HAL it's using. Going through the changes I don't see what may break here...

har7an · 2022-08-08T09:02:18Z

Found an oversight while working on some other SPI-related code. From the commit message:

The previous read implementation would only read the contents of the
SPI receive FIFO and return that as data. However, the SpiBusRead
trait defines that while reading, bytes should be written out to the bus
(Because SPI is transactional, without writing nothing can be read).

Tested the examples again here on my PC, they still work.

as counterpart to `send_bytes` that is responsible only for reading bytes received via SPI.

to use `send_bytes` and `read_bytes` under the hood and remove duplicate code.

that is re-exported when the `eh1` feature flag is active. This removes lots of duplicate `#[cfg(...)]` macros previously part of the code.

traits from the `embedded-hal 1.0.0-alpha.8`.

and bump the SPI speed to 1 MHz.

This cuts down the time between consecutive transfers from about 2 ms to less than 1 ms.

cutting down the time between transfers from just below 1 ms to ~370 us. The implementation is currently broken in that it will always fill the entire FIFO from the input it is given, even if that isn't FIFO-sized...

and compile a dummy instead when the "eh1" feature isn't present.

in normal builds, where the feature flag "eh1" isn't given. Building the example directly via `cargo build --example spi_eh1_loopback` will now print an error that this requires a feature flag to be active.

and drop `send_bytes` instead. Previoulsy, both served the same purpose, but `send_bytes` was introduced more recently and is hence less likely to cause breaking changes in existing code.

to all targets.

The previous `read` implementation would only read the contents of the SPI receive FIFO and return that as data. However, the `SpiBusRead` trait defines that while reading, bytes should be written out to the bus (Because SPI is transactional, without writing nothing can be read). Reimplements the `embedded-hal` traits to correctly implement this behavior.

All esp variants except for the esp32s2 have a 64 byte FIFO, whereas the esp32s2 has a 72 byte FIFO.

by reverting to the old method of reading 32 bytes at a time and assembling the return buffer from that. It turns out that the previous `core::slice::from_raw_parts()` doesn't work for the esp32s2 and esp32s3 variants, returning bogus data even though the correct data is present in the registers.

har7an · 2022-08-17T06:37:31Z

I've just noticed there are a few tasks in the original post which have not been completed.

Sort of lost track of these, heh...

The consistent naming is done now,
The SpiDevice trait is best implemented elsewhere, currently in SpiDevice trait on the esp32c3 #100 and embedded-hal SpiDevice on all esp32 variants #106 (And thanks to common/gpio: Allow disconnecting IO from peripheral #147 we can do the shared CS in hardware!)
The error type is best left to another PR someday in the future

I think there were some changes pushed to main which will require some updates to the examples.

Correct, should be fixed now!

bjoernQ · 2022-08-17T07:07:31Z

This is looking pretty good, thank you for all the time and effort put into this! I've tested spi_loopback and spi_eh1_loopback, using both dev and release profiles, for all four supported chips. No issues.

I would just like one more review on this prior to merging, @bjoernQ and/or @MabezDev could you please take a look when you get a chance?

Looks good to me and the examples work fine 👍

MabezDev

LGTM! Thanks for the PR @har7an!

jessebraham marked this pull request as draft July 13, 2022 16:29

har7an mentioned this pull request Jul 19, 2022

embedded-hal SpiDevice on all esp32 variants #106

Merged

har7an force-pushed the feature/eh1-spi-bus branch from bb9ca64 to ca733d1 Compare August 2, 2022 12:52

har7an marked this pull request as ready for review August 3, 2022 09:17

har7an force-pushed the feature/eh1-spi-bus branch 2 times, most recently from 1b8385a to e6cb3e9 Compare August 4, 2022 07:32

jrmoulton reviewed Aug 4, 2022

View reviewed changes

har7an added 23 commits August 17, 2022 08:22

common/spi: Implement read_bytes

5d198f4

as counterpart to `send_bytes` that is responsible only for reading bytes received via SPI.

common/spi: Rewrite transfer

c76ba07

to use `send_bytes` and `read_bytes` under the hood and remove duplicate code.

common/spi: Create submodule for embedded_hal_1

cc9e522

that is re-exported when the `eh1` feature flag is active. This removes lots of duplicate `#[cfg(...)]` macros previously part of the code.

common/spi: Implement SpiBus and SpiBusWrite

9a9299b

traits from the `embedded-hal 1.0.0-alpha.8`.

common/spi: Make mosi pin optional

ba1c781

esp32-hal: Add new SPI example with eh1 traits

38b831d

esp32-hal/examples/spi_eh1: Add huge transfer

1097877

and bump the SPI speed to 1 MHz.

common/spi: Apply rustfmt

45fd580

common/spi: Use memcpy to read from registers

9a26920

This cuts down the time between consecutive transfers from about 2 ms to less than 1 ms.

WIP: common/spi: Use ptr::copy to fill write FIFO

4ead721

cutting down the time between transfers from just below 1 ms to ~370 us. The implementation is currently broken in that it will always fill the entire FIFO from the input it is given, even if that isn't FIFO-sized...

common/spi: Add more documentation

83bc3b4

esp32/examples/spi_eh1: Fix transfer_in_place

558b29b

esp32/examples/spi_eh1: Add conditional compile

68a52a6

and compile a dummy instead when the "eh1" feature isn't present.

esp32-hal: Ignore spi_eh1 example

37a9d9f

in normal builds, where the feature flag "eh1" isn't given. Building the example directly via `cargo build --example spi_eh1_loopback` will now print an error that this requires a feature flag to be active.

common/spi: Use write_bytes

13c5cd7

and drop `send_bytes` instead. Previoulsy, both served the same purpose, but `send_bytes` was introduced more recently and is hence less likely to cause breaking changes in existing code.

common/spi: Fix mosi pin setup

7d95e47

Add SPI examples with ehal 1.0.0-alpha8 traits

64fd4ea

to all targets.

common/spi: Use full FIFO size on all variants

dd2ad09

All esp variants except for the esp32s2 have a 64 byte FIFO, whereas the esp32s2 has a 72 byte FIFO.

common/spi: Use common pad byte for empty writes

50a695f

common/spi: Fix typos

2d04587

examples: Fix spi_eh1_loopback examples

1938c90

har7an force-pushed the feature/eh1-spi-bus branch from 140dedf to 1938c90 Compare August 17, 2022 06:31

MabezDev approved these changes Aug 17, 2022

View reviewed changes

MabezDev merged commit 2fe2753 into esp-rs:main Aug 17, 2022

har7an deleted the feature/eh1-spi-bus branch August 17, 2022 11:06

jessebraham mentioned this pull request Aug 19, 2022

Implement the embedded-hal alpha version Traits #69

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPI: Implement more SPI traits from embedded-hal 1.0.0-alpha.8 #101

SPI: Implement more SPI traits from embedded-hal 1.0.0-alpha.8 #101

har7an commented Jul 13, 2022 •

edited

Loading

har7an commented Jul 13, 2022 •

edited

Loading

har7an commented Jul 15, 2022 •

edited

Loading

bjoernQ commented Jul 15, 2022

har7an commented Jul 15, 2022

har7an commented Jul 15, 2022

bjoernQ commented Jul 15, 2022 •

edited

Loading

har7an commented Jul 15, 2022

bjoernQ commented Jul 15, 2022

har7an commented Jul 18, 2022 •

edited

Loading

bjoernQ commented Jul 18, 2022

jrmoulton commented Jul 18, 2022

jessebraham commented Jul 18, 2022

har7an commented Jul 19, 2022

har7an commented Jul 19, 2022

jrmoulton commented Jul 19, 2022

jrmoulton commented Jul 19, 2022

jessebraham commented Jul 25, 2022 •

edited

Loading

har7an commented Aug 2, 2022

har7an commented Aug 4, 2022

jrmoulton Aug 4, 2022

MabezDev commented Aug 4, 2022

bjoernQ commented Aug 8, 2022

har7an commented Aug 8, 2022

har7an commented Aug 8, 2022

har7an commented Aug 17, 2022

bjoernQ commented Aug 17, 2022

MabezDev left a comment

SPI: Implement more SPI traits from embedded-hal 1.0.0-alpha.8 #101

SPI: Implement more SPI traits from embedded-hal 1.0.0-alpha.8 #101

Conversation

har7an commented Jul 13, 2022 • edited Loading

har7an commented Jul 13, 2022 • edited Loading

har7an commented Jul 15, 2022 • edited Loading

Update

bjoernQ commented Jul 15, 2022

har7an commented Jul 15, 2022

har7an commented Jul 15, 2022

bjoernQ commented Jul 15, 2022 • edited Loading

har7an commented Jul 15, 2022

bjoernQ commented Jul 15, 2022

har7an commented Jul 18, 2022 • edited Loading

bjoernQ commented Jul 18, 2022

jrmoulton commented Jul 18, 2022

jessebraham commented Jul 18, 2022

har7an commented Jul 19, 2022

har7an commented Jul 19, 2022

jrmoulton commented Jul 19, 2022

jrmoulton commented Jul 19, 2022

jessebraham commented Jul 25, 2022 • edited Loading

har7an commented Aug 2, 2022

har7an commented Aug 4, 2022

jrmoulton Aug 4, 2022

Choose a reason for hiding this comment

MabezDev commented Aug 4, 2022

bjoernQ commented Aug 8, 2022

har7an commented Aug 8, 2022

har7an commented Aug 8, 2022

har7an commented Aug 17, 2022

bjoernQ commented Aug 17, 2022

MabezDev left a comment

Choose a reason for hiding this comment

har7an commented Jul 13, 2022 •

edited

Loading

har7an commented Jul 13, 2022 •

edited

Loading

har7an commented Jul 15, 2022 •

edited

Loading

bjoernQ commented Jul 15, 2022 •

edited

Loading

har7an commented Jul 18, 2022 •

edited

Loading

jessebraham commented Jul 25, 2022 •

edited

Loading