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

Add mouse DPI notifier #2238

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

justintime4tea
Copy link

@justintime4tea justintime4tea commented Apr 26, 2024

It is the intent of this PR to propose a "notifier" similar to that of the existing BatteryNotifier.

It would be more ideal to create a general purpose "notifier" that could be used to support both cases; battery and mouse DPI notifications. I hadn't the time to take this on, would like an immediate solution to know what my mouse DPI is when changing it via physical button, and felt this might be a good way to start a conversation.

Also, it would be ideal if notifications, be them for the battery or the mouse DPI, were triggered in response to the battery level or mouse DPI changing, through some eventing mechanism, rather than short-polling. I have no idea the impact of making the get_dpi_xy or available_dpi DBUS calls every 500ms (1000ms for the case of battery).

@justintime4tea justintime4tea marked this pull request as draft April 27, 2024 02:00
@justintime4tea
Copy link
Author

justintime4tea commented Apr 27, 2024

Found an issue with the polling of get_dpi_xy from system journal/log:

kernel: razermouse: Command timed out. status: 04 transaction_id.id: 1f remaining_packets: 00 protocol_type: 00 data_size: 07, command_class: 04, command_id.id: 85 Params: 00000000000000000000000000000000 .

It seems that when the mouse receiver is connected, and the mouse cannot be reached through the receiver, this error/warning is logged because the command times out? Preliminary thoughts, anyway.

Will investigate and bring out of draft once resolved.

@z3ntu
Copy link
Member

z3ntu commented Apr 27, 2024

Interesting idea. What is quite unfortunate for this and any other kind of 'notifier' is that I'm not aware of any mechanism where the device tells us actively that something has changed as opposed to us querying the value continuously.

I'm also wondering if we could limit this code to devices with DPI buttons or something but I don't think we have this information available at all, because we're just wasting power for devices where you can't change the DPI on the device anyways.

But for sure for this I'd disable the notifier by default and interested users can enable it if they want to.

@justintime4tea
Copy link
Author

justintime4tea commented May 2, 2024

Interesting idea. What is quite unfortunate for this and any other kind of 'notifier' is that I'm not aware of any mechanism where the device tells us actively that something has changed as opposed to us querying the value continuously.

The first case, perhaps the easiest case, is when the user changes the DPI from an application like Polychromatic. They see the DPI they are setting.

  • I'd wager 99% of the time "d-bus triggered" DPI changes do not warrant a notification.
  • One case that may warrant a notification is if one of these applications, like Polychromatic, change the DPI through automation or hotkeys.

For the second case, a physical button being pressed to change the DPI, we may be able to leverage the raw mouse event handling done in the mouse driver.

One example use case (in code) for this raw event processing is for wheel tilt edge detection for various devices (screenshot below). This is one place where we have the opportunity to "act" upon the pressing of the DPI button.

image

Here's a working example of acting upon the DPI button being clicked on (screenshot below).

image

I don't have any solid ideas yet for any specific actions to take, but this is where my archeological dig has landed me thus far 😂.

Another opportunity for detecting changes would be detecting changes to the "dpi device file" from within openrazer-daemon; when /sys/devices/pci0000:00/0000:00:00.0/usb0/0-0/0-0:0.0/0000:000:0000.0000/dpi changes, then notify.

I'm also wondering if we could limit this code to devices with DPI buttons or something but I don't think we have this information available at all, because we're just wasting power for devices where you can't change the DPI on the device anyways.

If there are no ways to identify devices with changing DPI's via code, then we could selectively enable this for those devices that we do know ahead of time; ie: Razer Pro Click. If people come asking for support for a device that is not currently supported, it should be relatively simple to add such support, like is done with other device specific code/support.

But for sure for this I'd disable the notifier by default and interested users can enable it if they want to.

I agree we should leave it disabled by default, like you suggested below. This should solve the running unnecessarily in the general case.

@z3ntu
Copy link
Member

z3ntu commented Jul 2, 2024

Okay.. So some time ago I booted up a Windows VM with Synapse and also saw that they have this popup when you change the DPI using the buttons so having this functionality is probably desired by some people.

Assuming we will switch devices with DPI buttons to "driver mode" (from "device mode") then we can handle the button events ourselves and then we obviously know when the button is pressed since we need to handle it ourselves anyways.

But I think doing that without regressing some functionality on some random mice for users is not possible yet, so we probably want to have something for the short-term if somebody really wants this...

So.. to the code:

For starters what I don't quite get is why you built the loop the way you did, every 0.1 seconds in any case you call notify() and only in there you check whether enough time has elapsed? Why don't you sleep for 'frequency' milliseconds and only then run something? Other than that, this looks reasonable I think, I guess some code is copied from battery manager etc, so I didn't look into the notify2 usage too closely for example.

@z3ntu
Copy link
Member

z3ntu commented Sep 3, 2024

@justintime4tea Any update on this?

@justintime4tea
Copy link
Author

@justintime4tea Any update on this?

There are no updates as of now, but I will take a look today and try to make some sort of progress. Thanks for the reminder! 🙏

@justintime4tea
Copy link
Author

justintime4tea commented Sep 16, 2024

But I think doing that without regressing some functionality on some random mice for users is not possible yet, so we probably want to have something for the short-term if somebody really wants this...

I agree wholeheartedly. In the future, if mice are in "driver" mode, we won't have to poll anything and will be much cleaner.

So.. to the code:

For starters what I don't quite get is why you built the loop the way you did, every 0.1 seconds in any case you call notify() and only in there you check whether enough time has elapsed? Why don't you sleep for 'frequency' milliseconds and only then run something? Other than that, this looks reasonable I think, I guess some code is copied from battery manager etc, so I didn't look into the notify2 usage too closely for example.

As far as the 0.1 sleep and coding style/structure, I just copied BatteryNotifier and only changed the core logic for determining when to notify and what to notify, keeping the existing structures and style in-place.

I have made three updates:

  • The loop will now bail early if "not ready to notify" per your feedback
    • I kept the 0.1 sleep in-between the loop iterations.
    • The 0.1 sleep may not even be necessary, but it also might be necessary if the event loop being used by openrazer (GLib?) is anything like JavaScripts whereby the sleep provides a chance to "yield" to other work. Without 0.1, if we bail immediately because we're "not ready" to notify, we'll "spin" in our loop very tightly.
      • I'm not knowledgeable enough regarding the event loop being used to schedule work, but even with newer changes to BatteryNotifier, the 0.1 sleep remains. My guess was that whoever authored the BatteryNotifier knew better than I, regarding such matters.
      • If you can confirm we don't need it, I'll gladly remove the 0.1 sleep.
  • I removed notify2 dependency and replaced it with a "subproc" call to notify-send; it was removed by BatteryNotifier and I wanted to keep the "notifiers" congruent.
  • I changed the default configuration for the Mouse DPI notifier to be false and added a warning regarding the use of libdbus and it not being OOM safe.

@justintime4tea
Copy link
Author

PS: I'm "battle testing" this by running with a 500ms frequency and leaving it run for quite some time to make sure these 500ms back-to-back dbus calls don't cause issues. My system isn't a great test for this, though, because it is ridiculously beefy...

@justintime4tea
Copy link
Author

justintime4tea commented Sep 16, 2024

Two more considerations:

  1. I wonder what the impact of the DPI notifier is on the battery of the mouse (when wireless)

    If the get_dpi call is constantly polling the mouse every 500ms, does that happen energy efficiently over existing communications with the mouse? Or, will we be draining the battery more?

  2. When the mouse disconnects (idle), and I suspect this would be the same for the battery notifier but haven't confirmed, an error is raised in system log for each timeout/failure to get_dpi until the mouse turns back on.

    a. I have a frequency/poll-rate of 500ms and the timeout of the get_dpi call seems to be 1000ms, what does this mean? At some point, some upper bound will be reached; OOM?

    b. Error:

Sep 16 03:26:26 fedora kernel: razermouse: Command timed out. status: 04 transaction_id.id: 1f remaining_packets: 00 protocol_type: 00 data_size: 07, command_class: 04, command_id.id: 85 Params: 00000000000000000000000000000000 .


dpi_level = str(dpi_level_x)
if dpi_level_x != dpi_level_y:
dpi_level = "[x:{0}, y:{1}]"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need some .format()?


def close(self):
"""
Close the manager, stop ripple thread
Copy link
Member

Choose a reason for hiding this comment

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

ripple -> DPI notifier

mouse_dpi_notifier = False

# Mouse DPI notification frequency [ms] (0 to disable)
# WARNING: The openrazer-daemon will poll an openrazer DBUS service at this frequency
Copy link
Member

Choose a reason for hiding this comment

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

"[...] will poll the mouse at this frequency"

Also, why have both = 0 here to disable but also the boolean? Can we get rid of this boolean for example?


# Mouse DPI notification frequency [ms] (0 to disable)
# WARNING: The openrazer-daemon will poll an openrazer DBUS service at this frequency
# WARNING: The openrazer dbus service uses libdbus which is not out-of-memory safe- YMMV
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate what this means for the end user?

@z3ntu
Copy link
Member

z3ntu commented Sep 24, 2024

Also when you think this is ready, please set as Ready on GitHub (so it's not a Draft anymore)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants