rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
1.95k stars 197 forks source link

spi: make SpiDevice transaction take an operation slice instead of a closure. #443

Closed Dirbaio closed 1 year ago

Dirbaio commented 1 year ago

As discussed in today's WG meeting.

ryankurte commented 1 year ago

hey seems reasonable to me! a couple of questions:

Dirbaio commented 1 year ago

how do we represent busses with actual hardware CS, pass through a NullCS of some sort?

implementations with hardware CS would directly implement SpiDevice, not SpiBus. You dont take a "SpiBus with hardware CS" and then wrap it on an ExclusiveDevice with NullCS.

Dirbaio commented 1 year ago

can we test this with a PR against linux-embedded-hal as well as another hal and a driver?

@ryankurte @eldruin @therealprof could you please take a look? I'd really like to get this unblocked :)

Dirbaio commented 1 year ago

@eldruin: @Vollbrecht made an interesting comment about read_transaction but it seems only a mild HAL-impl-side inconvenience:

Yes, it's somewhat annoying but I think it's better than taking a &[Operation] and then asserting at runtime that it has only reads (or only writes). And I can't think of any other solution...

The duplication also shows up in #444 . It won't affect most HALs though, as they'll likely implement only SpiBus and let the user use embedded-hal-bus to get an SpiDevice. The only reasons a HAL would implement SpiDevice is for hardware CS (which is very annoying and the perf benefits are questionable in MCUs), or when the kernel handles CS for you (like in esp-idf or Linux).

@eldruin: It seems to me like this should be fine to implement on linux-embedded-hal but we should check on it.

Yeah I think so too. It has to convert from either &[Operation] or &[&[u8]] to the linux ioctl struct anyway...

bors[bot] commented 1 year ago

Build succeeded: