Updating to embedded-hal v1.0

[ryan-summers]

This PR refactors the driver to use the 1.0 release of the embedded-hal.

Because of embedded-hal-bus being available to provide for SPI buses with multiple peripherals, the Ref implementations have been removed entirely because this crate doesn't need to worry about sharing the SPI bus across multiple owners any longer.

[kellerkindt]

LGTM except the removal of the *Ref part. Maybe I dont understand it or overlook something. Consider the following example (semi-pseudo-code):

pub struct MyControllerState {
    spi: SpiBus<...>,
    cs_w5500: OutputPin<..>,
    cs_ds93c46: OutputPin<...>,
    w5500_state: InactiveDevice<w5500::Manual>,
    ds93c46_state: DS93C46<..>,
}

impl MyControllerState {
    pub fn new(...) -> Self { ... }

    pub fn send_udp(&mut self, destination: SocketAddr, data: &[u8]) -> Result<...> {
         let mut device = self.w5500_state.activate_ref(FourWireRef::new(&mut self.spi, &mut self.cs_w5500))?;
         let mut socket = device.socket().unwrap(); // for simplicity of this example, this socket is not cached in MyControllerState
         device.connect(&mut socket, destination).unwrap(); // for simplicity of this example...
         nb::block!(device.send(socket, data))
    }


    pub fn write_eeprom(&mut self, position: usize, data: &[u8], delay: impl DelayMs<u16>) -> Result<...> {
        self.ds93c46_state.enable_write(spi, delay)?;
        let result = self.ds93c46_state.write(spi, delay, position, data);
        self.ds93c46_state.disable_write(spi, delay)?;
        result
    }
}

In this example, the SPI bus is shared between the w5500 chip and an eeprom:

• without additional runtime cost
• and with a compile-time-check that prevents me from actually borrowing/accessing it twice simultaneously.

How would that look with the embedded-hal-bus crate?

[ryan-summers]

That would look (roughly) like:

// Instantiate the SPI peripheral
let bus = hal::spi::new();
let w5500_csn = hal::gpio::get();
let eeprom_csn = hal::gpio::get();

// Store the SPI bus somewhere that all of your users can access it. In this case, assume we're using SPI always in the same context (i.e. same interrupt priority level)
let bus_refcell = core::cell::RefCell::new(bus);

// Construct the EEPROM and W5500, each of which get their own owned handles to the bus + CS pins.
let w5500_spi = embedded_hal_bus::spi::RefCellDevice::new(&bus_refcell, w5500_csn);
let w5500 = w5500::UninitializedDevice::new(w5500::bus::FourWire(w5500_spi)).initialize().unwrap();

let eeprom_spi = embedded_hal_bus::spi::RefCellDevice::new(&bus_refcell, eeprom_csn);
let eeprom = Eeprom::new(eeprom_spi);

// Now freely use the EEPROM and W5500

You can use different types of storage for the actual bus depending on your application requirements (i.e. Mutex-based, Atomic-access based if you're using RTIC, or a critical-section protected bus). https://docs.rs/embedded-hal-bus/latest/embedded_hal_bus/spi/index.html has all of the supported bus synchronization types currently.

The synchronization type (i.e. Bus container) that you pick may be zero / non-zero run time cost depending on how you use it, but it will always guarantee at compile-time that you do not access the bus simultaneously. If you i.e. are using the bus in multiple different interrupt priorities, you can never have zero runtime cost (although it is very low cost with an NVIC priority configuration)

[ryan-summers]

Also, I want to point out that the embedded-hal-bus approach is likely less runtime cost than you currently have. As it stands, you're moving the spi peripheral around between your drivers, which is copying around data and updating internal state. The approach of embedded-hal-bus requires generally either:

1. No runtime overhead at all (i.e. RefCellDevice)
2. Single atomic flip runtime check (i.e. AtomicDevice)
   • This would require using the NVIC to handle synchronization across multiple ISR contents, like what RTIC does
3. Critical section (interrupt disable/enable) (i.e. CriticalSectionDevice)

[ryan-summers]

I'd also be happy to reinstate InactiveDevice to accomodate the existing use case you have. I don't think the Ref devices make sense given that the embedded-hal-bus essentially does that for you via the RefCell types

[kellerkindt]

I'd also be happy to reinstate InactiveDevice to accomodate the existing use case you have. I don't think the Ref devices make sense given that the embedded-hal-bus essentially does that for you via the RefCell types

Sorry, I am currently at work and cannot look into this in detail until later. I support the idea to keep at least InactiveDevice around. I just noticed that the trait SpiDevice is implemented on &mut "impl SpiDevice" / &mut "impl SpiBus", with the InactiveDevice this would then support my example from above again.

[ryan-summers]

No worries, I've reintroduced the InactiveDevice and associated functions

[kellerkindt]

Sorry, I should have been more verbose with my earlier response. There is still the need for the DeviceRefMut - but not for our custom BusRef impl - so that the call w5500_state.activate_ref is possible. I suggest to restore most of the removed lines in src/device.rs, src/tcp.rs and src/udp.rs. The README.md and CHANGELOG.md also need to be updated then again (they mention the removal of DeviceRefMut).

Regarding your earlier points

As it stands, you're moving the spi peripheral around between your drivers, which is copying around data and updating internal state.

This isn't true, because only references are passed around. The DeviceRefMut is nothing more than two references: one to the current driver state, one to the spi-bus. Nothing needs to be copied around.

If you have the spare time, it might be useful to add one or two sentences to the DeviceRefMut and/or InactiveDevice for that. Maybe even referencing &mut "impl SpiDevice" / &mut "impl SpiBus".

Sorry to bother you so much with this and thank you.

[ryan-summers]

@kellerkindt I proposed a new approach to provide an equivalent of DeviceRefMut without any of the code duplication. Let me know what you think. It changes the Device type signature to add one more trait indirection, but this is a simple transformation for users:
Device<BUS, Mode> -> Device<BUS, DeviceState<Mode>>

[ryan-summers]

Thanks for the discussion and review! :)