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

Upgrade to embedded-hal v1.0.0 #476

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Upgrade to embedded-hal v1.0.0 #476

wants to merge 13 commits into from

Conversation

richardeoin
Copy link
Member

@richardeoin richardeoin commented Dec 22, 2023

  • Use released version of embedded-hal v1.0 (when released)
  • Use released version of stm32-fmc (when released) Not required
  • Review I2C transaction implementation
  • Implement DelayNs for Timers. After DelayFromCountDownTimer was removed DelayNs is only implemented for SysTick
  • Improve the implementation of the delay_ns method in the DelayNs implementation on DelayFromCountDownTimer
  • Review and update doc comments
  • Low prio - add an example that uses DelayFromCountDownTimer

@pdgilbert
Copy link

I am stuck trying to create a second (non-SYST) delay. Do you have an example of how to do this now? I'm not having any luck with approaches I've used before. (Also, you might remove the example using DelayFromCountDownTimer in the comments in src/delay.rs and put in the new preferred way to do it.)

@richardeoin
Copy link
Member Author

It's not possible until there's another DelayNs implementation. This trait could just be implemented directly on the Timer types, or recreate a DelayFromCountDownTimer type to avoid the confusion that delay_us/ns will override any other functions that were setup on the timer.
I'll make a note to update the documentation for each module!

@pdgilbert
Copy link

@richardeoin could you clarify "not possible until there's another DelayNs implementation". Is that another embedded-hal implementation or an stm32h7xx-hal implementation? I'm confused because I have non-SYST examples working with stm32f4xx_hal, but they may be using that hal's magic for dual support of an older embedded-hal.

@mlamoore
Copy link
Contributor

mlamoore commented Dec 24, 2023 via email

@richardeoin
Copy link
Member Author

@pdgilbert Indeed I mean that more code needs to be added to this PR. I added a very minimal implementation of DelayFromCountDownTimer now, but the doc comments do still need to be reviewed and updated.

@pdgilbert
Copy link

pdgilbert commented Jan 3, 2024

Ok, thanks @richardeoin. That seems to work to get some of my examples compiling. Now the main problem is device crates using eh-1. For anyone interested, the CI is in the jobs for eh-1-rc3 and eh1-rc3-dev at https://github.com/pdgilbert/rust-integration-testing/actions .

@richardeoin
Copy link
Member Author

I think there is some worth in supporting both e-h v1.0 and e-h v0.2 traits to avoid dependency hell during the transition (including our own dependencies like stm32-fmc). I've implemented this a little, and rebased onto master

@ryan-summers
Copy link
Member

I think there is some worth in supporting both e-h v1.0 and e-h v0.2 traits to avoid dependency hell during the transition (including our own dependencies like stm32-fmc). I've implemented this a little, and rebased onto master

I actually would say to completely drop 0.2. I thought about this in the past and if there's support for the older version, it discourages people from actually putting in the work to upgrade things (which isn't actually very hard) and it increases the overhead on the HAL drastically

@pdgilbert
Copy link

I agree that having a hal support both eh v1.0 and eh v0.2 is slowing transition. It seems more productive to encourage users to move forward by having a clean implementation of the new way to do things, rather than trying to make it easy for them not to move forward.

Unfortunately just merging eh-v1.0 into master could be a bit difficult. There will be users that have critical dependencies on crates that are not updated, and they need more time to find a solution. For instance, in my test examples I am finding bus sharing and rtic are still hard to make work in combination with some other crates in eh v1.0.

One solution might be to have two branches, eh-v1.0 and eh-v0.2. Abandon master so no one gets confused about what is more stable. Then choose what is the default. My preference would be to soon make eh-v1.0 the default, so users have to specify the branch to indicate they really want the old version.

@ryan-summers
Copy link
Member

I inquired about this on the Matrix channel, and it sounds like the consensus is to support both 0.2 and 1.0 as best practice. This document helps describe best methods to do it as well.

@pdgilbert
Copy link

Thanks for the link @ryan-summers . That makes it sound easy. Glad there is a best practice. I look forward to having eh-1.0 in the master branch, and to getting a few more things working.

@ryan-summers
Copy link
Member

@richardeoin I had a thought to slowly add v1.0 e-h peripherals slowly as they get implemented in #504 instead of converting the whole repo at once (since its a lot). This way we can encourage users to add new support as they need it, too. Open to some thoughts here, just seems like this PR is a lot of effort on your part (which is greatly appreciated) :)

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