Created attachment 138070 [details] [review] patch to fix the above bug On linux-next (commit has been reversed due to this bug, use linux-next 20180223), a new state for batteries has been introduced in the ACPI battery driver for Lenovo ThinkPad laptops as "Not Charging". This state occurs when the one of the batteries has been charged up to a certain threshold and stops charging. UPower reports that correctly as pending-charge for the real battery, but the fake DisplayDevice has a state of "unknwon" thus breaking the icon and state in almost all desktop environments. The below patch fixes this issue and has more information available.
Is anyone bloody reading these bug reports?
(In reply to Ognjen Galic from comment #1) > Is anyone bloody reading these bug reports? Don't forget to ask for your money back.
Comment on attachment 138070 [details] [review] patch to fix the above bug Review of attachment 138070 [details] [review]: ----------------------------------------------------------------- We don't use signed-off-by in user-space. ::: src/up-daemon.c @@ +160,4 @@ > gint64 time_to_empty_total = 0; > gint64 time_to_full_total = 0; > gboolean is_present_total = FALSE; > + guint num_pending = 0; This all looks complicated when the effect of the patch would be to ignore the state of the "pending-charge" battery. Could you please add a test case in src/linux/integration-test? This will make it easier to test either this patch, or alternative patches for this bug.
>> Is anyone bloody reading these bug reports? > Don't forget to ask for your money back. Sorry for the attitude. I though this project was abandoned after 2 weeks of email silence from the maintainer. I have attached a patch for the integration test on Linux. Without my fix, the tests fail with an expected error, that is the battery displaydevice state us unknown. > ====================================================================== > FAIL: test_battery_thinkpad (__main__.Tests) > Test a single battery on a Lenovo ThinkPad system > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "./src/linux/integration-test", line 463, in test_battery_thinkpad > self.assertNotEqual(self.get_dbus_display_property("State"), UP_DEVICE_STATE_UNKNOWN) > AssertionError: 0 == 0 > > ====================================================================== > FAIL: test_multiple_batteries_thinkpad (__main__.Tests) > Test multiple batteries on a Lenovo ThinkPad system > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "./src/linux/integration-test", line 505, in test_multiple_batteries_thinkpad > self.assertNotEqual(self.get_dbus_display_property("State"), UP_DEVICE_STATE_UNKNOWN) > AssertionError: 0 == 0 > > ---------------------------------------------------------------------- > Ran 33 tests in 54.234s > This is the state I observe in GNOME 3 and KDE. With my fix, the tests complete without issues, as expected. > Testing binaries from local build tree > ................................. > ---------------------------------------------------------------------- > Ran 33 tests in 55.252s > > OK There is a set of rules for the DisplayDevice, and they are defined in the test patch. Thanks for the time!
Created attachment 138336 [details] [review] v2-0001-test-add-test-cases-for-Lenovo-ThinkPad-systems-o.patch
It's been a week and no updates. What is the state of this bug/patch?
Hi Ognjen, I'm responding here because you send me a direct email asking to take a look. First of all please watch your language, language such as "Is anyone bloody reading these bug reports" is imply not acceptable. Please always be polite and professional in your communications. Also please be patient sending such a comment after only 4 days have passed since filing the bug is inexcusable. All freedesktop run projects are community projects and a lot of us do this (partly) in our spare time. With that said, lets take a look at the actual problem, you are the patch of this kernel commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=91eea70e5e5ce12eb1c7cd922e561fab43e201bd Which later got reverted, this commit makes the power_supply class report POWER_SUPPLY_STATUS_NOT_CHARGING instead of UNKNOWN when the ACPI battery state field has neither the ACPI_BATTERY_STATE_CHARGING and ACPI_BATTERY_STATE_DISCHARGING bits set. So we are back to the kernel reporting UNKNOWN in this case. Note that Daniel Drake wrote in the revert series: "Revert this quirk before it gets included in a release, while we look for better approaches." This was about another (somewhat related) kernel patch, but we are still figuring things out on the kernel side, so making userspace change now does not seem like a good idea. But lets assume the kernel state stays as is, then I wonder what are you trying to fix? upower already has UNKNOWN state handling, see: src/linux/up-device-supply.c starting aorund line 690: /* the battery isn't charging or discharging, it's just * sitting there half full doing nothing: try to guess a state */ if (state == UP_DEVICE_STATE_UNKNOWN && supply->priv->is_power_supply) { ... } My Asus T200TA also reports UNKNOWN (rather then being fully-charged) when unplugged after being fully charged and then replugged before the charge drops below the threshold where it will do another charge cycle and there are likely many other devices doing this. On my Asus T200TA the unknown state handling simply just reports FULL in this case, granted it might be only 91% full, but to me that seems better then reporting "not charging" as your kernel patch was doing, not charging will actually get translated to the charging icon for the desktop environment, which is misleading in this case. Full actually is more accurate because the laptop considers itself full enough to no bother charging anymore. ### Or in other words, can you please explain step by step, what you are doing and what is the problem you are seeing, I guess you start by fully charging the laptop, then unplug and after loosing say 5% you replug? Do I have that right? Then after the replug was does your desktop environment show and what would you expect it to show? Regards, Hans
"you are the patch of this kernel commit" should be "you are the author of this kernel commit"
(In reply to Hans de Goede from comment #7) > Hi Ognjen, > > I'm responding here because you send me a direct email asking to take a look. > > First of all please watch your language, language such as "Is anyone bloody > reading these bug reports" is imply not acceptable. Please always be polite > and professional in your communications. I'm really, really sorry. A simple feature kernel patch that should have taken two weeks has spread out to half a year of emails, reports and waiting, and It's starting to get to me badly. > > Also please be patient sending such a comment after only 4 days have passed > since filing the bug is inexcusable. All freedesktop run projects are > community projects and a lot of us do this (partly) in our spare time. I do realize that they are community driven, but I think if I did not react in that way this report would have gotten stale like the other ~60 bug reports without comments/fixes. > > With that said, lets take a look at the actual problem, you are the patch of > this kernel commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ > ?id=91eea70e5e5ce12eb1c7cd922e561fab43e201bd > > Which later got reverted, this commit makes the power_supply class report > POWER_SUPPLY_STATUS_NOT_CHARGING instead of UNKNOWN when the ACPI battery > state field has neither the ACPI_BATTERY_STATE_CHARGING and > ACPI_BATTERY_STATE_DISCHARGING bits set. > This is the desired behavior for Lenovo ThinkPad laptops as documented by the ACPI specification from Lenovo. The laptop shall report "Not Charging" when the battery is between the start and stop charge thresholds, a feature on those Lenovo ThinkPads. Windows NT reports it as "Not Charging". > So we are back to the kernel reporting UNKNOWN in this case. Note that > Daniel Drake wrote in the revert series: Incorrect. When the kernel reports UNKNOWN upower handles that fine. What I'm trying to do is to prepare upower for that reverted kernel patch to be applied again, as we did not want to break the userspace with a new battery state. > > "Revert this quirk before it gets included in a release, while we look for > better approaches." > > This was about another (somewhat related) kernel patch, but we are still > figuring things out on the kernel side, so making userspace change now does > not seem like a good idea. > > But lets assume the kernel state stays as is, then I wonder what are you > trying to fix? upower already has UNKNOWN state handling, see: > > src/linux/up-device-supply.c starting aorund line 690: > > /* the battery isn't charging or discharging, it's just > * sitting there half full doing nothing: try to guess a state */ > if (state == UP_DEVICE_STATE_UNKNOWN && > supply->priv->is_power_supply) { > ... > } This is also correct. UPower handles the kernel state of UNKNOWN just fine. When the state is unknown from the kernel UPower guesses a battery state based on the AC status, in my case it guesses "Charging". > > My Asus T200TA also reports UNKNOWN (rather then being fully-charged) when > unplugged after being fully charged and then replugged before the charge > drops below the threshold where it will do another charge cycle and there > are likely > many other devices doing this. > > On my Asus T200TA the unknown state handling simply just reports FULL in > this case, granted it might be only 91% full, but to me that seems better > then reporting "not charging" as your kernel patch was doing, not charging > will > actually get translated to the charging icon for the desktop environment, > which is misleading in this case. Full actually is more accurate because the > laptop considers itself full enough to no bother charging anymore. > > ### > > Or in other words, can you please explain step by step, what you are doing > and what is the problem you are seeing, I guess you start by fully charging > the laptop, then unplug and after loosing say 5% you replug? Do I have that > right? Here's the story: Lenovo ThinkPads have a feature inside their battery firmware that can control when the battery stars and stops charging. This is designed to stop the cells from wearing out while using the laptop on the charger. Linux did not support this feature until I wrote a patch series back in November with the help of some Lenovo engineers. Here is that series: https://patchwork.kernel.org/patch/10205339/ Now, that series defines a NEW state for the ACPI battery driver, which is "Not Charging". That is here: https://patchwork.kernel.org/patch/10205359/ The problem was that a state of Not Charging was completely new for UPower and the userspace, which broke the battery indicator on some desktops like MATE and GNOME. See: https://lkml.org/lkml/2018/3/12/843 https://lkml.org/lkml/2018/3/12/957 When a battery was in a "Not Charging" state, UPower's DisplayDevice did now know how to handle that, and it reported a default state of "UNKNOWN" to the higher userspace. This resulted in a "battery is broken/not available" icon in desktops. Because of this we reverted that commit to not break desktops. This behavior is reflected by my test cases I wrote for UPower's integration-test: > ====================================================================== > FAIL: test_battery_thinkpad (__main__.Tests) > Test a single battery on a Lenovo ThinkPad system > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "./src/linux/integration-test", line 463, in test_battery_thinkpad > self.assertNotEqual(self.get_dbus_display_property("State"), UP_DEVICE_STATE_UNKNOWN) > AssertionError: 0 == 0 > > ====================================================================== > FAIL: test_multiple_batteries_thinkpad (__main__.Tests) > Test multiple batteries on a Lenovo ThinkPad system > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "./src/linux/integration-test", line 505, in test_multiple_batteries_thinkpad > self.assertNotEqual(self.get_dbus_display_property("State"), UP_DEVICE_STATE_UNKNOWN) > AssertionError: 0 == 0 > > ---------------------------------------------------------------------- > Ran 33 tests in 54.234s > UP_DEVICE_STATE_UNKNOWN occurs only when a battery is in UP_DEVICE_STATE_PENDING_CHARGE, aka. Not Charging. Now, we want to apply the reverted kernel patch again to restore full functionality, as reporting "Unknwon" is not desired. In order to do this we need to patch UPower to support the new "Not Charging" state of the batteries. The patch I submitted does this by reporting "Fully Charged" in order to not break desktops when a battery is "Not Charging". It also handles multiple batteries properly. See the test cases for more details. This patch is a prepare-patch, and if/when this gets merged to upower I will re-send the kernel patch. The end-result of this is a proper state of "Not Charging" and proper reporting in desktop environments. > > Then after the replug was does your desktop environment show and what would > you expect it to show? > > Regards, > > Hans Thanks Hans.
Hi, Thank you for your explanation. So your kernel patch causes 1 or more batteries to report UP_DEVICE_STATE_PENDING_DISCHARGE which gets translated to UP_DEVICE_STATE_UNKNOWN for the virtual combined DisplayDevice up-device and that is what you are trying to fix on the upower side, correct? With that in mind your patch looks ok, one remark: @@ -235,6 +238,22 @@ up_daemon_update_display_battery (UpDaemon *daemon) num_batteries++; } + /* The display battery has an unknown state (Usually ThinkPads on Linux 4.17 with + * the battery charge threshold states set). */ + if (num_batteries > 0 && state_total == UP_DEVICE_STATE_UNKNOWN) { + /* If all batteries are pending a charge, return the total state + * as "fully charging". We return that all are fully charged because + * if a battery is pending a charge, it has been fully charged up to + * its threshold and is in idle. We could return "pending charge" here + * but that is too complicated for the display battery */ + if (num_pending == num_batteries) + state_total = UP_DEVICE_STATE_FULLY_CHARGED; + /* Else if there is one battery that is pending but one battery is not, + * we will return "charging" as the other is charging. */ + else if (num_pending > 0) + state_total = UP_DEVICE_STATE_CHARGING; If one of the batteries is charging the: if (state == UP_DEVICE_STATE_CHARGING) state_total = UP_DEVICE_STATE_CHARGING; Will have already triggered and we never get here, I think this whole bit can be simplified to: if (num_batteries && num_pending == num_batteries && state_total == UP_DEVICE_STATE_UNKNOWN) state_total = UP_DEVICE_STATE_FULLY_CHARGED; With that changed I think that this is a sensible fix for upower. I don't think simply re-introducing your kernel patch is a good idea though, mostly because of it being dmi-quirk based which means we will need a ton of quirks if we're not careful. Since you've written the threshold patches I would like to suggest to modify drivers/acpi/battery.c to hook into the code which has set the new start-threshold and make acpi_battery_is_charged() use the charge_threshold rather then full/design capacity on devices with a charge-threshold, then the kernel will report POWER_SUPPLY_STATUS_FULL when neither ACPI_BATTERY_STATE_DISCHARGING and ACPI_BATTERY_STATE_CHARGING are set, which will even make older upower's do the right thing. Generally speaking making kernel changes which cause regressions for older userspace versions is not allowed. So even with the upower changes you would need to wait at least a year before the old version of your kernel patch can be merged. Regards, Hans
(In reply to Hans de Goede from comment #10) > Hi, > > Thank you for your explanation. > > So your kernel patch causes 1 or more batteries to report > UP_DEVICE_STATE_PENDING_DISCHARGE which gets translated to > UP_DEVICE_STATE_UNKNOWN for the virtual combined DisplayDevice up-device and > that is what you are trying to fix on the upower side, correct? That's correct. > > With that in mind your patch looks ok, one remark: > > @@ -235,6 +238,22 @@ up_daemon_update_display_battery (UpDaemon *daemon) > num_batteries++; > } > > + /* The display battery has an unknown state (Usually ThinkPads on Linux > 4.17 with > + * the battery charge threshold states set). */ > + if (num_batteries > 0 && state_total == UP_DEVICE_STATE_UNKNOWN) { > + /* If all batteries are pending a charge, return the total state > + * as "fully charging". We return that all are fully charged because > + * if a battery is pending a charge, it has been fully charged up to > + * its threshold and is in idle. We could return "pending charge" here > + * but that is too complicated for the display battery */ > + if (num_pending == num_batteries) > + state_total = UP_DEVICE_STATE_FULLY_CHARGED; > + /* Else if there is one battery that is pending but one battery is not, > + * we will return "charging" as the other is charging. */ > + else if (num_pending > 0) > + state_total = UP_DEVICE_STATE_CHARGING; > > If one of the batteries is charging the: > > if (state == UP_DEVICE_STATE_CHARGING) > state_total = UP_DEVICE_STATE_CHARGING; > > Will have already triggered and we never get here, I think this whole bit > can be simplified to: > > if (num_batteries && num_pending == num_batteries && state_total == > UP_DEVICE_STATE_UNKNOWN) > state_total = UP_DEVICE_STATE_FULLY_CHARGED; > Thanks for the feedback. I will make these changes and re-send a new patch. > > With that changed I think that this is a sensible fix for upower. Thanks for actually being interested and looking into it. > > I don't think simply re-introducing your kernel patch is a good idea though, > mostly because of it being dmi-quirk based which means we will need a ton of > quirks if we're not careful. Meh, the only regression that was reported is this one in UPower, and it's a bad one. > > Since you've written the threshold patches I would like to suggest to modify > drivers/acpi/battery.c to hook into the code which has set the new > start-threshold and make acpi_battery_is_charged() use the charge_threshold > rather then full/design capacity on devices with a charge-threshold, then > the kernel will report POWER_SUPPLY_STATUS_FULL when neither > ACPI_BATTERY_STATE_DISCHARGING and ACPI_BATTERY_STATE_CHARGING are set, > which will even make older upower's do the right thing. Well that would be kinda tricky, as we are already hooking INTO the battery module. But I will consult with the Intel guys and look what to do next. But I would really like for this to be merged IF that previous patch gets applied to the kernel. > Generally speaking > making kernel changes which cause regressions for older userspace versions > is not allowed. So even with the upower changes you would need to wait at > least a year before the old version of your > kernel patch can be merged. A year? Wonderful. > > Regards, > > Hans Thanks Hans.
Created attachment 139393 [details] [review] v2-0001-displaydevice-support-Not-Charging-as-a-state.patch This is the patch with the suggestions from Hans applied. This passes all the ThinkPad extended integration tests. It would also be nice to merge the integration tests too for ThinkPad systems to avoid future breakage.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/upower/upower/issues/19.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.