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

groovy: Charge Thresholds #140

Open
wants to merge 7 commits into
base: master_groovy
Choose a base branch
from
Open

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Jan 19, 2021

This follows the threshold values and descriptions from #101 (comment), though the styling uses a ListBox to match other parts of Gnome Control Center (especially the Alt Chars / Compose Key dialogs).

Screenshot from 2021-01-19 10-15-52

This dialog is opened by pressing the thresholds row here, which shows up on System76 systems with the right firmware / software:

Screenshot from 2021-01-19 10-21-23

Changing the value opens a dialog through polkit, since administrator permission should be required for changing persistent settings:

Screenshot from 2021-01-19 10-16-15

Requires pop-os/system76-power#197 and firmware with thresholds support. If someone wants to test it without such firmware, I have code locally for system76-power to fake thresholds support.

(Note: commits before the one adding thresholds are already MR'd and merged upstream. Refactoring the code made it cleaner to add this, and just helped me understand the power panel code.)

Question

I don't know that it's been proposed in previous discussion, but do we want something like a toggle to disable thresholds? Users may not be happy with the fact that this includes no option to charge starting above 88%.

Also, currently the default in the firmware is 0:100, which disables charge thresholds. This is not any of these profiles, so it displays like this:

Screenshot from 2021-01-19 09-40-06

Whatever we end up deciding, the default option should be something exposed through the UI, whether it's disabled thresholds or one of these profiles.

(In any case, power users will be free to set any values they want through the system76-power CLI.)

State of firmware

Most open firmware systems do not have released firmware with charge thresholds, and the EC doesn't currently persist thresholds (system76/ec#128).

`set_primary()` and `add_primary()` had a lot of redundant code. This
unifies them, and moves them to a custom widget called `CcBatteryRow`.
This also decreases the somewhat excessive size of `cc-power-panel.c`,
and makes it easier to see the layout of widgets, now that it's
specified in xml.

Before this, `warning-battery-offset` was set to `0.03` for a "primary"
battery, and `0.05` otherwise. I expect this is a bug, so I've changed
both to `0.03`.

No other style or behavior change is intended.
This was added in 6c447dc to deal with
both older and newer UPower versions.

It should be safe now to assume a UPower version with this property.
It seems this was added in ee36b0d, but the `status` variable has
been unused since a968377 (committed in 2012).

Seems safe to remove if it's been unused since 2012.
This had another slightly different version of the same code.
@WatchMkr
Copy link

The thresholds and copy need updating to the below options. This removes the need for disabled and makes Balanced useful.

Can we detect if thresholds are supported and remove the option if they're not?


Increase the lifespan of your battery by setting a maximum charge value below 100%.

Full Charge (100%)
Battery is charged to its full capacity for the longest possible use on battery power. Charging resumes when the battery falls below 96% charge.

Balanced (90%)
Use this threshold when you unplug frequently but don't need the full battery capacity. Charging stops when the battery reaches 90% capacity and resumes when the battery falls below 85%.

Maximum Lifespan (60%)
Use this threshold if you rarely use the system on battery for extended periods. Charging stops when the battery reaches 60% capacity and resumes when the battery falls below 50%.

@ids1024
Copy link
Member Author

ids1024 commented Jan 19, 2021

The thresholds and copy need updating to the below options.

That should work. Then I suppose we update the EC so it defaults to the values listed here for "full charge"?

Can we detect if thresholds are supported and remove the option if they're not?

The thresholds row is hidden unless the system is running system76 open firmware, and the /sys/class/power_supply/BAT0/charge_control_start_threshold and /sys/class/power_supply/BAT0/charge_control_end_threshold paths exist, which should only exist if the firmware supports thresholds.

@ids1024 ids1024 force-pushed the gcc-thresholds_groovy branch from 6e86c1a to f558510 Compare January 19, 2021 18:52
@ids1024
Copy link
Member Author

ids1024 commented Jan 19, 2021

Updated:

Screenshot from 2021-01-19 10-52-11

@maria-komarova
Copy link

Looks good with the updated copy and styled as the ListBox.

We might want to adjust the copy and add the arrow at the end of the row to signify that clicking on the row would bring up the dialog. Similar to how it is done on the Keyboard panel for the Compose Key dialog. The copy might give a bit more context to users who are unfamiliar with setting battery thresholds.
image

In a rare case when there are two batteries how does it look like right now?

@WatchMkr
Copy link

@maria-komarova looks good but small change to remove double battery and the description matches the next page (lifespan rather than longevity). "Charge Threshold" "Adjust threshold to extend battery lifespan".

@ids1024 ids1024 force-pushed the gcc-thresholds_groovy branch from f558510 to c83beb8 Compare January 19, 2021 20:01
@ids1024
Copy link
Member Author

ids1024 commented Jan 19, 2021

Great idea, and fairly easy to do. I was just thinking it could be clearer that the row is clickable and what it does:

Screenshot from 2021-01-19 12-01-07

@bflanagin
Copy link

Undefined

Manually setting the charge-threshold outside of the suggested ranges found in the UI causes the UI report back "Undefined."

@WatchMkr
Copy link

Manually setting the charge-threshold outside of the suggested ranges found in the UI causes the UI report back "Undefined."

Ah, I missed that connection from Ian’s comment. How about changing this to “Set outside predefined thresholds.”? It’s a mouthful but will accurately describe what happened. The user should understand given that they modified the threshold via the /sys/class interface.

@ids1024
Copy link
Member Author

ids1024 commented Jan 20, 2021

It could show something like Custom (60%-70%).

It's inevitably kind of weird determining how the UI should deal with values it doesn't allow setting. But hopefully that's not too much of a problem because users who have changed it through the command line will know they've done so.

@WatchMkr
Copy link

Great idea. Custom with percentages is much better. It’s likely to be uncommon with the UI introduction anyway.

@ids1024 ids1024 force-pushed the gcc-thresholds_groovy branch from c83beb8 to 514eba9 Compare January 20, 2021 16:22
@ids1024
Copy link
Member Author

ids1024 commented Jan 20, 2021

Updated, as suggested. When the profile is "custom", opening the dialog results in no radio button being selected, and choosing one changes the values as usual (it already worked that way, I just haven't mentioned that part):

Screenshot from 2021-01-20 08-21-58

@maria-komarova
Copy link

Looking at it this way makes me wonder if it is necessary to clarify in a one-sentence copy where the Custom (60%-70%) comes from. But on the other hand, it seems more weird to be pointing users to set any values they want through the system76-power CLI in the UI here. I guess if Custom appears there it would most likely mean someone already knows where this is coming from.

@ids1024
Copy link
Member Author

ids1024 commented Jan 20, 2021

Yeah, I was wondering if there's a way to clarify that, without just making things more complicated. Since generally someone should know they set thresholds by some other means.

It's probably not necessary, but if we want something like that, perhaps the dialog could have some additional text in bold that appears only when "custom" thresholds are set.

@maria-komarova
Copy link

That might be a solution. Although I can't think of a suitable copy that won't be confusing. I'll keep thinking. We can probably always add that copy later if we think of a good option.

@digitalcircuit
Copy link

What about…

Custom battery charge threshold (60%–70%) set via command line. Learn more

Where Learn more links to an updated battery charge threshold section (e.g. <URL>/#section-id) on the System76 documentation page on battery life? Or some form of local explanation in the system help viewer application if having this available offline is important (e.g. perhaps someone is wanting to change this during an airplane flight or while out camping).

Repeating the exact percentages here (e.g. 60%–70%) makes it easier to switch to a provided profile that's closest to whatever custom configuration was set.

Alternatively, something I might prefer…

Increase the lifespan of your battery by setting a maximum charge value below 100%. Learn more

Custom battery charge threshold (60%–70%) set via command line.

Thus the Learn more link is always visible, providing an unobtrusive way for someone to read more about lithium ion chemistry and how to fully customize this without overwhelming someone with words up front.

The "custom" message would be hidden whenever the charge start/end matches an available profile. It's possible someone didn't set this via the command line (e.g. if charging profiles change in a Pop!_OS update, or if someone confugured this in KDE Plasma whenever support is added there), but it may be okay to assume it was changed via command line.

(I'd wonder if the System76 coreboot UI should provide a way to reset this to default as well. That'd be a separate issue regardless.)

@ids1024 ids1024 force-pushed the gcc-thresholds_groovy branch 2 times, most recently from 1c38743 to 8bce643 Compare January 22, 2021 20:54
@ids1024 ids1024 force-pushed the gcc-thresholds_groovy branch from 8bce643 to c9f51cc Compare January 22, 2021 21:09
@maria-komarova
Copy link

After more thinking, we can probably add "Custom battery charge threshold (60%–70%) set via command line. Selecting one of the profiles below will override custom setting." in bold or italic (if we use it anywhere) to the dialog for the cases when threshold was changed via command line. I still think it somewhat complicates the design and introduces some sort of awkward intersection. But that is probably still better to say something when none of the options is selected in the dialog while threshold seems to be set. More as a reminder.

I am hesitant about Learn more. This is not something we do anywhere else at this point. And it might potentially be an additional maintenance point. This will probably have to wait for now. At least until we have an article on how to customize thresholds on the website.

@ids1024
Copy link
Member Author

ids1024 commented Jan 27, 2021

Added a commit with a label like that:

Screenshot from 2021-01-27 13-02-15

Copy link

@digitalcircuit digitalcircuit left a comment

Choose a reason for hiding this comment

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

The custom profile notice looks great, and overall I am excited for this effort to make long-term battery longevity easily configured.

In addition to the minor tweak to the custom profile message, the default start/end charge thresholds for the embedded controller side may want to be changed so a new Pop!_OS setup won't trigger the custom profile message. I wasn't able to find an issue or pull request in the other repository for this.

panels/power/cc-charge-threshold-dialog.c Outdated Show resolved Hide resolved
@ids1024
Copy link
Member Author

ids1024 commented Feb 1, 2021

In addition to the minor tweak to the custom profile message, the default start/end charge thresholds for the embedded controller side may want to be changed so a new Pop!_OS setup won't trigger the custom profile message.

Yep, it shouldn't be triggered by the default values. This will probably need to be sorted out in some way before this PR can merge.

I wasn't able to find an issue or pull request in the other repository for this.

We've discussed this internally, but there wasn't an issue for it. It seems good to have an issue though: system76/ec#143

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.

5 participants