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

sleep: Adds module for light and deep sleep with examples #288

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

oyvindnetland
Copy link
Contributor

A suggestion for an API for light/deep sleep functionality. Tried a few different approaches, but found this to be the one that made the most sense.

The goals that caused me to end up with this API:

  • Wanted to contain the information about a sleep in a struct and then do the sleep by running the structs sleep().
  • The sleep struct could have multiple wakeup sources.
  • Share as much code between light and deep sleep as possible.
  • Allow "shortcuts" for single wakeup source sleeps: LightSleep::timer(Duration::from_secs(5))?.sleep()?;.

This PR just implements Timer and UART wakeup sources. The reason for doing these two is to demonstrate how two different sources work together, and to show how to handle that one type of source isnt allowed for one type of sleep (no uart wakeup during deep sleep).

This is very much a first draft, so im absolutely open for suggestions. Also have to admit that i still done feel "fluent" in rust, so I might have missed some clever rust stuff

related to #287

src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated
esp!(unsafe { esp_sleep_enable_timer_wakeup(dur.as_micros() as u64) })?;
}
WakeupSource::Uart {
uart_num,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This smells unsafe. Shouldn't you pass an actual UART peripheral here? As in Peripherals::uart0? Also, I'm a bit unclear on that - is any setup of the UART peripheral necessary so that it can be used as a wakeup source?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, shouldn't you actually even pass the configured UART driver here? I mean, if the UART peripheral is not enabled (via the driver) for receiving data, how are we supposed to actually be awoken by it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention here was that you need to set up the UART outside of the sleep driver, and it would then of course not work if that is set up incorrectly. My light sleep example works since the stdio UART has already been set up (I assume).

Passing an UART that has been configured, and then get the UART number for that sound plausible and reasonable, so ill check that

src/sleep.rs Outdated
}
WakeupSource::Uart {
uart_num,
threshold,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shuldn't this be u16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to cargo build it expects a i32

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but that's just an oddity of the C API. Looking at the documentation, it should be between 3 and 0x3ff, right? Which is u16.

src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated
Ok(())
}

fn sleep(&self) -> Result<(), EspError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more thing to add: I think you are misunderstanding how deep sleep is supposed to work, hence you are equating the deep sleep API with the light sleep API:

  • In light sleep - once you call esp_light_sleep_start, the chip enters light sleep, but if you are awoken from light sleep, the execution of the program continues right after the call to esp_light_sleep_start
  • In deep sleep - once you call esp_deep_sleep_start, the chip enters deep sleep which means ALL the state of the program is completely erased. When you are awoken from deep sleep, the execution of the program will start from the very beginning - with the bootloader, and everything, including - from the beginning of your main function. So in a way, the deep_sleep. function signature should be fn deep_sleep(...) -> ! to signify that this function never returns execution control back to the caller.

Copy link
Collaborator

@ivmarkov ivmarkov Aug 14, 2023

Choose a reason for hiding this comment

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

I also would suggest you create a modify your deep_sleep example. This way you'll be able to test what I'm saying for real.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do understand the difference, thats why my light and deep examples differ, even if they use a "similar" api.

The light sleep example starts and infinite loop and the sleep is basically just like any other sleep/delay. The wakeup reason is checked after each wakeup.

The deep sleep example however just check reset/wakeup reason, then do a deep sleep at the end of the program, and expect to end up at the start again afterwards.

I agree that the deep sleep return type might be abit confusing (more on that later), but apart from that im not sure i understand your concern here

Copy link
Collaborator

Choose a reason for hiding this comment

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

My biggest problem with the current approach that I'm now constantly repeating all over the comments is that I don't see the need to treat light and deep sleep in a uniform way. Do you actually have this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My biggest problem with the current approach that I'm now constantly repeating all over the comments is that I don't see the need to treat light and deep sleep in a uniform way. Do you actually have this use case?

Well, at least you successfully convinced me finally, see below. Agree that we probably should have had one thread instead of 3-4, since it was basically the same root concept that was discussed

examples/deep_sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated
unsafe { esp_deep_sleep_start() };
#[allow(unreachable_code)]
// if function returns, it means that deep sleep has been rejected
Err(EspError::from_infallible::<ESP_FAIL>())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. This would prevent our deep_sleep function from having the -> ! signature. :(
Shall we panic! here instead, if the esp_deep_sleep_start function fails to run? Not sure myself, but might be the better option, given that we check enough preconditions before entering deep sleep, to make sure that panicing would be next to impossible - as in checking for a too short sleep interval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is abit confusing, I would say that -> ! would make alot of sense here, but didnt want to have that since the documentation says that esp_deep_sleep_startmight actually return.

Seems to me that the only reason that would happen is that the timer duration is too short, but i was not able to find out what "too short" means. So not sure we can check that?

My reasoning here was that you actually can "try" to deep sleep, but if that doesnt work you can actually check that and do whatever, try again perhaps? Doing a panic here doesnt "feel right", since you can end up doing something different that you asked for.

But then again... maybe panic is a good solution, it will make the function signature more self explanatory. If the only way to get to that panic is to ask for a too short deep sleep duration. And the functional difference between a panic and a very very short deep sleep is not much i guess.

@oyvindnetland
Copy link
Contributor Author

ill see that comments started coming in as I tried to fix no std issues and just created more issues, so ill pause that for now and will answer comments shortly

@oyvindnetland
Copy link
Contributor Author

oyvindnetland commented Aug 14, 2023

to sum up now that I have read through and thought about all comments:

  1. probably do -> ! for deep sleep, and just panic if it fails (do precheck of timer duration so its reasonable >1ms or something?)
  2. that will break the Sleep trait, so then Light and Deep will become separate structs
  3. I dont want to give up the shared wakeup source concept, at least try that for another iteration cycle.
  4. try to make a heapless vector solution

@ivmarkov
Copy link
Collaborator

To sum up my take on this:

to sum up now that I have read through and thought about all comments:

  1. probably do -> ! for deep sleep, and just panic if it fails (do precheck of timer duration so its reasonable >1ms or something?)

Medium priority. I think in the end we can live with either signature.

  1. that will break the Sleep trait, so then Light and Deep will become separate structs

Not quite. That is an important yet not the primary reason I think the Sleep trait should be gone; the primary reason is because there is not enough overlap (I think) between light and deep sleep sources and treating them uniformly will bring more confusion than benefits. Folks who usually don't read the ESP IDF documentation will be confused that e.g. UART can do deep sleep wakeup (can it?) or that you can use any GPIO pin to wake up from deep sleep (you can't; you can only do this in light sleep, right?) and so on.

  1. I dont want to give up the shared wakeup source concept, at least try that for another iteration cycle.

Sure, though I do not understand why you think this brings any benefit? If you can show me a real life example where you want to abstract over light and deep sleep and treat these two uniformly, I can change my mind, but for now I see this a bit as a "bringing abstractions just because we can" exercise, which might actually sacrifice something more valuable - type safety, where users just can't get it wrong, even if they haven't checked the ESP IDF documentation carefully.

  1. try to make a heapless vector solution

Sure, but that's a detail.

@oyvindnetland
Copy link
Contributor Author

Sure, though I do not understand why you think this brings any benefit? If you can show me a real life example where you want to abstract over light and deep sleep and treat these two uniformly, I can change my mind, but for now I see this a bit as a "bringing abstractions just because we can" exercise, which might actually sacrifice something more valuable - type safety, where users just can't get it wrong, even if they haven't checked the ESP IDF documentation carefully.

Thanks, now I got your point :) I understand now that my priorities were incorrect in this case, and that trading a bit code duplication to make it as easy to use as possible. Ill give it another go soonish

@oyvindnetland
Copy link
Contributor Author

I have been tinkering with the API now, and struggle to get it just right. So @ivmarkov , I would hope to pick your brain abit more.

Your pub fn light_sleep(wakeups: &[LightWakeupSource]) suggestion works and looks great for the simpler cases, but when adding more wakeup sources its gets messy since alot of runtime checks is needed. (e.g. you cant have Ext wakeup and touch wakeup at the same time, and probably shoudlnt have two timer wakeup sources)

To get some overview here, I set up a simple table parsed from ESP-IDF docs.

