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.

Sign up for GitHub

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

Jump to bottom

Ignore battery level zero when notifying #2248

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Ignore battery level zero when notifying #2248

merged 1 commit into from
Jul 2, 2024

Conversation

NeilGirdhar
Copy link
Contributor

Fixes #2247

@NeilGirdhar
Copy link
Contributor Author

@z3ntu Would you mind taking a look at this? The 0% notifications are totally avoidable and really annoying 😄

@NeilGirdhar NeilGirdhar mentioned this pull request May 13, 2024
Open
z3ntu
z3ntu approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@z3ntu z3ntu left a comment

Choose a reason for hiding this comment

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

Should've thought about just doing this earlier... Way better than before

I also did some refactoring also to clean up a few bits

LGTM, thank you!

NeilGirdhar
NeilGirdhar commented Jul 2, 2024
View reviewed changes
daemon/openrazer_daemon/misc/battery_notifier.py
self._logger.debug("Got bogus battery value: {0}, ignoring.".format(battery_level))
# Since we don't update _last_notify_time here we're going to retry very soon again.
# Sleep a bit so we don't spam the device with requests.
time.sleep(10)
Copy link
Contributor Author

@NeilGirdhar NeilGirdhar Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you need to sleep here? Can't you just return and the caller sleep for you?

Copy link
Member

Choose a reason for hiding this comment

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

Sleep for 0.1 seconds, if some device consistently returns 0 for some reason I don't want to spam CPU and USB bus because of that.

Copy link
Contributor Author

@NeilGirdhar NeilGirdhar Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe you didn't notice, but I moved the update of _last_notify_time, so it won't get spammed? After returning, the caller waits 0.1s, and then calls again. notify_battery won't accept the call based on self.frequency.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly because you moved it the spamming starts because the outer loop only sleeps 0.1 and that function itself was checking whether enough time has passed. If _last_notify_time is not updated, then it'll retry again after 0.1 seconds because the if in that function still triggers.

I also tried it at runtime, and it looks like this then:

2024-07-02 21:45:39 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:40 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:41 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:42 | razer.device0                  | DEBUG    | DBus call get_battery
2024-07-02 21:45:43 | razer.device0                  | DEBUG    | DBus call get_battery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you have self.frequency set to? I thought self.frequency was supposed to control how often you collect requests?

Copy link
Member

Choose a reason for hiding this comment

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

If you look at the code in this file it's a bit funky written. There's a loop that calls notify_battery every 0.1 seconds and only that function decides it's not time yet to call. No clue why it was written that way (the code dates back to 2016), and it should probably get refactored because it's already now a pretty tight loop it seems.

I'm going to merge the current patch now, feel free to improve the code in this function, it's definitely desperately needed! And I personally don't really care about battery notifier since I don't own any wireless Razer device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, the code is super weird. It would have been better to just sleep by self.frequency in the first place.

Thanks for the merge!

Just want to say though that if you don't remove sleep(10) , someone may complain that setting the frequency parameter seems to have no effect sometimes. Essentially 10 becomes the temporary frequency when it hits bad data. And if it hits a lot of bad data in a row, you may not get an update in quite a while. Also, is 10Hz really spamming anyway? Seems like video games would poll the mouse more often than that.

Copy link
Member

Choose a reason for hiding this comment

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

Just want to say though that if you don't remove sleep(10) , someone may complain that setting the frequency parameter seems to have no effect sometimes.

I'm aware of that and will put the blame on myself if that happens ;) Hopefully once I have some time to improve the driver we'll have less (or no) of these situations where the driver randomly reports 0 because of missing error handling.

Also, is 10Hz really spamming anyway? Seems like video games would poll the mouse more often than that.

The daemon is running always, video games only when you actually want to run them. So I'm more careful there.

@NeilGirdhar @z3ntu
daemon: Ignore bogus 0% battery level when notifying
b996ba4 
Due to various issues in the stack sometimes we get bogus 0% battery
percentage and showing a notification for that is pretty useless, so
ignore that.

[Luca: refactor a bit]

Fixes openrazer#2247
@z3ntu z3ntu merged commit b996ba4 into openrazer:master Jul 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@z3ntu z3ntu z3ntu approved these changes

Assignees
No one assigned
Labels
bug enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spurious battery level notification
2 participants
@NeilGirdhar @z3ntu