Simplify the sleep API (a followup on #555)

[ivmarkov]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo fmt command to ensure that all changed code is formatted correctly.
• I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
• My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

@fa993

This PR is adding changes on top of your changes in an effort to simplify the existing API.

Truth is, it is a bit bulky to use even if I think the API originally was my idea. :(
We have multiple make_* methods, each taking a bunch of Options.

To make it more user friendly, it should actually not take Options but rather - the actual values.
The thing is, the moment we do this, we have to implement a combinatorial explosion of make_* methods, each taking some combination of Wakeup sources.

I've work-arounded that by making (Light|Deep)WakeupSource chain-able, using the Empty and Chain structs.

This way - and in the end - you can have just one light_sleep method and just one deep_sleep method.

However the price to pay is that the new API does not preclude you from chaining multiple UartWakeups or TimerWakeups and so on.

This is not great, but it would still work in that the last wakeup source of the type would prevail.

There is an ever simpler option, which would be for light_sleep and deep_sleep to just take - roughly speaking - a &[&dyn wakeup_source1, ... &dyn wakeup_sourceN] slice, or in general - any IntoIterator where IntoIterator::Item: WakeupSource.

Which would get rid of Empty and Chain as well. The one issue here is that we need to use dyn or else we can't place wakeup sources of a different type (or even references to those) in a slice.

I might try this out in a follow-up commit, so that you could see how that looks.

Testing

Describe how you tested your changes.

[ivmarkov]

@fa993 If you compare the last commit I just pushed with the previous one you could see the Empty and Chain types being completely gone.

BUT... not 100% sure users would be able to cope with the &dyn traits (as if Rust without &dyn is not complex already).

On the other hand, wondering how often folks would use more than one wakeup source, which is when the need to use dyn comes up? But by that same logic, Empty and Chain also come up only if folks would use > 1 wakeup source.

In any case, my feeling is we should do it either with Empty and Chain or with dyn.
But retire the Options approach.

[fa993]

@ivmarkov I'll go through this changes in detail as well, but as of now, high level thoughts about these are:

1. I do not like the introduction of &dyn into the API (or slices in general), there are 2 particular reasons for it,
   a. Conceptually, a slice type (in an input type), represents an unbounded input (obviously this requirement is not strict), but in our case, there is a determinstic, finite number of combinations that we can accept. Additionally there is a smaller set of these combinations which are sound (I am referring to the example you pointed out, chaining multiple UartWakeup source structs, etc).
   b. What the earlier reason leads to, is that, considering rust specifically, I would say that we lose the entire thing of "If it compiles, it works as you intended", in those specific cases.

This seems like a perfect use case for the builder pattern, and rust has plenty of macros (third-party crates) for generating these builders based on these structs, would we be open to including them here? (Unfortunately, if we do go this route, we reduce boilerplate, but our struct would have to retain the Option semantics, additionally, we will face further problems with helper traits and things conditionally defined for a particular cfg (esp32c2, etc).

I'll munch on the implementation more, but these are my feelings on it as of now. Please let me know if any of this makes sense (or doesn't)

[fa993]

So the Empty and Chain approach is looking the most desirable to me right now, but I dislike the Empty component of it.

instead of saying Empty::chain(thing1).chain(thing2), we should be able to say thing1.chain(thing2). (this should be possible, I remember the earlier design also did this optimisation.

But the earlier issue about chaining multiple sources of the same type still remain, which is the unfriendliest part of the design proposed so far (to me), if we can somehow figure that out, I think the design would be very clean, and more importantly, sound.

[ivmarkov]

b. What the earlier reason leads to, is that, considering rust specifically, I would say that we lose the entire thing of "If it compiles, it works as you intended", in those specific cases.

"If it compiles it works as you intended" is not black and white. It is a gradient. True: Rust pushes the gradient much further than other languages (due to its sophisticated type system), but it does have its limits too. Or else we would not have had bugs in Rust code-base. Or in other words, Rust is not Agda, Idris or Coq where your type system ends up being theorems.

This seems like a perfect use case for the builder pattern, and rust has plenty of macros (third-party crates) for generating these builders based on these structs, would we be open to including them here?

I'm open for anything as long as it does not take 2 years and 2000 lines of code to implement what is otherwise a relatively small aspect of the HAL crate.

The thing is, even with builders I'm not very optimistic.

There are just too many #cfgs is the code (by necessity) and then you'll end up needing different generics on your builder for different chips which will end up in a very difficult to maintain code-base with lots of repetitions.

So unless you take the effort and can demonstrate a better API that the current two options (dyn or chains) I'm not willing to go that route.

Also, I hope it is clear that the current
"a bunch of sleep methods convering some permitations and then lots of places where you have to pass Option::<what-the-hell-should-I-put-here???>::None"
API is sub-optimal w.r.t. usability and maintainability to both the dyn and the chain approaches.

I'll munch on the implementation more, but these are my feelings on it as of now. Please let me know if any of this makes sense (or doesn't)

As per above - if you have time, give it a try. Perhaps you have more experience in Rust than me. But if you are just starting with Rust a warning - there will be dragons down the road.

[ivmarkov]

So the Empty and Chain approach is looking the most desirable to me right now, but I dislike the Empty component of it.

But the earlier issue about chaining multiple sources of the same type still remain, which is the unfriendliest part of the design proposed so far (to me), if we can somehow figure that out, I think the design would be very clean, and more importantly, sound.

Empty + Chain on one hand, and the dyn approach on the other are pretty much the same in terms of usability.
The problem with Empty + Chain is that it introduces two extra structures which the dyn method gets-by without.

As for the unfriendliest part - see my earlier comment.

instead of saying Empty::chain(thing1).chain(thing2), we should be able to say thing1.chain(thing2). (this should be possible, I remember the earlier design also did this optimisation.

I think this can be done by moving the chain method down to the LightSleep / DeepSleep traits.

[fa993]

"If it compiles it works as you intended" is not black and white. It is a gradient. True: Rust pushes the gradient much further than other languages (due to its sophisticated type system), but it does have its limits too. Or else we would not have had bugs in Rust code-base. Or in other words, Rust is not Agda, Idris or Coq where your type system ends up being theorems.

True, I agree, shouldn't stop us from trying to be as sound as possible. But maybe in this context, the goal could be a bit too ambitious. Let me iterate on this as well.

I'm open for anything as long as it does not take 2 years and 2000 lines of code to implement what is otherwise a relatively small aspect of the HAL crate.

The thing is, even with builders I'm not very optimistic.

There are just too many #cfgs is the code (by necessity) and then you'll end up needing different generics on your builder for different chips which will end up in a very difficult to maintain code-base with lots of repetitions.

So unless you take the effort and can demonstrate a better API that the current two options (dyn or chains) I'm not willing to go that route.

Agreed, this shouldn't be the cause of long term tech debt or a blackhole of efforts, but for the sake of trying, let me take a stab at this as well.

Also, I hope it is clear that the current "a bunch of sleep methods convering some permitations and then lots of places where you have to pass Option::<what-the-hell-should-I-put-here???>::None" API is sub-optimal w.r.t. usability and maintainability to both the dyn and the chain approaches.

Noted.

As per above - if you have time, give it a try. Perhaps you have more experience in Rust than me. But if you are just starting with Rust a warning - there will be dragons down the road.

Haha, I very much doubt that I have more experience than you. In either case, I'll give this a shot as well, I'll push changes to my PR and then we can compare across branches (your PR and mine) which approach would be better? Would that work?

[ivmarkov]

In either case, I'll give this a shot as well, I'll push changes to my PR and then we can compare across branches (your PR and mine) which approach would be better? Would that work?

Yep!

[ivmarkov]

@fa993 Do you still pan to work on this?

[fa993]

@ivmarkov Yeah, got busy this past month, I’ll get some time to work on this over the weekend.

[fa993]

@ivmarkov I am commenting here so as to not lose the thread of our conversation, I was iterating on this design, and I realised that the root of our problems is that we are storing the pins, their thresholds, etc etc in a struct and then calling the prepare() and sleep() functions on it, what if, we removed the middleman, we could make our structs stateless, have them call the configuring functions directly, since each wakeup source is independent of the other, this approach would drastically reduce the code footprint of the implementation, and also we don't have to go into the nasty generics and the dyn vs chain tradeoff (for the most part) I've committed a version of what that could look like.

The difference in the approaches can be summarised as earlier we were doing

configure(source1) -> configure(source2) -> prepare() (where actual c functions were called) -> sleep()

Now we are doing

configure(source1) (source1 specific c functions are called here) -> configure(source2) (same logic) -> sleep()

Let me know if there are some drawbacks or things that are non-negotiables with this approach.

Edit: The CI on my PR seems to be failing because of some transient 504 gateway timeout while running the install steps, I can't seem to find the re-run jobs button, how do I retrigger the CI?

[ivmarkov]

@ivmarkov I am commenting here so as to not lose the thread of our conversation, I was iterating on this design, and I realised that the root of our problems is that we are storing the pins, their thresholds, etc etc in a struct and then calling the prepare() and sleep() functions on it, what if, we removed the middleman, we could make our structs stateless, have them call the configuring functions directly, since each wakeup source is independent of the other, this approach would drastically reduce the code footprint of the implementation, and also we don't have to go into the nasty generics and the dyn vs chain tradeoff (for the most part) I've committed a version of what that could look like.

The difference in the approaches can be summarised as earlier we were doing

configure(source1) -> configure(source2) -> prepare() (where actual c functions were called) -> sleep()

Now we are doing

configure(source1) (source1 specific c functions are called here) -> configure(source2) (same logic) -> sleep()

Let me know if there are some drawbacks or things that are non-negotiables with this approach.

Edit: The CI on my PR seems to be failing because of some transient 504 gateway timeout while running the install steps, I can't seem to find the re-run jobs button, how do I retrigger the CI?

@fa993

What I don't see in your latest changes is the solution of the main problem I thought you aim to solve. Concretely:

b. What the earlier reason leads to, is that, considering rust specifically, I would say that we lose the entire thing of "If it compiles, it works as you intended", in those specific cases.

Your solution erases generics, so maybe we should consider it, but in the end it does not solve the ^^^ problem?

I mean, your solution does not stop me from configuring a timer 10 times, and a uart 15 times, just like the "chain" and "dyn" solution don't.

I am missing a bit the "builder" pattern here I thought you plan to explore?

[fa993]

@ivmarkov I was exploring the builder pattern, but turns out, you were right, the implementation grew too complex too quickly, if we move towards storing everything inside a struct first, we have to deal with nasty generic type bounds where depending on a particular cfg gate, we may need to keep Empty (No Op) implementation structs for a particular marker trait, which seems ugly.

My current proposed solution has at the least the same pitfalls as yours, but it's cleaner in terms of code.

[fa993]

Hi @ivmarkov any updates on this?

[ivmarkov]

Hi @ivmarkov any updates on this?

Sorry for the delay :(
Will look into it tmr.