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

Make embedded-hal 1.0.0 non-optional #753

Merged
merged 16 commits into from
Jan 13, 2024
Merged

Make embedded-hal 1.0.0 non-optional #753

merged 16 commits into from
Jan 13, 2024

Conversation

jannic
Copy link
Member

@jannic jannic commented Jan 10, 2024

Marked as draft because I still have to check if the code needs some clean-up.

@ithinuel
Copy link
Member

Shall we make eh1 the embedded_hal and rename 0.2 as embedded_hal_02 ?

When we introduce async, should we make it optional or not?

@Sympatron
Copy link
Contributor

If you make async optional it should be enabled by default IMO. And I agree that v1 should just be embedded_hal and v0.2 should be treated as legacy.

@9names
Copy link
Member

9names commented Jan 10, 2024

If you make async optional it should be enabled by default IMO.

We should just include it unconditionally. There is no benefit to having it optional.

v1 should just be embedded_hal and v0.2 should be treated as legacy.

It makes no difference to the HAL or it's users what the dependencies are called internally. It only matters that we implement the traits in the HAL so that drivers that depend on those trait versions work.

@jannic
Copy link
Member Author

jannic commented Jan 10, 2024

I also think there's no reason to make async optional. It should be possible to implement it in a way that has minimal if async is not used in a firmware. (There is some impact because of additional dependencies which will increase compilation time. I'd hope that this increase won't be significant.)

Regarding the internal naming: That's one of the possible cleanups I was thinking about. Not critical because it should not affect the API, but we still should aim for well readable code within the HAL.

@Sympatron
Copy link
Contributor

I looked into renaming embedded-hal-1 internally and it is a lot more complicated than I thought. This should probably be done in a separate PR so this one can go through quickly.

@jannic jannic added this to the 0.10.0 milestone Jan 10, 2024
@jannic
Copy link
Member Author

jannic commented Jan 10, 2024

I updated several files to use the eh-1 traits without renaming, and instead rename the eh-0.2 if there are conflicts.
This is mainly cosmetic (just internal, not changing the API).
A notable exception is spi::FrameFormat, which uses Mode from embedded-hal in one of its variants. I changed that to use Mode from embedded-hal 1.0.
To avoid conflicts with #747, I skipped updating the i2c code.

It kept the commits nicely separated, so it's easy to move these changes to their own PR in case you prefer to merge the minimal change first.

@jannic jannic marked this pull request as ready for review January 10, 2024 22:10
Adds notes where this isn't (yet) possible.

This also involved adding an inherent method to PWM Channels, so you can enable or disable them without using the old trait.
@jonathanpallant
Copy link
Contributor

I propose changing the examples to use the new traits by default. Basically, you shouldn't have to know about the 0.2 traits unless you need the backwards-compatibility.

See https://github.com/rp-rs/rp-hal/tree/extra-1_0-impls.

@jannic
Copy link
Member Author

jannic commented Jan 12, 2024

I propose changing the examples to use the new traits by default. Basically, you shouldn't have to know about the 0.2 traits unless you need the backwards-compatibility.

See https://github.com/rp-rs/rp-hal/tree/extra-1_0-impls.

Yes, definitely!
Should I merge your changes into this pull request, or should we merge this one first and you add a separate one to update the examples?

@jonathanpallant
Copy link
Contributor

jonathanpallant commented Jan 12, 2024

If you like my proposal, please do merge it into your branch, so we get can it into main in one hit.

I thought this was easier than a million ```suggestion blocks on the review page.

@thejpster
Copy link
Member

I find that little and often is a good policy, and this PR is already large, so I think we should merge it and then work on more clean-up stuff later. In particular, we need this in so we can rebase the I2C changes over it, and we then need to work on embedded-io support for the serial ports.

@jannic
Copy link
Member Author

jannic commented Jan 13, 2024

I find that little and often is a good policy, and this PR is already large, so I think we should merge it and then work on more clean-up stuff later. In particular, we need this in so we can rebase the I2C changes over it, and we then need to work on embedded-io support for the serial ports.

Definitely. That's why I marked the pull request as ready for review 3 days ago. (Sorry, I forgot to update the top comment accordingly.)
I don't claim that the patch contains all changes that should be done. It's a first step, but there are still many improvements possible. Please review and decide if the general approach is ok, and look for mistakes I might have made. If it's good enough, we can merge it and then improve in separate pull requests.

@jannic jannic merged commit 1481429 into rp-rs:main Jan 13, 2024
8 checks passed
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.

6 participants