unify DelayUs and DelayMs to DelayUs for v1.0

[almindor]

Implements a unified DelayUs using u32 only for v1.0 of embedded-hal to prevent confusion.

Keeps Delay open for future, better abstractions.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eldruin (or someone else) soon.

Please see the contribution instructions for more information.

[eldruin]

Could you add an entry to the changelog?

[eldruin]

Sorry, one last thing, could you also squash this into one commit?

[eldruin]

Context:
We discussed this in today's WG meeting and removing DelayMs in favor of DelayUs seemed like an acceptable compromise. The goal being coming up with an unified Delay trait once we figure out constraints for its type Time: ???.
@therealprof also agreed to this.

Do you have any objections @ryankurte ?

[Dirbaio]

damnit almindor you beat me (my fault for not looking first 🤦 )

here's the summary I wrote:

---

Quick summary of discussions in #177 and in today's REWG meeting:

• We need a hardawre-agnostic, efficient way of representing time durations for use in the e-h traits, but that's probably far away.
• The current DelayMs/DelayUs traits are not the answer, they require duplicating impls for each unit and each integer size.
• There's issues where HALs don't implement all DelayMs/DelayUs combinations, causing some drivers to not work with them.
• However, "blocking delay" is an incredibly important and widely used trait, releasing 1.0 without it is not an option.

The compromise found to unblock 1.0 while mitigating the issues is:

• remove DelayMs
• change DelayUs to use u32 instead of generic width integers.

Why:

• Fixes the pain of HALs not implementing all combinations.
• For the most common use cases (blinks, waiting for chips to initialize...) microsecond precision is enough.
• u32 allows for a max delay of ~1.2h. Such blocking delays should be extremely rare, as the core can do absolutely nothing else during it (unless it uses interrupts, but then you're probably already doing interrupt-based timers).
• The runtime cost of forcing using 32bit is negligible, most HALs are already doing 32bit math on the duration to convert it to clock ticks.

The name Delay is reserved for the future trait that will use the new shiny Duration representation, hence DelayUs is kept even though there's only one trait.

When the final Delay trait is released, we may (or may not if we still think it's handy) deprecate DelayUs.

[Disasm]

I'm somewhat against the situation where we have only DelayUs trait because it gives wrong feeling of having a precise enough microsecond delay. While we have no guarantees on upper bound of actual delay for a given requested delay, it's a good idea to avoid situations when a delay implementation works type-system-wise, but doesn't work in runtime.

[almindor]

I'm somewhat against the situation where we have only DelayUs trait because it gives wrong feeling of having a precise enough microsecond delay. While we have no guarantees on upper bound of actual delay for a given requested delay, it's a good idea to avoid situations when a delay implementation works type-system-wise, but doesn't work in runtime.

The main problem with two traits is that if a HAL doesn't implement one of them it blocks Drivers that use the other one from being used. The idea here is to avoid such cul-de-sacs.

A proper solution is to introduce a new Time type and figure out all the nuances of delays and then introduce Delay as a trait that uses it, but that won't happen before v1.0 is out.

[Dirbaio]

Perhaps the docs should specify "delays for at least X us, but may delay for longer, if for example the implementation doesn't have enough precision"?

Delaying for shorter could break drivers that need to wait for at least X us for some chip to do some operation, delaying for longer usually doesn't.

This would require HALs to round up instead of down if there's not enough precision, and warn driver authors to not rely on it for extremely precise timing.

[almindor]

Perhaps the docs should specify "delays for at least X us, but may delay for longer, if for example the implementation doesn't have enough precision"?

Delaying for shorter could break drivers that need to wait for at least X us for some chip to do some operation, delaying for longer usually doesn't.

This would require HALs to round up instead of down if there's not enough precision, and warn driver authors to not rely on it for extremely precise timing.

I like this approach, @Disasm what do you think?

[Disasm]

Yes, this is one of the solutions. I still don't understand why it's better than the "best effort" solution when the provided delay tries to work with a good precision. We can't guarantee this precision of course, but type system provides a way to define it at least on some level by defining two separate traits, and this PR removes the difference between old DelayMs and DelayUs. Imagine that we have a Pin trait instead of all the other pin traits which sometimes doesn't work as expected. Driver wants to use it as output, but if user configured it as input, it will not work. Same here: if the user took a bad delay source, it will not work in a situations when you need a microsecond delay. If you need to toggle GPIO fast enough, but with a tiny delay, you would use delay_us for this, but if this delay gives you 2+ms delay each time, you will not get what you expect.

[almindor]

Yes, this is one of the solutions. I still don't understand why it's better than the "best effort" solution when the provided delay tries to work with a good precision. We can't guarantee this precision of course, but type system provides a way to define it at least on some level by defining two separate traits, and this PR removes the difference between old DelayMs and DelayUs. Imagine that we have a Pin trait instead of all the other pin traits which sometimes doesn't work as expected. Driver wants to use it as output, but if user configured it as input, it will not work. Same here: if the user took a bad delay source, it will not work in a situations when you need a microsecond delay. If you need to toggle GPIO fast enough, but with a tiny delay, you would use delay_us for this, but if this delay gives you 2+ms delay each time, you will not get what you expect.

The problem is that if the HAL decides to implement only one for whatever reason, it locks out many drivers that might expect the other. This happened on the e310x-hal which we then implemented with precision that only goes down to ~30us anyhow, so the two trait system didn't really prevent precision issues.

I don't think we can prevent precision issues/force proper precision on implementors until we have a better type around the delays defined.

[Disasm]

Well, that's how traits work. If your implementation has some functionality, it implements a trait, otherwise it doesn't. If the delay from e310x-hal doesn't implement DelayUs, it's only because it's a bad delay and we need a better delay instead of making a fake DelayUs implementation for the bad one.

[Dirbaio]

Many places where you need a delay are fine with "delay for at least X" and don't break if the delay is longer than requested. Stuff like "after deasserting RST wait for at least 500us before sending commands". I'd argue this is the majority of DelayMs/DelayUs uses today.

For use cases that do break when the impl delays for longer, DelayMs/DelayUs isn't a good choice anyway. The code can be preempted and arbitrarily delayed anyway by interrupts, it should be interrupt-driven, not blocking.

The advantage of documenting that DelayUs does "delay for at least X" is that it solves the compatibility problem: all HALs will be capable of implementing it, so drivers using it will work everywhere. The disadvantage is the "precision delay" use case is left unsolved, but we can address that with the "fancy" Duration trait we'll add in the future.

[ryankurte]

Do you have any objections @ryankurte ?

this all seems pretty reasonable to me. we do need a basic delay function like this for constructing drivers (and i suspect this is the majority of existing uses), i certainly wouldn't want it removed without an equivalent replacement.

Many places where you need a delay are fine with "delay for at least X" and don't break if the delay is longer than requested.

at least with linux users, subject to the whims of the scheduler, this is already the case. documenting it as "delay for at least X" sounds great. this trait is never going to do a great job for bit-bashing, that said, if you need more accuracy you can switch to a delay implementation able to provide it, we're just not guaranteeing it at the abstract HAL level.

[almindor]

Extended comments to include at minimum for requirements on pausing.

@ryankurte @Dirbaio @Disasm @eldruin @adamgreig would it make sense to also return the value actually used? E.g. turn it into Result<u32, Self::Error> so that in case the implementation needed to block longer the user can know the approximation of how much?

[adamgreig]

would it make sense to also return the value actually used? E.g. turn it into Result<u32, Self::Error> so that in case the implementation needed to block longer the user can know the approximation of how much?

My first thought is that measuring that accurately could add a lot of complexity to HAL impls, and I don't think many use cases would benefit from finding out (i.e. they'd probably just ignore the number, as usually there's nothing to be done about it if the delay was longer than expected after it's already happened). Maybe it would be a neat feature to add to a future Delay trait as an option?

[eldruin]

Looks good to me as-is, thanks!

[Dirbaio]

Some last-minute bikeshedding 😅

If the trait has methods delay_ms, delay_us perhaps DelayUs is not the best name?

Delay would make more sense, but we want to keep that for the trait using the future "Duration" abstraction. Maybe SimpleDelay, BasicDelay? Or DelayMsUs lol...

[Sh3Rm4n]

If the trait has methods delay_ms, delay_us perhaps DelayUs is not the best name?

Even though it sounds a bit weird, but DelayUs is the precision and delay_ms / delay_us is the actual delay duration, so I think we should keep these names and probably make it more clear in the documentation?

[Dirbaio]

The 'precision' of the impl can be whatever, there'll probably be impls using a 32khz low-frequency clock or whatever.

It was just a random thought anyway, wanted to share to see what other people think. I'm OK with DelayUs too.

[almindor]

The 'precision' of the impl can be whatever, there'll probably be impls using a 32khz low-frequency clock or whatever.

It was just a random thought anyway, wanted to share to see what other people think. I'm OK with DelayUs too.

It's DelayUs to keep same name as before, for familiarity and because we decided Delay is reserved for the "proper" abstraction. I could name it something else but then it'd be even more confusing IMO.

[therealprof]

As agreed, let's get this merged.

bors r+

[bors]

Build succeeded:

• ci-linux (1.35.0, x86_64-unknown-linux-gnu)
• ci-linux (stable, thumbv6m-none-eabi)
• ci-linux (stable, thumbv7m-none-eabi)
• ci-linux (stable, x86_64-unknown-linux-gnu)
• ci-linux-test (1.36.0, x86_64-unknown-linux-gnu)
• ci-linux-test (stable)
• fmt