rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.01k stars 202 forks source link

Rename `DelayUs` to `BasicDelay`, add `delay_ns`. #537

Closed Dirbaio closed 11 months ago

Dirbaio commented 11 months ago

As discussed in today's meeting, we should add nanosecond precision to the delay trait.

Fixes #521 Fixes #535

Though as discussed in the meeting as well, we should add configuration settings to the SpiDevice impls in embedded-hal-bus to do the delays, and document drivers are discouraged from using Operation::DelayNs for CS-to-clocks delays. Opened #537 to track this

adamgreig commented 11 months ago

What makes it "Basic"? I still prefer DelayNs in line with having DelayUs before, assuming we save Delay for some future trait, but maybe it's too confusing since it provides all three methods?

I think of it as "delay with ns resolution", plus methods to delay for longer units which can trivially be done with more nanoseconds.

MabezDev commented 11 months ago

What makes it "Basic"? I still prefer DelayNs in line with having DelayUs before, assuming we save Delay for some future trait, but maybe it's too confusing since it provides all three methods?

I also thought about making this point, but I guess as an end user with no knowledge of these traits I wouldn't assume DelayNs would be able to do other resolution delays. It's a bit subjective, to be honest. Maybe we can trial it in an rc and see what implementors think?

Dirbaio commented 11 months ago

it's "basic" as "not using the fancy hypothetical future Duration type that will magically solve all our problems"... :see_no_evil:

I think BasicDelay makes more sense than DelayNs, because DelayNs implies we also have DelayUs, DelayMs, which we don't. Also it might throw off users looking for microsecond or millisecond delays?

No strong opinion from me, I'm fine with DelayNs too. If people think DelayNs is better I'll change it.

MabezDev commented 11 months ago

Just to throw some other names into the mix:

I have no strong opinions on any of the suggestions so far, I'd be happy with any of them.

Dirbaio commented 11 months ago

DelayU32

same as DelayNs, it implies we have other traits in the set like DelayU16 but we don't.

ImpreciseDelay or LowResolutionDelay

hmm I'm not sure, there's nothing inherently "imprecise" about it, it just rounds up if needed. The Delay trait with the magic rainbow unicorn Duration type will also likely be spec'd to round up if the hardware can't reach the requested precision, it won't be more precise.