Type        Light   Deep    Comment
-----------------------------------------------------------------------------------
Timer     | y       y
UART      | y       
EXT0      | y       y       Cannot be used with ULP or Touch
EXT1      | y       y       Not specified, but assume also not usable with ULP and Touch
Touch     | y       y
GPIO      | y               Cannot be used with ULP or Touch
ULP       | y       y
-----------------------------------------------------------------------------------
Out of current scope
BT        | y
WIFI      | y
COCPU     | y       y       Not really documented in ESP-IDF
-----------------------------------------------------------------------------------

In addition, I would argue that there is no reason to have GPIO and RTC at the same time.

So, what do you think of a skeleton structure like this. I believe that would enforce the "rules" compile time, and in a way its very explicit about how its configured. But it feels a bit complicated.

pub struct TimerWakeup {
}

pub struct UartWakeup {
}

pub enum LightSleepIOWakeup {
    Ext0,
    Ext1, // might be possible to combine ext0 and ext1
    Gpio,
    UlpAndTouch,
}

pub struct LightSleep {
    timer: Option<TimerWakeup>,
    io: Option<LightSleepIOWakeup>,
    uart: Option<UartWakeup>,
}

@ivmarkov
Copy link
Collaborator

Your pub fn light_sleep(wakeups: &[LightWakeupSource]) suggestion works and looks great for the simpler cases, but when adding more wakeup sources its gets messy since alot of runtime checks is needed. (e.g. you cant have Ext wakeup and touch wakeup at the same time, and probably shoudlnt have two timer wakeup sources)

If that's the case, you might just model this with a

pub enum TouchOrExt0Wakeup {
    TouchWakeup(...),
    Ext0Wakep(...),
}

(UPDATE: I found out you are modelling it exactly like that below.)

With that said, ext0 and touch (and ULP) seem to be only incompatible with each other on ESP32 chip revision < 2. Now that might be too much of a detail to capture at runtime, as we cannot even detect the chip revision at compile time (not sure whether we can do it at runtime either).

In fact, a lot of these incompatibilites in the table below are precisely because of ESP32 revision 0 or 1, aren't they?
Specifically:

Timer | y y
UART | y
EXT0 | y y Cannot be used with ULP or Touch

^^^ ESP32 rev 0/1 only

EXT1 | y y Not specified, but assume also not usable with ULP and Touch

^^^ Ditto?

Touch | y y
GPIO | y Cannot be used with ULP or Touch

I can't find anything mentioned about such an incompatibility? Given that the GPIO subsystem is completely separate from the RTC subsystem, while all of ext0/ext1/touch/ULP are in a way related to the RTC subsystem, I find your incompatibility statement hard to believe, actually.

ULP | y y

Out of current scope
BT | y
WIFI | y
COCPU | y y Not really documented in ESP-IDF

COCPU is ULP. ULP comes in two flavours: FSM ULP (on ESP32, ESP32S2 and ESP32S3) and RISCV ULP (on ESP32S2 and ESP32S3)
. So in a way, esp32s2 and esp32s3 have both, but only one can be active (and that's an esp-idf compile-time configuration even).


In addition, I would argue that there is no reason to have GPIO and RTC at the same time.

So, what do you think of a skeleton structure like this. I believe that would enforce the "rules" compile time, and in a way its very explicit about how its configured. But it feels a bit complicated.

I think it is actually quite OK. Some specific comments below.

pub struct TimerWakeup {
}

pub struct UartWakeup {
}

pub enum LightSleepIOWakeup {
    Ext0,
    Ext1, // might be possible to combine ext0 and ext1
    Gpio,
    UlpAndTouch,
}

As per above, you probably don't need "one of" ext0, ext1, gpio, touch, ulp, as this is ESP32 rev 0/1 only, and we cannot detect that.

pub struct LightSleep {
timer: Option,
io: Option,
uart: Option,
}

That ^^^ would work, for both light and deep sleep (of course, modulo some wakeups for deep sleep).
I don't find this overly complicated, to be honest. Where is the complexity? You can even expose this structure pith pub members directly.

@oyvindnetland
Copy link
Contributor Author

After last post i basically decided that this new way is better, and definatly not complicated. So glad you agree, will update pr shortly, so we can continue discussions based on actual code.

Ill double check what can run at the same time and not, and as far as i can try to test this on hardware (just got esp32s3 and esp32c3 dev boards in the mail, yey) I might have been tripped up by the esp32 early versions special cases

@oyvindnetland
Copy link
Contributor Author

So, took abit of time, but here is a slightly new look at this, with more complete API and examples. I think most comments have been addressed, but due to significant changes i assume some new might pop up.

I think it works pretty well as is, but there are a few places where im uncertain if the best solution was found or I wasn't able to get it exactly as I wanted, especially around types for gpio, rtc and uart wakeup types.

Touch and ULP wakeup sources only turns the wakeup on, and assume that these things has been configured beforehand. Not really experienced with either, so not sure if there is a better way to do this.

src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Show resolved Hide resolved
src/sleep.rs Outdated
/// Will wake up the CPU when a touchpad is touched.
#[cfg(any(esp32, esp32s2, esp32s3))]
#[derive(Debug)]
pub struct TouchWakeup {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is unclear to me here is if the touch pins are not configured to work in touch mode, whether wakeup would even work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

esp_sleep_enable_touchpad_wakeup() will fail if you run it without setting up touch, just as esp_sleep_enable_ulp_wakeup() will fail if ULP is not running (probably needs to be set up to actually wake up as well, but not sure)

So, since neither of these takes any arguments, just assumes that things are set up, I think that at least as a starting point this lib can do the same, i.e. just "blindly" apply and check return value.

Maybe this can be improved upon later, hopefully by somebody with abit better understanding and experience with touch/ulp. Maybe ill play around with that at some point, but not any time soon.

src/sleep.rs Outdated

impl<'a> GpioWakeupPin<'a> {
pub fn new(
pin: PinDriver<'a, AnyInputPin, Input>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

See the question below w.r.t. UartDriver. Also applicable here: should the pin be configured in GPIO mode, and as input, for the wakeup to happen?

In any case, passing AnyInputPin here is not ideal.

@oyvindnetland
Copy link
Contributor Author

Made a new commit fixing all the nits and resolved those threads. Commented on the other threads, but will start a new thread regarding pin/pindriver etc so we dont start discussing related things multiple places again

So, my thoughts on this is as follows (but im very open for changing my mind here):

For GPIO the input is configured before its passed to GpioWakeup, These settings will be active also for the wakeup source. So I think it makes sense to send a PinDriver which implies (i think) that it has already been configured. and that the sleep API only will add it to the list of gpios to check for during (light) sleep.

For RTC, the rtc_gpio_pull*() functions are used to set pull, and I assumed that this would not make sense to do before actually starting sleep, therefore I thought it would be logical to pass the actual pins together with pull settings, and then apply all of it before starting sleep. BUT, now I became unsure, since I see that set_pull() actually does rtc_gpio_pull*() already, which makes me think that maybe you could "pre-configure" the RTC and then pass a pindriver.

For UART I think the whole thing needs to be configured for it to work, so again i preferred to pass the driver. When looking at the light sleep example in esp-idf, then multiple uart things are set up. I dont think it makes senses to just send the pin here, I dont think that would work, but ill double check.

I have a few things to experiment with here, so ill get back in a few days I think, Will also check how passing references will work as well.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Sep 7, 2023

For GPIO the input is configured before its passed to GpioWakeup, These settings will be active also for the wakeup source. So I think it makes sense to send a PinDriver which implies (i think) that it has already been configured. and that the sleep API only will add it to the list of gpios to check for during (light) sleep.

I think you are probably right here. You must however, ensure that the passed pin driver instance has its pin configured in GPIO mode and not in RTC mode (if it is not clear, esp32 and all esp32s* chips have two separate APIs to deal with pins (and two separate hardware pieces for that) - the RTC API, and the GPIO API (which are roughly offering the same functionality, with the GPIO one supporting much more pins and being a bit more powerful).

I've just introduced two new marker traits, so that you can distinguish whether the passed PinDriver instance is configured in RTC or GPIO at compile time. You should roughly do pin: PinDriver<'d, M> where M: InputMode + GPIOMode.

You should also not pass the pin driver by value (as the user might want to re-use it post-light sleep, but either by mut reference, or by BorrowMut but let's keep this for later.

For RTC, the rtc_gpio_pull*() functions are used to set pull, and I assumed that this would not make sense to do before actually starting sleep, therefore I thought it would be logical to pass the actual pins together with pull settings, and then apply all of it before starting sleep. BUT, now I became unsure, since I see that set_pull() actually does rtc_gpio_pull*() already, which makes me think that maybe you could "pre-configure" the RTC and then pass a pindriver.

Hopefully the previous paragraph has cleared the confusion here. If you pass the pin driver to the GPIO wakeup source, you should also pass the pin driver (not the pin peripheral!) to the RTC wakeup source as well. You should assert that the driver is in RTC mode by using a similar newly-introduced trait: RTCMode.

For UART I think the whole thing needs to be configured for it to work, so again i preferred to pass the driver. When looking at the light sleep example in esp-idf, then multiple uart things are set up. I dont think it makes senses to just send the pin here, I dont think that would work, but ill double check.

OK. We are becoming symmetrical everywhere: for the UART, you should also pass the UartDriver instance, and not the UART peripheral itself.

I have a few things to experiment with here, so ill get back in a few days I think, Will also check how passing references will work as well.

Not sure what you mean by passing references. If you mean, passing drivers which might not be configured on degraded pins (AnyIOPin and friends) - yeah, you need to fix that and it won't be an easy code (it will be full of generics and tricks like the ADC chaining code I referred to in one of my earlier comments; if you get stuck, I can help here).
UPDATE: ^^^ It will actually be much easier, now that you pass PinDriver and not the actual Pin peripheral! The reason for this is - because the PinDriver instance does not contain anymore - in its type signature - the type of the pin peripheral it is operating on. So in a way, you don't care whether this was a degraded AnyIOPin or a normal pin peripheral - you don't have to specify that; you only need to assert what is the operating mode of the pin driver and that's all. (I simplified this several months ago by copy-pasting an idea from the embassy hal drivers.)

Finally - and in the long term - I think we should be consistent about this "pass the driver, not the peripheral" thing also in other cases; most notably - the touch wakeup source. It is just that... the touch driver (TouchDriver) is not implemented yet. So yeah, for the time being, you can either not support yet the touch wakeup source, or continue with your current implementation, which does not get one or more touch pins (touch drivers, actually).

I also need to look at the ULP wakeup source. It might be the case that there as well we need to pass-in an instance (or mut ref) to the UlpDriver, not even the ULP peripheral...

@oyvindnetland
Copy link
Contributor Author

Im struggling with RTC... Here are the general idea for wakeup structs for gpio and rtc:

pub struct GpioWakeup<'a, P, M>
where
    P: InputPin,
    M: InputMode + GPIOMode,
{
    pub pins: &'a [ &'a PinDriver<'a, P, M>],
}

pub struct RtcWakeup<'a, P, M>
where
    P: RTCPin + InputPin,
    M: InputMode + RTCMode,
{
    pub pins: &'a [ &'a PinDriver<'a, P, M>],
}

This is all nice, but when adding more than one pindriver to these struct I get the problem that each pindriver are different types since they have different P. So, for GPIO this is ok, because I can downgrade to AnyInputPin, but thats not an option for RTC.

Should there be a AnyRtcInputPin? Should AnyInputPin implement RTCPin? (i dont think it can?)

Seems like you are touching something related to this in your "UPDATE" in the last post, but Im not sure I understand it fully. PinDriver does contain the type of pin peripheral its working on, right?

Do we need the ChainedAdcChannels solution?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Sep 8, 2023

Im struggling with RTC... Here are the general idea for wakeup structs for gpio and rtc:

pub struct GpioWakeup<'a, P, M>
where
    P: InputPin,
    M: InputMode + GPIOMode,
{
    pub pins: &'a [ &'a PinDriver<'a, P, M>],
}

pub struct RtcWakeup<'a, P, M>
where
    P: RTCPin + InputPin,
    M: InputMode + RTCMode,
{
    pub pins: &'a [ &'a PinDriver<'a, P, M>],
}

This is all nice, but when adding more than one pindriver to these struct I get the problem that each pindriver are different types since they have different P.
So, for GPIO this is ok, because I can downgrade to AnyInputPin, but thats not an option for RTC.

Even for GPIO it is not OK, as the downgrade decision is not with you, but with the user. I.e. accepting only PinDrivers with downgraded AnyInputPin makes the API very restricted.

Should there be a AnyRtcInputPin? Should AnyInputPin implement RTCPin? (i dont think it can?)

Seems like you are touching something related to this in your "UPDATE" in the last post, but Im not sure I understand it fully. PinDriver does contain the type of pin peripheral its working on, right?

Indeed. I thought I had removed the P parameter from PinDriver, but unfortunately I have actually preserved it, so that the PinDriver::into_* methods can be implemented.

But regardless, because even if the P parameter was not there, you'll have the same problem with the M parameter, as restricting it to Input means you cannot take pin drivers, which are InputOutput, and you probably should.

Do we need the ChainedAdcChannels solution?

Yes. That's the reason I was pointing you to it - an array just won't do.

@oyvindnetland
Copy link
Contributor Author

oyvindnetland commented Sep 8, 2023

So, for GPIO this is ok, because I can downgrade to AnyInputPin, but thats not an option for RTC.

Even for GPIO it is not OK, as the downgrade decision is not with you, but with the user. I.e. accepting only PinDrivers with downgraded AnyInputPin makes the API very restricted.

yeah that makes sense

Do we need the ChainedAdcChannels solution?

Yes. That's the reason I was pointing you to it - an array just won't do.

seems like thats the only way to get everything we would want. I think it get the gist of it, but think im gonna need some time to struggle with the implementation :)

@oyvindnetland
Copy link
Contributor Author

wow, this stuff hurt my brain on several occasions, but ended up somewhere that works.

Probably still abit rough around the edges, so maybe not completely done, but think its getting there.

examples/light_sleep.rs Outdated Show resolved Hide resolved
examples/deep_sleep.rs Outdated Show resolved Hide resolved
examples/light_sleep.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

I think we are on the right path. It is just that I feel we don't need some of the structs and traits introduced here. Could be wrong and would be happy to be proven wrong!

examples/deep_sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
{
}

pub trait RtcWakeupPinTrait {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto. Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think i got rid of all you mentioned except this.

src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved

#[cfg(any(esp32, esp32s2, esp32s3))]
impl<P> RtcWakeupPins for P
where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Simplify to where P: PinDriver<...>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this fails since PinDriver is a struct not a trait, or did I misunderstand what you were suggesting?

src/sleep.rs Outdated Show resolved Hide resolved
src/sleep.rs Outdated Show resolved Hide resolved
}
}

#[cfg(any(esp32, esp32s2, esp32s3))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we can get rid of the RtcWakeupPinTrait I think we can get rid of this struct as well.

/// Will wake up the CPU based on changes in GPIO pins. Is only available for light sleep.
/// It takes PinDriver as input, which means that any required configurations on the pin
/// can be done before adding it to the GpioWakeup struct.
pub struct GpioWakeup<P>
Copy link
Collaborator

Choose a reason for hiding this comment

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

All suggestions for simplifications for the RTC subsystem apply here as well, as the approach is the same.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Oct 5, 2023

I'm sorry for the delay here. Trying to push a new release of the crate during the last month, and still not quite there.
Will return to this once the release is out.

@Vollbrecht
Copy link
Collaborator

how is the status, do we want to try to push it to completion here? i think overall it would be nice to have it done.

Copy link
Collaborator

@Vollbrecht Vollbrecht left a comment

Choose a reason for hiding this comment

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

After you applied the suggestions from ivmarkov, could you add comments on every pub facing ( trait, struct, function etc ) api object for a nicer generated docu with cargo doc. Though this can be done as a last step

@oyvindnetland
Copy link
Contributor Author

just added a missing piece, gpio wakeup from deep sleep on riscv devices.

@oyvindnetland
Copy link
Contributor Author

After you applied the suggestions from ivmarkov, could you add comments on every pub facing ( trait, struct, function etc ) api object for a nicer generated docu with cargo doc. Though this can be done as a last step

I think the outstanding comments from @ivmarkov are ideas for simplifications that didnt work out (or I wasnt able to make them work out).

Agree, ill add more comments on pubs when the rest of the stuff is ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants