charging: Apply thresholds after full just once. JB#58372 #13

martyone · 2022-07-14T14:30:47Z

No description provided.

monich · 2022-07-19T20:38:59Z

Regarding "more natural enumeration order" - of course it's more or less an arbitrary choice (unless it's a part of an ABI in which case the values must remain backward-compatible) but the original one looks more natural to me (non-zero if enabled, zero if disabled)

martyone · 2022-07-20T06:38:30Z

My reasoning is that the default behavior is that charging is enabled once the cable is connected and there are three options how to disable it - either fully or based on the current charge level. So enable is the default (zero), while disable, apply-thresholds or apply-thresholds-after-full are three levels of disabling it, i.e., overriding the default behavior.

monich · 2022-07-20T15:00:16Z

My reasoning is that the default behavior is that charging is enabled once the cable is connected and there are three options how to disable it - either fully or based on the current charge level. So enable is the default (zero), while disable, apply-thresholds or apply-thresholds-after-full are three levels of disabling it, i.e., overriding the default behavior.

I'm sure one can come up with a reasonable justification for pretty much any order 🙂

Some devices seem to never report the Full state despite reaching 100% charging level. Signed-off-by: Martin Kampas <martin.kampas@jolla.com>

Make the apply-thresholds-after-full option effective just once - change to apply-thresholds once fully charged. Do the same also when the charger gets disconnected, irrespective of the battery level. Clear battery full seen when changing the mode or it would require to reconnect the charger in order to set apply-thresholds-after-full after reaching the upper threshold. Signed-off-by: Martin Kampas <martin.kampas@jolla.com>

martyone · 2022-08-17T05:50:05Z

Fixed two issues:

Mode not changed back to apply-thresholds after fully charged
Not possible to activate apply-thresholds-after-full after reaching the upper threshold

martyone · 2022-08-17T05:54:31Z

The diff to look at is 9bd6235..aebe3a9

vigejolla

LGTM

spiiroin

Unnecessary new dbus methods should be removed [and any client side code should be changes to use mce settings dbus methods]

spiiroin · 2022-08-18T11:05:01Z

modules/battery-udev.c

@@ -1720,6 +1720,15 @@ udevdevice_evaluate_battery(udevdevice_t *self, mcebat_t *mcebat)
        mcebat->battery_status = BATTERY_STATUS_OK;
        if( self->udd_full && capacity >= BATTERY_CAPACITY_FULL )
            mcebat->battery_status = BATTERY_STATUS_FULL;
+        /* Some devices never report Full state */
+        else if ( capacity >= 100 ) {
+            mcebat->battery_status = BATTERY_STATUS_FULL;


This ought to be already taken care by "charging -> not-charging transition while capacity >= limit" heuristics few lines below... or do you mean that there is some device that gets stuck in "charging" state forever?

(Not getting battery full immediately after reaching 100% is more or less expected and change like this would trigger premature battery full signaling on well behaving devices).

Yes, my X10 II seems to stuck forever in Charging state. It was at 100% capacity for longer time and I never saw Full under sysfs. And since the code below is in an else if block, it never reaches it.

omg. yet another kind of potential weird behavior... As I have x10 ii as daily device and I do not recall seeing such thing: how was your charging setup? pc/wall charger? display on/off? live tracing over wlan? other sources of battery drain? i.e. anything abnormal that would/could affect final stages of charging.

It would be nice to not have weird scenarios affecting well behaving devices, but maybe this kind of kiss solution is okay in the context of this PR.

Originally tested with cable connected to laptop, later using a wall charger, both behaving equally WRT this bug. Both with display on and off, monitoring over wlan but also checking journal afterwards. It took a while to determine what actually happens there so I could exercise various options :)

Basically IMO: if you just connect a charger/pc, allow display to blank, leave the device to charge without doing anything special and it reaches battery full in somewhat sane proximity to 100% (led pattern / after the fact journal inspection) -> we should not add heuristics to second guess kernel reporting.

spiiroin · 2022-08-18T11:11:47Z

modules/charging.c

