nRF52840 Power Management Stage 1 v2.1 - Boot lockout fix

[entr0p1]

Improvements:

• Add configurable battery chemistry
• Add per-chemistry LPCOMP wake threshold and boot lockout voltage
• Add configurable enabled/disabled state for boot lockout
• Reduce Li-ion/Li-Po lockout threshold to 3.0V from 3.3V
• Initialise FS earlier in the startup process to allow reading configured settings for boot lock

Fixes:

• Add additional shutdown reason "None" for instances where the reason isn't set
• Boot lockout disabled by default

[MGJ520]

Good job

[entr0p1]

Tested on a couple of boards now without issues so far. Ready to be reviewed.

[Pacjunk]

Does setting pwrmgt.bootlock to off also prevent the low voltage shutdown? or is it only for boot?

[recrof]

I truly understand what @entr0p1 wanted to do here and I respect it, however: I feel this became tad too over-engineered and we need to regroup and make it really simple:
ability to configure shutdown voltage and make it 0V as default. adding chemistries raises complexity and resources - we need to get rid of that. make the feature opt-in and not opt-out is also important.

[entr0p1]

@recrof appreciate the respectful approach you've taken, I'm not fussed if it has to be pared right back or removed entirely. The fix is here if desired, but if you'd rather a different approach I won't be offended if you want to close it and do that.

[fschrempf]

I truly understand what @entr0p1 wanted to do here and I respect it, however: I feel this became tad too over-engineered and we need to regroup and make it really simple:
ability to configure shutdown voltage and make it 0V as default. adding chemistries raises complexity and resources - we need to get rid of that.

100% agree.

make the feature opt-in and not opt-out is also important.

This is the most crucial point. #1413 was not ready for merging and still got merged for v1.12. It broke a lot of setups and now three months and three releases later things are still broken. People need to modify their firmware manually and climb on roofs because of this.

Can we please either:

1. leave the current code as is, but add a configuration option that enables/disables the undervoltage bootlock and have this option disabled by default or
2. just disable NRF52_POWER_MANAGEMENT for all boards.

Thanks!

[ssozonoff]

When the final decision has been made and code merged and released it will be important to communicate about this in any case. I personally believe we will be seeing more and more LTO and Sodium-ion based repeaters in the future.

[fschrempf]

just disable NRF52_POWER_MANAGEMENT for all boards.

I've implemented this as a quick fix in #2377. After deciding on and implementing a proper solution this can be reenabled.

[entr0p1]

I've been thinking about this more and based on what @recrof said I remembered we already have an auto shutdown voltage in the companion firmware (I think its in others now too?). Couldn't we basically drop this entire feature out of the codebase and just build on that to make the voltage configurable via the CLI?

No chemistries, wake, or lockouts to worry about and the code basically exists and just needs a pref added.

Thoughts?

[1nerdherder]

My thoughts after being on the 2088 adventure:
In a world where people are getting busier and reading less, I agree simpler is better.
The majority of users will not be concerned with this feature because they are using standard boards, generally with the one chemistry allowed. If they do go "custom" with an external solar battery charge controller, or some other adaptation, it will be too difficult for the firmware to know all the permutations (it would be a rather large table chasing innovation). So, those of us who are using advanced configurations, should be knowledgeable enough to change the defaults.
Simplifying to one parameter (the low voltage shutdown level - using zero to disable it completely) is a good start.
The only downside I can think of is that we lose the ability to tell the iOS/Android app what battery type it is so it can calculate percentage. However I have full confidence that someone smart will figure that one out at some point.

[TheSlowGrowth]

Regarding the app percentage display: The app could configure the battery chemistry for pecentage display purposes on the app level instead of polling it from the device. It's then saved locally in the app preferences. That way the device stays clean of any unnecessary settings; boot lock voltage can be configured freely by expert users and we can still have a sensible percentage display in the app.

[luigi311]

looking at this it does look like this is trying to do a lot at once. Instead of trying to add battery chemistry on here i would instead look at building this on top of my battery chemistry implementation at #1176.

You could just do an ifdef for the different cell types and use that to set the minimum voltage either hard coded or perhaps even just fetch the last value in OCV_ARRAY as that should line up with 0% to the devices configured chemistry. In most cases though for Liion it will set it to 3.1v though instead of your proposed 3.0v if you take the OCV route.

[entr0p1]

I understand the apprehension here, but I really want to get this one off my plate once and for all and put an end to the frustration and complaints. So we really need to make a call on what we want here.

If I distil down the various conversations on what people want, I can think of these options:

1. Drop it entirely
2. Drop the chemistry selection, leave two configurables: lockout voltage (which we probably shouls make the same setting as runtime shutdown voltage to keep simple), and wake REF for the voltage divider.
3. Keep this version that I've posted which sows the seeds for a chemistry setting so that the app can use it later (this was the intent in addition to people asking for it to be configurable)

Its already opt-in (disabled by default) in this PR so that's implied.

What do we think?

[LennertVA]

Yes, this really should be pushed out the door, especially since not having it can (and has) cause(d) serious operational problems.

1 is not an option. 2 sounds good, will keep it simple and solves the core of the problem. 3 sounds equally good to me - but I'm in no position to judge whether it's a base which will have future use, or falls under YAGNI.

Regardless of whether it's 2 or 3, please get this merged 🙏

[luigi311]

i think 2 makes the most sense to get this out quickly. It massively reduces the code change and if someone else wants to use this with a different chemistry they can just do a build with the voltage overriden to match their chemistry themselves until there is a better way merged in for battery chemistry management.

[1nerdherder]

Here is my $0.02
There has been a lot of good work done here - that merits inclusion into the code.
If we think about the situations that require this feature - they will be those who mostly know what they are doing, using less common boards with their own particular power configuration issues, or bypassing existing charging hardware with external power management boards - for which we can never accomodate all the permutations; therefore an approach like #3 will need more thought to consider how to handle all of those permutations. What I do like about #3 is that there is a declaration of the battery type that can be queried and acted upon when calculating charge % - we lose this in option 2 (meaning the iOS and Android apps will always report the incorrect battery % if not LiPo).
So, as others have said, If we go with #2, it is quick, flexible - but really should be accompanied by table of the two suggested settings for particular chemistries in the user documentation (particularly for LiFePO4). This should NOT require a custom firmware build and would release with the standard LiPo settings.
If we go with #3, people need to understand it is a first iteration and be invited to innovate toward something that equally simple to use. The "opt-in" flag that was mentioned, allows people to try it and provide valuable feedback or additional code or just leave it off (default).
Make sense?