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

Refactor for embedded-hal 1.0.0 #157

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

systec-ms
Copy link
Contributor

@systec-ms systec-ms commented Nov 16, 2021

TODOs:

  • fix fmc example
    DelayUs<u8> must be adjusted to DelayUs<u32> in stm32-fmc:
         pub fn init<D>(&mut self, delay: &mut D) -> *mut u32
         where
             D: DelayUs<u8>,
         {...}
  • fix usb_serial example
  • Adapt CAN to embedded-hal::can?
    I'm not sure yet if the customization should happen here in hal or on the bxcan side.
  • Reexport embedded-hal regarding the following change?

    Removed prelude to avoid method name conflicts between different flavors (blocking, nb) of the same trait. Traits must now be manually imported.

Copy link
Contributor

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request, @systec-ms! I've taken a very quick look and left a comment.

I think porting HALs over to embedded-hal 1.0(-alpha) in one go is the wrong approach though, for multiple reasons:

  • It's going to be one big change, which is always harder to review and get merged than multiple small ones.
  • I'm not aware of the latest plans, but embedded-hal 1.0 has been in development for a long time, and might not come out any time soon. Meanwhile, this pull request will have to be maintained, and will always be in danger of bitrot.
  • It's going to force the user to upgrade in one go, which might be challenging if they're using drivers that have not upgraded yet.

I think it's much better to use an iterative approach: Add support for the latest alpha in parallel to the latest stable version. Implement the alpha traits bit by bit, as the need arises. Phase out support for the older version once the ecosystem is ready.

src/spi.rs Outdated Show resolved Hide resolved
@systec-ms
Copy link
Contributor Author

Thanks, @hannobraun, as always.

  • It's going to be one big change, which is always harder to review and get merged than multiple small ones.
  • I'm not aware of the latest plans, but embedded-hal 1.0 has been in development for a long time, and might not come out any time soon. Meanwhile, this pull request will have to be maintained, and will always be in danger of bitrot.

I agree, however this was also intended more as a starting point and for discussion than to be merged. I just wanted to see how much needs to be done to implement it.

  • It's going to force the user to upgrade in one go, which might be challenging if they're using drivers that have not upgraded yet.

Furthermore, I'm not sure here, wouldn't it be better to make a hard cut and have a 0.x branch and a 1.x branch. In my opinion that is the point of major release and the embedded-hal does that. Also, @ryankurte already said here regarding the f4-hal that he would not expect both versions to be supported. On the other hand there is the Embedded HAL Compatibility Layer Crate.

@hannobraun
Copy link
Contributor

I agree, however this was also intended more as a starting point and for discussion than to be merged. I just wanted to see how much needs to be done to implement it.

Understood. However, I think merging support for the embedded-hal 1.0 alpha version is still desirable. There's really no point in having those alpha releases, if the ecosystem doesn't have an opportunity to experiment with them 😄

Furthermore, I'm not sure here, wouldn't it be better to make a hard cut and have a 0.x branch and a 1.x branch. In my opinion that is the point of major release and the embedded-hal does that. Also, @ryankurte already said here regarding the f4-hal that he would not expect both versions to be supported.

Well, I'm all for making the hard cut when that's the best option (which it often is because of limited developer resources), but in this case I believe it would actually be easier to do a smooth transition. I don't feel strongly about this, though. If you (or anyone else) want to develop embedded-hal 1.0 support in a fork and submit it all at once, that's fine by me.

However, I certainly won't accept two different branches for that purpose in this repository. In my (painful 😁) experience, developing multiple long-lived branches in parallel is a huge pain, and (approximately) never the right solution.

On the other hand there is the Embedded HAL Compatibility Layer Crate.

Ah, I kinda forgot this existed. However, it's noteworthy that this will explicitly not help anyone who still has older drivers, after we made a hard cut here.

@ryankurte
Copy link

Ah, I kinda forgot this existed. However, it's noteworthy that this will explicitly not help anyone who still has older drivers, after we made a hard cut here.

ahh, i need to update the readme there. as of v0.4.0 you can go both forwards and backwards (as per the docs).

@hannobraun
Copy link
Contributor

ahh, i need to update the readme there. as of v0.4.0 you can go both forwards and backwards (as per the docs).

Good to know! I guess that means we can just port peripherals over at our leisure, without needing to retain the old implementations.

@burrbull
Copy link
Contributor

@ghost
Copy link

ghost commented Jan 20, 2022

@hannobraun
hard cut? no branch? 1.x branch? anybody's fork? Can you summarize for me?

I believe it would actually be easier to do a smooth transition. I don't feel strongly about this

I think it's much better to use an iterative approach: Add support for the latest alpha in parallel to the latest stable version.

I certainly won't accept two different branches for that purpose

port peripherals over at our leisure

@hannobraun
Copy link
Contributor

@DarrylN

hard cut? no branch? 1.x branch? anybody's fork? Can you summarize for me?

"Hard cut" would mean to release a new version of stm32f7xx-hal that supports embedded-hal 1.0 and nothing else. This could be a pain for users, if it forced them to upgrade multiple dependencies at once, in a coordinated manner. However, as came to light in this discussion, there's embedded-hal-compat and it's both forward- and backward-compatible, so maybe users can use that to translate between their different dependencies and upgrade at their leisure.

I still think a more gradual approach would be beneficial, as migrating everything at once would force someone to do all the work at once. If we supported both versions, peripheral APIs could be upgraded one by one, at our leisure. Although thanks to embedded-hal-compat, we wouldn't have to support both versions at once in each peripheral API, and can just upgrade each API to the newer version.

I would recommend against creating a long-lived branch to do the upgrade, with the purpose of merging that branch all at once when the upgrade is done. That would require changes to the master branch to be merged into that 1.x branch, which is unnecessary and error-prone work.

If someone wanted to do the migration of all peripheral APIs at once, in their own fork, taking care of merging back any changes made in this repository here, and then submit that work in a single pull request, I wouldn't object to that. It would still not be my favorite approach, as reviewing one big thing is harder than reviewing many small things. And I wouldn't understand why someone would do that, when they could submit their work piecemeal. But who am I, to tell people how to use their time.

Does that answer your question?

@ghost
Copy link

ghost commented Jan 21, 2022

@hannobraun yes, that clears it up for me, thanks.

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.

4 participants