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

Subsequent calls to SpiBus::write corrupt previous writes without flushing #1369

Closed
markus-k opened this issue Mar 31, 2024 · 2 comments
Closed
Labels
bug Something isn't working peripheral:spi SPI peripheral

Comments

@markus-k
Copy link
Contributor

Subsequent calls to SpiBus::write seem to corrupt previous (unfinished) writes without calling SpiBus::flash() inbetween.

While this is somewhat documented, it does not follow the requirements by embedded-hal:

When calling another method when a previous operation is still in progress, implementations can either wait for the previous operation to finish, or enqueue the new one, but they must not return a “busy” error. Users must be able to do multiple method calls in a row and have them executed “as if” they were done sequentially, without having to check for “busy” errors.

This gets problematic with embedded-hal-bus transactions, where operations are just called one after eachother and only .flush()ed in the end: https://github.com/rust-embedded/embedded-hal/blob/5e21d566e25049a85a74010e0c92708f732b2a7f/embedded-hal-bus/src/spi/shared.rs#L23-L36.

Therefore something like the following example results in corrupted data on the bus:

    let mut spi_led0 = RefCellDevice::new(&spi_bus, spi_cs0, delay.clone());
    let mut txs = [
        embedded_hal::spi::Operation::Write(&[0x80, 0x20]),
        // this works if we insert a delay op here, which internally calls flush():
        // embedded_hal::spi::Operation::DelayNs(1),
        embedded_hal::spi::Operation::Write(&[0xff, 0xff, 0xff, 0xff]),
    ];
    spi_led0.transaction(&mut txs).unwrap();

The simplest solutions I would come up with is to call self.flush() before doing anything in Instance::write_bytes, but I don't know the inner workings of the SPI driver well enough to be sure this doesn't have any unwanted side-affects.

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 2, 2024

The simplest solutions I would come up with is to call self.flush() before doing anything in Instance::write_bytes, but I don't know the inner workings of the SPI driver well enough to be sure this doesn't have any unwanted side-affects.

I guess that should be the way to go. It probably shouldn't cause problems

markus-k added a commit to markus-k/esp-hal that referenced this issue Apr 2, 2024
Otherwise subsequent calls to `SpiBus::write` will cause corrupted writes
and break `embedded-hal-bus`es transactions. Fixes esp-rs#1369.
@jessebraham jessebraham added bug Something isn't working peripheral:spi SPI peripheral labels Apr 3, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 3, 2024
)

* Flush in `spi::master::Instancee::write` before attemting any writes

Otherwise subsequent calls to `SpiBus::write` will cause corrupted writes
and break `embedded-hal-bus`es transactions. Fixes #1369.

* Update Changelog for #1381

* Fix missing result handling when calling `flush()` in SPI `Instance::write_bytes`
@markus-k
Copy link
Contributor Author

markus-k commented Apr 3, 2024

Fixed with #1381

@markus-k markus-k closed this as completed Apr 3, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working peripheral:spi SPI peripheral
Projects
Archived in project
Development

No branches or pull requests

3 participants