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

Remove traits with unconstrained Time associated types. #324

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Nov 3, 2021

As discussed in #201 and
#177 (comment) ,
the traits that have an unconstrained type Time associated type end up
not being usable for HAL-independent drivers in practice, since there is too
much variation across HALs of what the actual Time type is.

These should be bound to something, probably a Duration trait, so that
drivers can actually create and use them. Alternatively embedded-hal could have
actual struct Duration and so.

Either way, it's unclear what the final solution would look like. We shouldn't leave
these traits in the 1.0 release, since fixing them later will be a breaking change.
We should temporarily remove them, and add them back once we have figured this out
which won't be a breaking change.

As discussed in rust-embedded#201 and
rust-embedded#177 (comment) ,
the traits that have an unconstrained `type Time` associated type end up
not being usable for HAL-independent drivers in practice, since there is too
much variation across HALs of what the actual Time type is.

These should be bound to something, probably a `Duration` trait, so that
drivers can actually create and use them. Alternatively embedded-hal could have
actual `struct Duration` and so.

Either way, it's unclear what the final solution would look like. We shouldn't leave
these traits in the 1.0 release, since fixing them later will be a breaking change.
We should temporarily remove them, and add them back once we have figured this out
which won't be a breaking change.
@Dirbaio Dirbaio requested a review from a team as a code owner November 3, 2021 15:47
@rust-highfive
Copy link

r? @eldruin

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Review is incomplete T-hal labels Nov 3, 2021
@eldruin
Copy link
Member

eldruin commented Nov 4, 2021

Uh, this removes a few traits.
Before considering this, I would like to see an assessment of which public HALs implement them and which drivers use any of them to quantify how much downstream code would break.

@Sh3Rm4n
Copy link
Contributor

Sh3Rm4n commented Nov 9, 2021

I've created this source graph query on the bases of #177 (comment) to see, which crate is implementing these traits and also to give an overview, which Time types are used.

Based on the curated drivers list on awesome-embedded-rust and a quick and dirty script,
I also created a similar query for drivers the driver part (which sadly times out).

But even smaller queries did not lead to any matches at all. So this might be the wrong way to get an idea, which driver uses these traits. (Or there are just no matches at all)

But hopefully this creates a basis for discussion.

@eldruin eldruin added this to the v1.0.0 milestone Nov 9, 2021
@ryankurte
Copy link
Contributor

thanks for the analysis! there's definitely room for improvement on em, which either way will be a breakage i guess, but, i still feel a little wary of removing all these (also particularly interested in whether this has flow-on impact on RTIC timer definitions?)

i'm not sure this achieves all goals, but, what if instead of associated types we elected here to define a really basic timestamp / duration type (maybe something like Ts{ s: u32, us: u32 } inspired by the classic struct timespec?) and then to bound the traits by Into<Ts>. that way we're providing a simple primitive and if embedded-time (or any other approach) want to work with this it can be implemented outside the hal.

@Sh3Rm4n
Copy link
Contributor

Sh3Rm4n commented Nov 11, 2021

I feel like, in previous discussion and PRs it was made clear, that the most important priority e-h should solve, is to provide a consistent interface between drivers and MCUs, especially to be able to independently implement a half of the interface<->driver combination.

Everything, that misses that goal, should not be part of this crate. And, as of now, the traits which will be removed in this PR do miss the goal with the unconstrained Time type.

The fear of breaking many downstream crates while removing these traits is very real and very reasonable, so this problem should be solved.

i'm not sure this achieves all goals, but, what if instead of associated types we elected here to define a really basic timestamp / duration type [...]

Even though this sounds like an interesting idea, I propose we should not go this route. Any discussion about an alternative simple Time type sets us back to square one, where we have to come to an agreement how this simple solution should look like. With a discussion like that started, this is still a blocker for a v1.0 release, which this PR is deliberately trying to solve, by just removing these traits for now.

IMHO, more important right now is to solve the breakage issue for downstream HAL implementations and maybe inter-op between HAL impls and RTIC apps. First of, I don't think this is a blocker for this PR in particular, but we should have a solution for that concern before v1.0 is released.

I have a few ideas, but I'm not really particularly happy with anyone of these (maybe someone else can iterate ontop of these ideas):

  1. Break the world. Just remove them. This breakage could either lead to a big hurdle to move to v1.0 or to the situation, that the implementations just vendor / copy & paste these traits into the HAL implementations. This could lose the uniformity between these implementations over time, as adjustments to these traits can be made anytime. I'd argue this problem already exists, because of the widely different Time types used.
  2. Move these traits behind a unstable feature flag, for which the semantic versioning rules are loosened, and breaking changes are ok, at least between minor versions. This reminds me of the unproven flag, so this might be a bad idea. Anyway, this would allow the HAL implementations to go-on their lives to keep their current Time based implementations and the churn is not unnecessarily higher as it is already to move to v1.0.
  3. Move these traits to a companion crate (could still be part of this repository) to solve point 1., which is advertised as temporary from the beginning until the Time situation is solved. This crate could also be used as a testing bed to iterate on the Time design, including breaking changes until the WG is comfortable to move these into e-h again.

I'm out of ideas, but maybe there are other solutions as well.

@burrbull
Copy link
Member

@burrbull
Copy link
Member

PwmPin does not contain associated Time type. Why it was removed?

@Dirbaio
Copy link
Member Author

Dirbaio commented Dec 7, 2021

@burrbull it was designed to work together with Pwm which does have the type Time problem. IMO the safest is to remove both, I'm not sure the PwmPin trait makes sense "standalone".

We can redesign and add it back later, either as a "Pwm" peripheral with multiple pins/channels, or a standalone "PwmPin" that just allows setting duty % without "caring" about the full peripheral...

@burrbull
Copy link
Member

burrbull commented Dec 7, 2021

it was designed to work together with Pwm

It is not true. PwmPin was added much more earlier than Pwm

@Dirbaio
Copy link
Member Author

Dirbaio commented Dec 8, 2021

I stand corrected then!

I still think it suffers from a similar issue: it has an unconstrained type Duty; that can be integer, float... How can a HAL-independent driver make effective use of it?

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

As discussed in yesterday's meeting, let's remove all traits with unconstrained associated types for the 1.0 release and add them back later on piece by piece as we figure out what constraints we can specify on them, which would make them usable for HAL-independent code.
For HALs that implement them, they have the following options:

  1. Continue offering implementations using the traits from the embedded-hal 0.2.x versions which should reduce the ecosystem fragmentation and keep any dependent code working.
  2. Copy the removed traits into the HALs and keep everything as-is.
  3. Offer only inherent methods on the structs.
  4. Remove the implementations.

TODO before merging/releasing this:

  • Update changelog

@therealprof agreed yesterday as well.
Opinions @ryankurte ?

@ryankurte
Copy link
Contributor

i'm a little sad to see us dropping rather than replacing em, but, seems reasonable to get us unblocked. would it be useful to point folks to embedded_time::clock::Clock in the interim?

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Yeah, we should start some kind of "Get me those traits back" initiative and detail what the types could be :)
Ok, let's merge this as-is and I will update the changelog separately.
For anyone finding this in the future, if you are interested in any of the removed traits, please speak up in the issues we will open about this.
bors r+

@bors bors bot merged commit 2f5d994 into rust-embedded:master Feb 9, 2022
eldruin added a commit to eldruin/embedded-hal that referenced this pull request Feb 9, 2022
bors bot added a commit that referenced this pull request Feb 10, 2022
354: Remove Qei trait as it has an unconstrained associated type r=ryankurte a=eldruin

For now. See #324 for the reasoning.

Co-authored-by: Diego Barrios Romero <[email protected]>
@niclashoyer
Copy link

@eldruin please point to the issues you mentioned. I have several drivers I'd like to publish in the next months that depend on timer traits (especially CountDown). I'm using embedded-time with https://github.com/drogue-iot/drogue-embedded-timer as a compatibility layer to circumvent the problems mentioned here. I want to speak up to get (reasonably bound) timer traits back in!

@eldruin
Copy link
Member

eldruin commented Feb 13, 2022

Thanks for the reminder @niclashoyer.
Here is the issue with the overall roadmap for these traits: #357

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants