Conversation
tdhowe
commented
Mar 1, 2026
tdhowe
commented
- Reworked how it discovers devices a bit so it's a little more scalable if we add support for other devices in the future
- Added debug logging for chatmix slider
awth13
left a comment
awth13
left a comment
There was a problem hiding this comment.
Hi! Thank you for your work, this looks great!
I do have a couple minor comments regarding the implementation, please let me know what you think.
Arctis_7_Plus_ChatMix.py
Outdated
|
|
||
| def _bind_device(self): | ||
| for device_name, device_info in supported_devices.items(): | ||
| dev = usb.core.find(idVendor=device_info["idVendor"], idProduct=device_info["idProduct"]) |
There was a problem hiding this comment.
Instead of calling find() in a loop, can we do it once and iterate over the result? Looks like usb.core.find() can return an iterator for multiple devices. We could, e.g., match on vendor id, which will always be the same.
Arctis_7_Plus_ChatMix.py
Outdated
| import re | ||
| import usb.core | ||
|
|
||
| supported_devices = { |
There was a problem hiding this comment.
Perhaps the top-level key into the device record should be the product id? Then we can match against it in _bind_device() when we get an iterator from usb.core.find().
Arctis_7_Plus_ChatMix.py
Outdated
| self.die_gracefully(trigger="Couldn't find supported arctis model") | ||
| return | ||
|
|
||
| self.log.info(f"Found supported device: {device_name} ({hex(device_info['idVendor'])}:{hex(device_info['idProduct'])})") |
There was a problem hiding this comment.
I understand that device_name gets implicitly bound in the loop above but I think it would be clearer to get it explicitly from a supported_devices record keyed by product id (see my comment on R30).
Arctis_7_Plus_ChatMix.py
Outdated
|
|
||
| self.log.info(f"Found supported device: {device_name} ({hex(device_info['idVendor'])}:{hex(device_info['idProduct'])})") | ||
| self.dev = dev | ||
| inf_idx = device_info["hid_interface_index"] |
There was a problem hiding this comment.
Why the different names for the variable here and for the field in supported_devices?
Arctis_7_Plus_ChatMix.py
Outdated
| if __name__ == '__main__': | ||
| a7pcm_service = Arctis7PlusChatMix() | ||
| a7pcm_service.start_modulator_signal() | ||
| a7pcm_service.start_modulator_signal() No newline at end of file |
There was a problem hiding this comment.
Looks like the only change here is that a newline got accidentally removed.
|
@awth13 Sure I will address these later this week. I also realized I didnt update the install stuff - I had to do it manually to get it working (dev.arctis7 was not in my systemd device tree). Do you want that in a separate PR? |
Yes, you'll have to add the new device to the installed udev rules. I think it should be in this PR since it is part of adding the new device support. I trust that you will test it as well since I do not have access to the device. |
Also this part in the .service file: to I don't have that device on my pc. I changed mine locally to pipewire instead. Any reason to have the device dependency instead of just pipewire? |
|
Ah I see... the rules file names the usb device, is that right? I havent worked much with those before. |
Yes, however, once pyusb module binds the device, the device gets detached from the kernel and, consequently, removed from the device tree. I haven't looked at this code base in a while and, now that you mention it, I don't think these lines are necessary. We only want This (in combination with the udev rules) tells systemd to start the the service running our script when the device is plugged in. In any case, this is out of scope for this PR -- I will look at it myself when I have time. You can safely remove or ignore |
- Reworked how it discovers devices a bit so it's a little more scalable if we add support for other devices in the future - Added opcode filtering on first word so other messages aren't interpreted as chatmix - Added debug logging for chatmix slider - Misc cleanup for rules and service registration
2 participants