@@ -366,6 +394,13 @@ mch_policy_evaluate_charging_state(void)

    if( usb_cable_state == USB_CABLE_DISCONNECTED ) {
        switch( mch_charging_mode ) {
+        case CHARGING_MODE_APPLY_THRESHOLDS_AFTER_FULL:
+            if ( mch_charging_mode_evaluated_with_cable_connected ) {
+                mce_setting_set_int(MCE_SETTING_CHARGING_MODE,


This looks suspicious (from mce pov settings are read-only - apart from sanitizing changes made by client side)

yeah.. when you think of the use case more generally, it is like user scheduling an action by raising a flag and the flag needs to be dropped when it happens. We have two options: either introduce a completely new mechanism (from persistence to UI) or use settings for that. What would you choose?

Disconnecting should already clear battery-full-seen flag. If it does not, it is a bug.

What should happen when charger is connected, battery has been full and user toggles back and forth between apply and apply-after-full is somewhat murky and subjective. (iirc the original behavior was to apply-limits, triggering one time charge to full without disconnecting could be achieved by clearing battery-full-seen under suitable conditions when after-full is seen to get selected?)

In any case there is behavioral change here (overriding policy setting that I as an user have selected) and I do not get why such thing would be done / beneficial.

I really dislike that policy mode is changed here as it completely breaks existing functionality that I happen to use and rely on...

A live and let live solution could be: use separate boolean override for this new "charge once" behavior, detach settings ui from underlying reality a bit more than what it already is (say, by treating apply-thresholds-after-full as it were apply-thresholds) -> command line can still be used for tweaking by people not served by ui.

modules/charging.c

modules/charging.h

spiiroin · 2022-08-19T07:59:11Z

modules/charging.c

@@ -366,6 +394,13 @@ mch_policy_evaluate_charging_state(void)

    if( usb_cable_state == USB_CABLE_DISCONNECTED ) {
        switch( mch_charging_mode ) {
+        case CHARGING_MODE_APPLY_THRESHOLDS_AFTER_FULL:
+            if ( mch_charging_mode_evaluated_with_cable_connected ) {
+                mce_setting_set_int(MCE_SETTING_CHARGING_MODE,


Disconnecting should already clear battery-full-seen flag. If it does not, it is a bug.

What should happen when charger is connected, battery has been full and user toggles back and forth between apply and apply-after-full is somewhat murky and subjective. (iirc the original behavior was to apply-limits, triggering one time charge to full without disconnecting could be achieved by clearing battery-full-seen under suitable conditions when after-full is seen to get selected?)

In any case there is behavioral change here (overriding policy setting that I as an user have selected) and I do not get why such thing would be done / beneficial.

spiiroin · 2022-08-19T08:12:11Z

modules/battery-udev.c

@@ -1720,6 +1720,15 @@ udevdevice_evaluate_battery(udevdevice_t *self, mcebat_t *mcebat)
        mcebat->battery_status = BATTERY_STATUS_OK;
        if( self->udd_full && capacity >= BATTERY_CAPACITY_FULL )
            mcebat->battery_status = BATTERY_STATUS_FULL;
+        /* Some devices never report Full state */
+        else if ( capacity >= 100 ) {
+            mcebat->battery_status = BATTERY_STATUS_FULL;


omg. yet another kind of potential weird behavior... As I have x10 ii as daily device and I do not recall seeing such thing: how was your charging setup? pc/wall charger? display on/off? live tracing over wlan? other sources of battery drain? i.e. anything abnormal that would/could affect final stages of charging.

It would be nice to not have weird scenarios affecting well behaving devices, but maybe this kind of kiss solution is okay in the context of this PR.

Signed-off-by: Martin Kampas <martin.kampas@jolla.com>

martyone · 2022-11-07T07:00:47Z

Superseded by #16

martyone requested review from abranson and spiiroin July 14, 2022 14:30

martyone force-pushed the jb58372 branch from 40ed179 to 4bce097 Compare July 19, 2022 14:10

martyone changed the title ~~[charging] Apply thresholds after full just once. JB#58372~~ Expose charging hysteresis control on DBus. JB#58372 Jul 20, 2022

martyone force-pushed the jb58372 branch 2 times, most recently from abd8369 to 9bd6235 Compare July 20, 2022 12:39

vigejolla approved these changes Aug 11, 2022

View reviewed changes

martyone added 2 commits August 17, 2022 07:31

[battery-udev] Assume fully charged state at 100%. JB#58372

a5b3355

Some devices seem to never report the Full state despite reaching 100% charging level. Signed-off-by: Martin Kampas <martin.kampas@jolla.com>

martyone force-pushed the jb58372 branch from ae89907 to aebe3a9 Compare August 17, 2022 05:48

martyone requested a review from vigejolla August 17, 2022 05:50

vigejolla approved these changes Aug 18, 2022

View reviewed changes

spiiroin requested changes Aug 18, 2022

View reviewed changes

spiiroin reviewed Aug 19, 2022

View reviewed changes

martyone force-pushed the jb58372 branch from aebe3a9 to 52f655d Compare August 22, 2022 12:36

charging: Refactor setting charging limits

3c76c5a

Signed-off-by: Martin Kampas <martin.kampas@jolla.com>

martyone force-pushed the jb58372 branch from 52f655d to 3c76c5a Compare August 22, 2022 12:43

martyone requested a review from spiiroin September 15, 2022 10:33

martyone changed the title ~~Expose charging hysteresis control on DBus. JB#58372~~ charging: Apply thresholds after full just once. JB#58372 Sep 15, 2022

martyone closed this Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

charging: Apply thresholds after full just once. JB#58372 #13

charging: Apply thresholds after full just once. JB#58372 #13

martyone commented Jul 14, 2022

monich commented Jul 19, 2022

martyone commented Jul 20, 2022

monich commented Jul 20, 2022

martyone commented Aug 17, 2022

martyone commented Aug 17, 2022

vigejolla left a comment

spiiroin left a comment

spiiroin Aug 18, 2022

martyone Aug 18, 2022

spiiroin Aug 19, 2022

martyone Aug 22, 2022

spiiroin Oct 3, 2022

spiiroin Aug 18, 2022

martyone Aug 18, 2022

spiiroin Aug 19, 2022

spiiroin Oct 3, 2022

spiiroin Aug 19, 2022

spiiroin Aug 19, 2022

martyone commented Nov 7, 2022

charging: Apply thresholds after full just once. JB#58372 #13

charging: Apply thresholds after full just once. JB#58372 #13

Conversation

martyone commented Jul 14, 2022

monich commented Jul 19, 2022

martyone commented Jul 20, 2022

monich commented Jul 20, 2022

martyone commented Aug 17, 2022

martyone commented Aug 17, 2022

vigejolla left a comment

Choose a reason for hiding this comment

spiiroin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martyone commented Nov 7, 2022