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

Begin implementing the embedded-hal alpha traits #82

Merged
merged 4 commits into from
Jun 14, 2022

Conversation

jessebraham
Copy link
Member

@jessebraham jessebraham commented Jun 13, 2022

This is not exhaustive, but covers the traits which are analogous to those already implemented. It's possible for the 0.2.x and 1.0.0-alpha.x releases to co-exist, as the user can control which set of traits are imported via the prelude.

This has had some minimal level of testing, but I was not very thorough. Most of the functionality is identical to what we previously had so it should all work in theory. These traits are not currently being used in any examples, just to avoid additional changes. We can decide how we want to handle this at some other point I figure.

Implemented:

  • embedded_hal::delay::blocking::DelayUs
  • embedded_hal::digital::blocking::InputPin
  • embedded_hal::digital::blocking::OutputPin
  • embedded_hal::digital::blocking::StatefulOutputPin
  • embedded_hal::digital::blocking::ToggleableOutputPin
  • embedded_hal::i2c::blocking::I2c
  • embedded_hal::serial::nb::Read
  • embedded_hal::serial::nb::Write
  • embedded_hal::spi::nb::FullDuplex

Not Implemented:

  • embedded_hal::digital::blocking::IoPin
  • embedded_hal::serial::blocking::Write
  • embedded_hal::spi::blocking::SpiBus
  • embedded_hal::spi::blocking::SpiBusFlush
  • embedded_hal::spi::blocking::SpiBusRead
  • embedded_hal::spi::blocking::SpiBusWrite

For compatibility reasons I've created the esp_hal_common::spi::SpiMode enum, as we were not actually using the data encoded in the enums provided by the two versions of embedded-hal and they are obviously different types. Let me know how you feel about this, I wasn't really sure how else to accomplish this.

If you feel anything is missing please let me know and I'll add it in. I'll likely deal with the unimplemented traits in a separate PR at some point.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 14, 2022

Awesome! Nice we have a start on this now. With the version pinned it shouldn't cause too much pain.

Just a few thoughts

  • other HALs like RP2040 feature gate the 1.0.x feature - I don't really have an opinion on that - just noticed that
  • maybe we want to add a small chapter about it to the README.md (again like RP2040-HAL)
  • could we keep the v0.2 prelude in its old namespace and only have the v1 live in its own module? This would avoid (another) breaking change (which is not really an issue) but that way it would (correctly) more feel like 0.2 is the current one and 1.0 is the experimental one

Regarding examples for 1.0.x - I'm not sure we should add them now. At this point in time, it will probably just add confusion for new users and users interested in 1.0.x can probably figure things out on their own. Maybe (also to verify our implementations) we could have a separate repo for them. (But just my gut feeling)

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - besides my previous comments - as long as we don't break v0.2 functionality I'm totally fine

Just the thing about the module name of the v0.2 prelude might be worth thinking about before merging

@jessebraham
Copy link
Member Author

Thanks for taking a look.

I considered putting this behind a feature, but decided against it as you basically control which are used by which traits are imported.

I actually agree with you on the prelude thing after sleeping on it, so I'll revert the last commit and switch to that approach instead.

@jessebraham jessebraham merged commit 2417cd0 into esp-rs:main Jun 14, 2022
@jessebraham jessebraham deleted the feature/e-h-alpha branch June 14, 2022 15:37
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

Successfully merging this pull request may close these issues.

2 participants