I am not sure if the cause of this problem is related to upower, systemd or the patches (see note below) I have installed. Steps to reproduce: 1. restart upowerd 2. wait until battery is critical: computer hibernates as expected. 3. charge computer (or start while connected to power) 4. wait until battery is fully charged (not sure if the battery needs to be fully charged to trigger ) 5. unplug power 6. wait until battery is critical: computer does not hibernate! 7. charge and start computer: computer does not resume from hibernate! Workarround: restart upowerd. I have now a systemd.timer which does this for me every hour to ensure not loosing any session-data when battery is critical. Note: I have this issue on a gentoo-box (Thinkpad X1, 2012-edition) with version upower-0.99.1-r1 ("r1" means, I have aplied the patches from bug https://bugs.freedesktop.org/show_bug.cgi?id=82925) How can I be of help with further debugging of this problem? Is there a way to log things happening when battery enters critical state? Recompile with additional flags, etc?
Created attachment 113012 [details] possible workaround to restart upower after resume from hibernate Howto use workaround: (as root): cp upower-restart-after-hibernate.service /etc/systemd/system systemctl enable upower-restart-after-hibernate.service systemctl status upower-restart-after-hibernate.service It works for me.
Can you get a full debug log from upowerd when reproducing the problem? Testing with 0.99.2 would be appreciated as well.
Created attachment 130886 [details] [review] fix for the bug
I found the problem. The take_action_timeout_cb() function returns G_SOURCE_REMOVE which makes GLib destroy the timeout. However the action_timeout_id stayed != 0 so when warning level turned to "action" again the daemon assumed that the timeout is already set and did nothing. (It only logged: "Not taking action, timeout id already set")
Comment on attachment 130886 [details] [review] fix for the bug Review of attachment 130886 [details] [review]: ----------------------------------------------------------------- I was about to review the patch saying it needed a test case, but you got everything right the first time, coding style, commit message formatting, test case. Stellar work :) Just a couple of nits, and that's good to commit. Either I or one of the other developers on upower should take care of committing this shortly when the changes have been made. And feel free to look into any other problems you see in upower :) ::: src/linux/integration-test @@ +252,4 @@ > with open(self.log.name) as f: > return text in f.read() > > + def have_text_in_log_n_times(self, text, n): Could you make this return the number of times the string is mentioned instead? This would allow us to implement the above function as a special case (count == 1) and have the assertion failures mention the number of times the string is mentioned, instead of simply failing. @@ +794,5 @@ > + self.testbed.uevent(bat0, 'change') > + time.sleep(0.5) > + > + self.assertEqual(self.get_dbus_display_property('WarningLevel'), UP_DEVICE_LEVEL_ACTION) > + self.assertEventually(lambda: self.have_text_in_log_n_times("About to call logind method Hibernate", 2), timeout=300) Can you please avoid using the lambda here? I'd rather the code was as straight forward as possible. It does increase the runtime slightly, but we're still talking tens of seconds to run the whole test suite.
Created attachment 132983 [details] [review] fix for the bug v2 Sorry for not replying for so long time. I only came here to see why my bugfix is not included in the new release (0.99.5). I was not in CC list so I was not notified about your comment. I expected Bugzilla to notify me about new comments automatically. :( > Stellar work :) Thank you for the compliments. > Could you make this return the number of times the string is mentioned instead? > This would allow us to implement the above function as a special case (count == 1) > and have the assertion failures mention the number of times the string is mentioned, > instead of simply failing. Done. Just note that "have_text_in_log()" is actually "count > 0" and not "count == 1" as you mentioned to keep the old behavior. > Can you please avoid using the lambda here? I'd rather the code was as straight > forward as possible. It does increase the runtime slightly, but we're still > talking tens of seconds to run the whole test suite. I changed it to explicit timeout of 20s (+ 0.5s to avoid race conditions). I used the lambda only to avoid duplicating the UP_DAEMON_ACTION_DELAY constant value in tests as this is basically a violation of "Don't Repeat Yourself" rule. (But in case anybody changes the constant either the test will fail and he/she will be notified or in case of lowering the value the test will run longer than it could but it will work.) The new patch (v2) is in the attachment.
Committed to git master, thanks very much!
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.