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

Unify embedded_hal::spi traits? #525

Closed
madsmtm opened this issue Nov 21, 2023 · 4 comments
Closed

Unify embedded_hal::spi traits? #525

madsmtm opened this issue Nov 21, 2023 · 4 comments

Comments

@madsmtm
Copy link

madsmtm commented Nov 21, 2023

I'm unfamiliar with SPI, so maybe I'm completely off here, but it seems like the two main traits in embedded_hal::spi could be somewhat unified?

As an example, consider the API below.

trait Spi: ErrorType {
    fn read(&mut self, words: &mut [Word]) -> Result<(), Self::Error>;
    fn write(&mut self, words: &[Word]) -> Result<(), Self::Error>;
    fn transfer(&mut self, read: &mut [Word], write: &[Word]) -> Result<(), Self::Error>;
    fn transfer_in_place(&mut self, words: &mut [Word]) -> Result<(), Self::Error>;
    fn flush(&mut self) -> Result<(), Self::Error>;
}

trait SpiBus: Spi {}

trait SpiDevice: Spi {
    fn transaction(&mut self, operations: &mut [Operation<'_, Word>]) -> Result<(), Self::Error>;
}

impl<T: ?Sized + SpiDevice> Spi for T {
    fn read(&mut self, words: &mut [Word]) -> Result<(), Self::Error> {
        self.transaction(&mut [Operation::Read(words)])
    }
    fn write(&mut self, words: &[Word]) -> Result<(), Self::Error> {
        self.transaction(&mut [Operation::Write(words)])
    }
    fn transfer(&mut self, read: &mut [Word], write: &[Word]) -> Result<(), Self::Error> {
        self.transaction(&mut [Operation::Transfer(read, write)])
    }
    fn transfer_in_place(&mut self, words: &mut [Word]) -> Result<(), Self::Error> {
        self.transaction(&mut [Operation::TransferInPlace(words)])
    }
    fn flush(&mut self) -> Result<(), Self::Error> {
        // Noop for SPI devices, the flushing happens automatically inside the transaction
    }
}

Again, unsure if this is actually beneficial to driver / HAL authors or not?

This was referenced Nov 21, 2023
@nyurik
Copy link
Contributor

nyurik commented Nov 22, 2023

I just consolidated the Spi transaction (3 identical copies) - possibly related: #529

@Dirbaio
Copy link
Member

Dirbaio commented Nov 25, 2023

I don't think this is a good idea. SpiBus and SpiDevice's write() methods have very different meaning, even if their signature is the same.

  • SpiBus::write(): clock out the bytes, return possibly before it's finished
  • SpiDevice::write(): assert CS, clock out the bytes, flush the bus to make sure it's indeed finished, deassert CS, return.

If they're in a shared trait Spi, a driver could do where T: Spi which would accept both SpiBus and SpiDevice objects. If the driver is expecting an SpiDevice (chip with CS), it'll break if the user passes a SpiBus, and vice versa. Also, there's no case where a driver would really want to be generic over SpiBus and SpiDevice: the chip either has CS or doesn't.

For the HALs it doesn't save code either, they don't implement SpiBus and SpiDevice on the same type at the same time for the same reason: it's either managing a CS pin or it's not.

@Tremoneck
Copy link

Tremoneck commented Nov 29, 2023

It would be useful to have the ability to use a bus as device. If the device has a CS pin, but it's permanent active by direct connection to GND or VCC. That would mean the driver implements device but it doesn't have a usable CS pin so for the user it would be a Bus.

So in the hal an explicit conversion from bus to device, that behaves like a device would be useful.

However here it was already discussed and the decision was against it. The argument was, that drivers depend on the CS toggling and some chips need the CS to actually toggle for correct functionality. However if this kind of error should be cought by the person designing the PCB not be the software developer.

@Dirbaio
Copy link
Member

Dirbaio commented Nov 29, 2023

It would be useful to have the ability to use a bus as device. If the device has a CS pin, but it's permanent active by direct connection to GND or VCC.

You can still do it if you write a struct that wraps SpiBus and implements SpiDevice. Same as ExclusiveDevice in embedded-hal-bus, but without toggling the CS pin. This is an "incorrect" SpiDevice implementation because the SpiDevice contract says CS must toggle for each transaction, but you can still opt in to do it at your own risk.

I don't think we should allow silently using a SpiBus as SpiDevice. It doesn't enable new use cases (you can already do it with the current traits as I explained above), it just creates a footgun.

@Dirbaio Dirbaio closed this as completed Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants