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

Rename DelayUs to BasicDelay, add delay_ns. #537

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Nov 28, 2023

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

@Dirbaio Dirbaio requested a review from a team as a code owner November 28, 2023 20:32
MabezDev
MabezDev previously approved these changes Nov 28, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@adamgreig
Copy link
Member

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
Copy link
Member

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?

MabezDev
MabezDev previously approved these changes Nov 28, 2023
@Dirbaio
Copy link
Member Author

Dirbaio commented Nov 28, 2023

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

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
Copy link
Member

Just to throw some other names into the mix:

  • DelayU32 - Rationale: All trait methods use a u32 instead of core::time::Duration which I guess more accurately describes why it's not just called Delay.
  • ImpreciseDelay or LowResolutionDelay - Depending on the implementor, there can be different behaviours, particularly for delay_ns.

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

@Dirbaio
Copy link
Member Author

Dirbaio commented Nov 28, 2023

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.

embedded-hal/src/delay.rs Outdated Show resolved Hide resolved
@MabezDev MabezDev enabled auto-merge November 28, 2023 21:57
@MabezDev MabezDev added this pull request to the merge queue Nov 28, 2023
Merged via the queue into rust-embedded:master with commit 0c05a0a Nov 28, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants