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

Implement the embedded-hal alpha version Traits #69

Closed
12 of 15 tasks
jessebraham opened this issue Jun 2, 2022 · 11 comments
Closed
12 of 15 tasks

Implement the embedded-hal alpha version Traits #69

jessebraham opened this issue Jun 2, 2022 · 11 comments

Comments

@jessebraham
Copy link
Member

jessebraham commented Jun 2, 2022

It should be possible for these to coexist with the existing 0.2.7 implementations. I have already begun work on this.

EDIT: This crate has been broken up into multiple crates, the task list below has been updated to reflect this. Not all new crates are included, they will be added as needed.


  • [email protected]
    • embedded_hal::delay::DelayUs
    • embedded_hal::digital::InputPin
    • embedded_hal::digital::OutputPin
    • embedded_hal::digital::StatefulOutputPin
    • embedded_hal::digital::ToggleableOutputPin
    • embedded_hal::i2c::I2c
    • embedded_hal::pwm::SetDutyCycle
    • embedded_hal::serial::Write
    • embedded_hal::spi::SpiBus
    • embedded_hal::spi::SpiDevice
  • [email protected]
    • embedded_hal::serial::Read
    • embedded_hal::serial::Write
    • embedded_hal::spi::FullDuplex
  • [email protected]
    • embedded_can::blocking::Can
    • embedded_can::nb::Can
@jessebraham jessebraham self-assigned this Jun 2, 2022
@jessebraham
Copy link
Member Author

A little over half of these traits have been implemented in #82.

@jessebraham
Copy link
Member Author

A couple more SPI traits implemented in #95.

@jessebraham
Copy link
Member Author

Additional SPI traits implemented in #101.

@har7an
Copy link
Contributor

har7an commented Aug 22, 2022

Just a heads up: According to the maintainer of embedded-hal-compat, a new release may be due soon that will add compat for 1.0.0-alpha.8 <-> 0.2.7. Maybe the old traits can be dropped in favor of using the compatibility traits then?

Edit: See ryankurte/embedded-hal-compat#10 (comment)

@jessebraham
Copy link
Member Author

jessebraham commented Aug 22, 2022

Thanks for the update, I am planning on doing that once we get a stable release of embedded-hal. I believe embedded-hal is still going to have an embedded-hal-nb package split out, so I'd like to just wait until things are stable.

@jessebraham jessebraham added this to the v0.2.0 milestone Aug 22, 2022
@jessebraham
Copy link
Member Author

SpiDevice has been implemented in #106

@jessebraham jessebraham removed this from the v0.2.0 milestone Sep 13, 2022
@jessebraham jessebraham removed their assignment Nov 22, 2022
@bugadani
Copy link
Contributor

bugadani commented May 3, 2023

I think SpiDevice holding a critical section throughout a complete transaction is extremely inefficient. If I understand correctly, holding a critical section disables interrupts on the current core (I'm assuming dual core MCUs have independent interrupt masks), and holds a lock across cores. I think this means that using the current implementation a) prevents interrupt handlers running, b) prevents the other core from entering critical sections, and c) makes using multiple SPI peripherals kind of pointless in general, as at most one can be active at any time.

If I'm correct, this is probably less optimal than desired.

@MabezDev
Copy link
Member

MabezDev commented May 4, 2023

I think SpiDevice holding a critical section throughout a complete transaction is extremely inefficient.

I agree, this also means we can't currently reuse the device/bus struct's for async versions because it's possible to hold RefCellMut borrows across await points and you'll get a panic if another bus device tries to borrow. We should change this to a proper Mutex to avoid these issues.

@bugadani
Copy link
Contributor

bugadani commented May 4, 2023

We should change this to a proper Mutex to avoid these issues.

I suspect this will be pretty difficult. I'm not actually sure about details, but if it's possible to use two SpiDevice instances on two RTIC tasks (1 for each), it will be trivially easy to deadlock a firmware - one task waiting for the other to release the Spi bus, without preemption that would make it actually possible. A critical section at least prevents this, though again, I'm not sure if the use case is actually possible or not.

@jessebraham
Copy link
Member Author

A number of traits have been removed, which has been reflected in the original comment. I believe that serial::Write will be going away as well in favour of the embedded-io traits, however I'm not certain on this yet.

@jessebraham
Copy link
Member Author

The embedded-hal-* ecosystem has moved on to 1.0.0-rc.1, which we already include and implement, so I am going to consider this issue completed and close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants