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

Move to embedded-hal 1.0 #118

Open
jbtheou opened this issue Jan 10, 2024 · 9 comments · May be fixed by #129
Open

Move to embedded-hal 1.0 #118

jbtheou opened this issue Jan 10, 2024 · 9 comments · May be fixed by #129

Comments

@jbtheou
Copy link
Contributor

jbtheou commented Jan 10, 2024

Now that 1.0 is released, is there a plan already to move the HAL to this version?

@no111u3
Copy link
Collaborator

no111u3 commented Jan 10, 2024

If you provide some implementation for it we are ready to use it.

@usbalbin
Copy link
Contributor

How do we want that to look like?

  • Move over completely to 1.0 and remove 0.2 support
  • Use both
    • Use both but behind features, 1.0 being enabled by default
    • Use both but behind features, 0.2 being enabled by default
  • Other?

@no111u3
Copy link
Collaborator

no111u3 commented Jan 11, 2024

I'm not sure about remove 0.2 causing feature incomplete of 1.0, but we can split trait implementation (internal/external) and use feature based flags for it (follows f4 & other hals).
0.2 support will be enable by default as in other hals now.

@techmccat
Copy link

I've been working on porting the SPI driver over to hal 1.0 with some optimizations using the FIFO and packed frames (and GPIO, which was mostly just copy-pasting code).

The code has been tested with a driver I'm working on and seems to work fine.
Right now it's just missing support for 16 bit frames, which should be a lot simpler than packed 8 bit.

@pdgilbert
Copy link

FWIW, my suggestion is to start a separate branch that is just using embedded-hal-1.0.0. That is what is being done in stm32h7xx-hal. The dual support in stm32f4xx-hal is pretty neat but the window for which it is valuable may be short, and it seems like a lot of work. You might re-consider if dual support is still interesting once an eh 1.0.0 version is working.

There seems to be a start made in branch hal-1 of @techmccat 's fork. It may be more than just changing to

[dependencies.embedded-hal]
version = "1.0.0"
package = "embedded-hal"

but that would be a beginning.

@techmccat
Copy link

techmccat commented Jan 31, 2024

I've gotten around to implementing all the new traits while keeping the old ones and fixing some examples, although i'm not quite satisfied with the delay, it could be much more high-res but the module needs a bit of rework (maybe something like the f4 hal with compile-time frequencies?).

Next comes updating the prelude and fixing the examples

@pdgilbert
Copy link

When you are ready, could you remove or change the 0.2.4 dependency to embedded-hal-old and make

[dependencies.embedded-hal]
version = "1.0.0"
package = "embedded-hal"

Then I will add your fork of stm32g4xx-hal to the active list I am testing at https://github.com/pdgilbert/rust-integration-testing/actions . At the moment I only have stm32f4xx-hal and stm32h7xx-hal active.

FYI, at the moment, with stm32f4xx-hal I have 33 examples compiling of the 67 that were compiling with eh 0.2.4, and with stm32h7xx-hal there are 23 of 67. The difference is stm32f4xx-hal magic support for both eh-1 and older. The magic might work on more of my examples, but many had shared-bus which fails now (see stm32-rs/stm32f4xx-hal#722 ). I am trying to convert to embedded-hal-bus but I cannot get that working with rtic (see rtic-rs/rtic#886).

@techmccat
Copy link

Done, examples all compile on g474
I had to change delay provider because cortex-m doesn't yet support DelayNs on SysTick

@pdgilbert
Copy link

pdgilbert commented Feb 11, 2024

I have forked @techmccat 's hal-1 branch to add the single line

pub use  stm32 as pac;

in lib.rs. This small change removes the need for a very large number of conditional statements in the setup functions for my examples. Somewhere I saw a general embedded-hal recommendation that this module be called pac rather than a manufacture/hardware specific name like stm32, but I can no longer find that recommendation. In any case, of the 11 stm32xxxx_hal's I was testing prior to moving to embedded-hal_1.0.0, stm32g4xx_hal was one of only 2 that still required using stm32 rather than pac. So, really this change should be put in the main branch too.

Regarding testing my examples with this fork, I have made very good progress. So much so that I am wondering if anything special was done to accomodate using embedded-hal_0.4.2 device crates?

There seem to be two many things I need to take care about. One is if the sensor device crate needs to consume a delay and does not yet accept DelayNs then I need to be careful to specify the delay type so that it is recognized as DelayUs. I have had better luck doing this with stm32g4xx_hal than with stm32h7xx_hal branch eh-v1.0. The second is when using embedded-io Read and Write. When I do this I can get both stm32g4xx_hal and stm32h7xx_hal versions of some examples compiling, but stm32f4xx_hal compiling fails because of stm32-rs/stm32f4xx-hal#721.

Of 69 examples I had working with many hals prior to the change to embedded-hal_1.0.0, I now have 49 compiling with the fork of stm32g4xx_hal. With stm32f4xx_hal 50 are compiling and 28 are compiling with stm32h7xx_hal branch eh-v1.0 (see discussion stm32-rs/stm32h7xx-hal#474). Details are at https://github.com/pdgilbert/rust-integration-testing/actions .

The way I choose to write some of the examples can affect these numbers because of the above issues. In general I lean toward trying to do things the embedded-hal_1.0.0 way but I am still learning what that means. Also, rtic's need for types rather than traits in Share and Local complicates a wholesale move to traits.

@techmccat techmccat linked a pull request Jul 30, 2024 that will close this issue
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 a pull request may close this issue.

5 participants