Revert low battery alert. Add low battery indicator.

[Riksu9000]

This reverts PR #1352

There's no need to break the focus of the user to alert them that the battery will be empty in 24h, when they can see that the next time they check the watch. I've made the battery icon turn orange at 15, and red at 5, for a stronger warning to charge the watch.

[minacode]

Obviously I don't like the reversion. But I like the coloring of the icon.

Low battery is one the few things every device alerts about. Companion apps implemented this feature before I did for the watch, so I'm sure some people wanted it. I do. I want to be actively notified and not having to look often enough by myself.

The loss of focus would be approximately once per week and only if you don't charge often enough.

Regarding the 24h, we could just change the threshold to a lower value. But people may not carry their charging device for the watch with them and are only able to charge once per day (e.g. during the night). We could also add a second "critical" threshold like you did to warn a second time.

[Riksu9000]

It's a bit different when my laptop warns me that there's 20 minutes remaining. In this case this isn't something that needs immediate attention, so there's no reason to grab attention immediately. This color change makes it so you don't have to try and count the number of pixels, but you know exactly when battery is below 15%, just like with the notification. I'm assuming you'll check the watch often enough if you're wearing it.

Also people most likely cannot charge the watch immediately when the notification comes, in which case they must ignore the alert and possibly forget about it. The notification is saved, but there's nothing to remind about that either.

[minacode]

What about a notification at 5% or maybe even 1%. I would at least take it as a "be careful now and maybe don't use the flashlight anymore". We can go more for the 20-minutes scenario with your visual clues present.

Also, we should keep the low battery functionality besides the notification, because we could do more things with it like lowering the screen brightness or turning off heart rate measurement for example.

It's a bit sad to work on a PR for a time and have a discussion and then get it completely "reverted" by a maintainer a couple of days after it got merged. I know that I have no rights for my code to be included, but it doesn't feel good. You even changed a clean function call to a random magic number in BatteryInfo.cpp line 62.

[Riksu9000]

I would at least take it as a "be careful now and maybe don't use the flashlight anymore"

You would still see the battery icon.

I used tools to revert the commits of the PR. I made no changes myself.

I think the alert was mainly inspired by the poor percentage calculation that was present at the time, which caused the watch to power off unexpectedly. This however shouldn't be an issue anymore with the new approximation.

The watch will keep working even after reaching 0%. What I've had in mind is to enter a very low power mode after reaching 0%.

I don't think all options were thoroughly considered before merging the PR and I'm sorry about how this happened. Let's discuss. @InfiniTimeOrg/core-developers.

[minacode]

What I've had in mind is to enter a very low power mode after reaching 0%.

Maybe this is something we should notify? Especially if the watch turns off features that people rely on (I don't know what you have in mind).

I don't think all options were thoroughly considered before merging the PR and I'm sorry about how this happened. Let's discuss.

It's in my interest to do what is best for the software, so if your approach is better, we should use it.

[Boteium]

Duplicate of #1276 ?
Personally, I want both. But I wonder why #1276 isn't merged.
I thought #1352 is a replacement of #1276.

[Riksu9000]

It seems to me like #1276 was simply made in protest of #1265. The thought process and design here is different, so not exactly a duplicate.

[JF002]

It's a bit sad to work on a PR for a time and have a discussion and then get it completely "reverted" by a maintainer a couple of days after it got merged.

I'm the one responsible for this, and I'm really sorry about this. I merged the code because it looked good and did what a few users were asking for.

However, we talked about the feature in more details later on the with @InfiniTimeOrg/core-developers team and a few points were raised:

• The warning will be issued once, probably at a time when the user do not have the ability to charge their watch, and they'll probably forget about it later on.
• The warning will be issued at any time. As a (very) light sleeper, I would be very frustrated to be awaken by my watch saying that it'll be out of battery in a few hours.
• The need for the low battery notification probably comes from the fact that the battery level estimation was really bad until recently.

Since the it can be missed (in case notifications are disabled and/or in sleep mode) and forgotten, the notification does not seem to be the best way to make the user know that they should charge the battery. The color scheme on the battery indicator have the advantage that it is not intrusive but will probably directly catch the eye of the user as soon as they'll have a look at their watch.

Regarding the implementation : Since we are only setting the color or a UI component, it makes sense that the UI is responsible to specify the threshold and the color (as opposed to the BatteryController). It could probably be refactored to avoid code duplication, but it should still stay on the UI side.
If we were to implement a special behavior in case of (very) low battery level, we could reuse the implementation from the original PR (BatteryController detects the low level, sends a message to SystemTask and SystemTask takes the appropriate action).

[minacode]

Ok, thank you. A few days passed and my initial feeling is gone. It seems to make sense to do it like this PR and that is ok.

[lman0]

some people are not able to see the red

in order to make all people feel the urgency
you could make the battery icon blink too (in the red state )

(to show it can shut down anytime)

that would be a fine addition @Riksu9000

[Riksu9000]

They can still see the icon. I don't think making it blink is necessary.

[Riksu9000]

I tried to move the colorization feature into BatteryIcon so the code wouldn't need to be duplicated, however it ended up using a bit more memory. Let me know if I should move it regardless.

Here's a patch with the change.
deduplication.patch.txt

[JF002]

I tried to move the colorization feature into BatteryIcon so the code wouldn't need to be duplicated, however it ended up using a bit more memory. Let me know if I should move it regardless.

According to my test, the patch only adds 32 bytes. I do not really understand how removing duplicated code ends up in bigger binary (I guess that's the magic of the optimizer), but since the additional memory usage is very small, I think we could apply this patch as it actually removes code duplication :)

[github-actions]

Build size and comparison to develop:

Section	Size	Difference
text	414708B	-84B
data	940B	0B
bss	53544B	0B