embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
5.52k stars 768 forks source link

embassy-embedded-hal fails to build with "time" feature disabled #2227

Closed tcbennun closed 11 months ago

tcbennun commented 11 months ago

Compile error occurs twice in this file: https://github.com/embassy-rs/embassy/blob/3a8a950bb8e8c8054aeb1338d17a3fa2e14c1b58/embassy-embedded-hal/src/shared_bus/blocking/spi.rs

62 |               let op_res = operations.iter_mut().try_for_each(|op| match op {
   |  __________________________________________________________________-
63 | |                 Operation::Read(buf) => bus.read(buf),
   | |                                         ------------- this is found to be of type `Result<(), <BUS as embedded_hal::spi::ErrorType>::Error>`
64 | |                 Operation::Write(buf) => bus.write(buf),
   | |                                          -------------- this is found to be of type `Result<(), <BUS as embedded_hal::spi::ErrorType>::Error>`
65 | |                 Operation::Transfer(read, write) => bus.transfer(read, write),
   | |                                                     ------------------------- this is found to be of type `Result<(), <BUS as embedded_hal::spi::ErrorType>::Error>`
66 | |                 Operation::TransferInPlace(buf) => bus.transfer_in_place(buf),
   | |                                                    -------------------------- this is found to be of type `Result<(), <BUS as embedded_hal::spi::ErrorType>::Error>`
67 | |                 #[cfg(not(feature = "time"))]
68 | |                 Operation::DelayUs(_) => Err(SpiDeviceError::DelayUsNotSupporte...
   | |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Result<(), ...>`, found `Result<_, ...>`
...  |
73 | |                 }
74 | |             });
   | |_____________- `match` arms have incompatible types
   |
   = note: expected enum `Result<(), <BUS as embedded_hal::spi::ErrorType>::Error>`
              found enum `Result<_, SpiDeviceError<_, _>>`
   = help: consider constraining the associated type `<BUS as embedded_hal::spi::ErrorType>::Error` to `SpiDeviceError<_, _>`
   = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html

The error is one thing, but the intended behaviour here is not ideal. If the crate is built without support for DelayUs, I do not think we should attempt the sequence of operations before shortcircuiting when a DelayUs is reached. It would be cleaner to no-op the whole transaction. For instance, this could be done at the start of transaction():

        if (
            cfg!(not(feature = "time")) &&
            operations.iter().find(|op| matches!(op, Operation::DelayUs(_))).is_some()
        ) {
            return Err(SpiDeviceError::DelayUsNotSupported);
        }

and then in the match, we kill off the error thus:

                #[cfg(not(feature = "time"))]
                Operation::DelayUs(_) => unreachable!(),
Dirbaio commented 11 months ago

agreed, can you send a PR?