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

Immediately go to sleep if battery is critical #274

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

SZenglein
Copy link
Contributor

Explanation is given in the code.

@SZenglein SZenglein force-pushed the battery_critical_immediate_shutdown branch from 29def2c to d360af9 Compare December 4, 2023 21:30
@laszloh
Copy link
Contributor

laszloh commented Dec 5, 2023

You should take a look at System_DeepSleepManager to see what actions are taken before entering the deep sleep.

What I see is, that you do not turn of any peripheral units, so both the AMP and the power system could still be active . I'd at least propose to call the two functions System_PreparePowerDown and Power_PeripheralOff (this one is the more important one) to bring the two GPIO pins to the required level, so the peripherals are unpowered.

@SZenglein
Copy link
Contributor Author

SZenglein commented Dec 12, 2023

System_PreparePowerDown is a good pointer.

The thing is, Battery_Init is intentionally placed very early in the setup function, meaning the peripheral power has not yet been turned on. Also Audioplayer has not been initialized, so it should not be exited either.

It has its quirks and order of operations in setup is a difficult topic, but I did design the battery module, the power module and influenced the setup order with all that in mind, and as far as I can see it has not changed.

Essentially, it's treated in the same way as esp_deep_sleep_start in Pn5180 when a false LPCD wakeup ha been detected.

Also, I think you might have missed this because I only wrote it in a private message to @tueddy, but this PR is essentially just reverting this hidden change: 63dc695#diff-f983eb1c0981492d0daafbef571290cd784c5b9cec1526ea3f344f7729e81232L31

I have added a Power_PeripheralOff just because the state of the power pin is kinda undefined at this point and to be sure it is off.

@laszloh
Copy link
Contributor

laszloh commented Dec 13, 2023

Yes, I did not knew the history of this change. In the light of it, it looks good.

@tueddy tueddy merged commit 461b62b into biologist79:dev Dec 13, 2023
10 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants