Added async delay and checked delay

[Artur-Romaniuk]

Delay lacked async implementation.

Needed it for my test so I added it.

[dbrgn]

Thanks! I left two comments.

[Artur-Romaniuk]

@dbrgn Fixed raised issues.

At the same time, the existing implementations weren't actually mocks, rather stubs. I added a new delay instance called CheckedDelay which is a proper mock with transactions.

[dbrgn]

Hi @Artur-Romaniuk, thanks for the update! I improved the module documentation and pushed a commit directly to your fork, I hope that's OK. I'll also rebase the branch against main, because there we changed the testing configuration (causing the tests to fail right now).

[dbrgn]

I'm really confused why the doc tests fail in CI... They work without any issues locally 😕 I'll try to investigate another day (but any pointers would be welcome).

By the way, @Artur-Romaniuk, I noticed that you used a different e-mail for the first commit compared to the others. If this was a mistake, now you could still fix it using git rebase 🙂

[Artur-Romaniuk]

Happy to see your changes. Will look at it tomorrow.

[tommy-gilligan]

Love that you've added a checked delay. This was something I needed so I came up with my own one of those (not as good ofc).

Not really that relevant to this PR but something minor and relevant enough that I figure I should bring it up here:
In looking at delay.rs on main I saw that currently methods with default implementations are being overridden. AFAICT only delay_ns needs to be implemented. I'm wondering if there's a good reason to override delay_ms/delay_us @dbrgn

Looking forward to this being merged. Let me know if there is any way in which I can help.

[dbrgn]

Thanks for the updates, @Artur-Romaniuk! I think the simplification with the nanosecond-only delay has improved the readability a lot!

I left some more comments. I hope that was the last round of review.

(Note to self: When merging, we should use squash mode.)

[dbrgn]

Not really that relevant to this PR but something minor and relevant enough that I figure I should bring it up here:
In looking at delay.rs on main I saw that currently methods with default implementations are being overridden. AFAICT only delay_ns needs to be implemented. I'm wondering if there's a good reason to override delay_ms/delay_us @dbrgn

I think you are right, thanks for bringing this up! @Artur-Romaniuk could you test if everything keeps working if you remove those potentially unneeded overrides?

[Artur-Romaniuk]

Not really that relevant to this PR but something minor and relevant enough that I figure I should bring it up here:
In looking at delay.rs on main I saw that currently methods with default implementations are being overridden. AFAICT only delay_ns needs to be implemented. I'm wondering if there's a good reason to override delay_ms/delay_us @dbrgn

I think you are right, thanks for bringing this up! @Artur-Romaniuk could you test if everything keeps working if you remove those potentially unneeded overrides?

I think there is a valid reason for doing it this way.
The way the std version is implemented with default implementations is that they call the delay_ns function for N amount of times in order to achieve us or ms. This means, that the mock would only receive delay_ns calls.

In that case, mock would have to add N expectations of delay_ns for each call of delay_ms. Not only does it complicate the expectation addition logic, but also expectation verification becomes much harder.
Look at this example:

delay_ms(5);
// do something
delay_ns(10);

using defaults, this would equate to

for(some N amount of times){
   delay_ns(MAX_NS);
} 
delay_ns(5 * NANOS_PER_MILI - N*MAX_NS);
// do something
delay_ns(10);

I believe the current impl is the easiest one and there is no benefit in using default impl.

[dbrgn]

I think there is a valid reason for doing it this way. (...)

You are right, thanks for your explanation!

[dbrgn]

I think this is now good to merge! Thanks @Artur-Romaniuk for your patience!