feat(Hardware Support): Razer Tartarus Pro#518
feat(Hardware Support): Razer Tartarus Pro#518dripsnek wants to merge 8 commits intoShadowBlip:mainfrom
Conversation
dripsnek
force-pushed
the
razer_tartarus_pro
branch
from
2c1c8ab to
7c8446e
Compare
dripsnek
force-pushed
the
razer_tartarus_pro
branch
from
7c8446e to
18a2cee
Compare
pastaq
left a comment
pastaq
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review. Overall I am impressed with the direction this is going. I do have a few concerns about a couple of the approaches, but we can discuss them below in each of the issues I raised.
| } | ||
| if info.interface_number() == RAZER_FEATURE_ENDPOINT { | ||
| // Check if we can get serial number | ||
| let mut packet = Self::razer_report(0x0, 0x82, 0x16, &[0x0], &mut razer_message_id); |
There was a problem hiding this comment.
In general, we try to stay away from sending input reports back to the device aside from rumble events in DInput. I would much rather see these sorts of things added to a kernel driver, exposed over sysfs, and controlled by udev, either through rules or the udev crate.
There was a problem hiding this comment.
OpenRazer just added support for the Tartarus Pro so there is no need to have a custom control pane now. Code has (pending push) been adjusted to use those sysfs nodes, but that does introduce a dependency on the razerkbd module for analog mode and potentially profile filtering to specific boards.
| let _ = Self::transaction(packet, &device).unwrap(); | ||
| } | ||
|
|
||
| let mut zeroes = VecDeque::with_capacity(REGRESSION_WINDOW); |
There was a problem hiding this comment.
Can you add a comment explaining the purpose of this array?
| // the ultimate byte being 0. Byte [1] is a status byte which for PC->DEV is 0. Replies | ||
| // always start with 0x2. Bytes [3..5] are used in other Razer devices but not the Tartarus | ||
| // so we leave them 0. | ||
| fn razer_report( |
There was a problem hiding this comment.
I'd prefer we use a PackedStruct instead of manually decoding the reports.
See https://github.com/ShadowBlip/InputPlumber/blob/main/src/drivers/legos/hid_report.rs for an example.
There was a problem hiding this comment.
Section removed as control related code offloaded to razerkbd module
| Ok(events) | ||
| } | ||
|
|
||
| fn handle_analog(&mut self, keys: &[u8]) -> Result<Vec<Event>, Box<dyn Error + Send + Sync>> { |
There was a problem hiding this comment.
In general this entire function is difficult to follow. It is very long, the variable names aren't obvious for what they are attempting to accomplish, and the comments refer to actions that aren't well defined. If you could take another look at this to make it less ambiguous I'd be able to provide more constructive feedback.
Please add a plain English explanation above the function using documentation markers ///
| Ok(Vec::new()) | ||
| } | ||
|
|
||
| fn linear_regression(&self, data: &[f64]) -> Option<f64> { |
There was a problem hiding this comment.
Please add a plain English explanation above the function using documentation markers ///
| // the ultimate byte being 0. Byte [1] is a status byte which for PC->DEV is 0. Replies | ||
| // always start with 0x2. Bytes [3..5] are used in other Razer devices but not the Tartarus | ||
| // so we leave them 0. | ||
| fn razer_report( |
There was a problem hiding this comment.
Please add a plain English explanation above the function using documentation markers ///
There was a problem hiding this comment.
Section removed as control related code offloaded to razerkbd module
| // each; For endpoints 1.1 and 1.3 this is where code 0x04 is interpreted and discarded for | ||
| // endpoint 1.2 as it was a report ID which has served its purpose if we got this far. | ||
| // Given the conversion to variant space it is open to misinterpretation if left there. | ||
| fn handle_basic( |
There was a problem hiding this comment.
Please add a plain English explanation above the function using documentation markers ///
| InputValue::Bool(event.pressed), | ||
| ), | ||
| razer_tartarus_pro::event::KeyCodes::ScrollDown => { | ||
| NativeEvent::new(Capability::Mouse(Mouse::Wheel), InputValue::Float(-1.0)) |
There was a problem hiding this comment.
Is this always -1 from the hardware? Many scroll wheels can have larger values depending on if they are high resolution or moved quickly.
There was a problem hiding this comment.
USB packet captures and evtest at the raw device show it only ever returning 1 and -1, tried scrolling at multiple speeds.
| razer_tartarus_pro::event::KeyCodes::Blank => { | ||
| NativeEvent::new(Capability::NotImplemented, InputValue::None) | ||
| } | ||
| razer_tartarus_pro::event::KeyCodes::ScrollUp => { |
There was a problem hiding this comment.
Please order these events logically to make finding a given event easier in the match list. I.e. scroll wheels together, either numerically or alphabetically for the keys, etc.
| } | ||
|
|
||
| /// List of all capabilities that the Razer Tartarus Pro implements | ||
| pub const CAPABILITIES: &[Capability] = &[ |
dripsnek
force-pushed
the
razer_tartarus_pro
branch
from
18a2cee to
7dc77a8
Compare
… and analog mode are not yet working.
…or variable actuation points, dual binding, retrigger and continuous retrigger. Does not currently emit key events.
… legibility of handle_analog(). Re-ordered items in razer_tartarus_pro.rs to share logical grouping.
dripsnek
force-pushed
the
razer_tartarus_pro
branch
from
7dc77a8 to
dad1e02
Compare
2 participants
This WIP PR implements support for the Razer Tartarus Pro gaming keypad. To date I've not found anything which allows usage of the analog mode specific to this model under Linux. This aims to bridge that gap.
The need for a driver exists because analog mode generates vendor defined HID reports that contain the positional value of each key instead of events. These must be processed and mapped to input events and this seemed like a natural fit for InputPlumber to handle.
I think this device is the first of its kind for InputPlumber and assuming the project wishes to extend support to these peripherals, how to represent them from a profile perspective in my mind is the major piece of work to go.
The driver has two modes, basic and analog. Basic mode simply uses the Tartarus Pro in its power-up configuration as a regular keyboard/mouse hybrid and is configured as any other device. Analog mode requires commanding the Tartarus Pro. This can occur manually before starting InputPlumber or if the OpenRazer kernel module >3.12.0 is loaded the driver will use it to set the mode when InputPlumber starts.
This implementation takes full control of the hidraw (x3) and evdev nodes (x4) but passes through the last endpoint allowing lighting control software e.g. OpenRGB or Polychromatic to manage the RGB functions whilst InputPlumber is running.
The analog event handler is implemented using simple linear regressions on a small buffer* to drive a per-key state machine and supports the following:
* The buffer depth of 5 was based on protocol analysis of fast key transitions from the device
** Quantization and implementation quirks label the minimum at 1.4mm but the Tartarus Pro marketing spec stands.
*** AKA Rapid Trigger from other vendors
Analog mode usage:
Analog mode parameters currently reside at
50-razer_tartarus_pro.yamlin the analogkeys mapping. There are 5 arrays of 20 values, each value left-to-right represents the numbered key on the Tartarus Pro in order, putting aside 0 indexing. All 20 keys have individual analog settings and these will apply to any loaded key binding profiles. Values are sanitised with a specific exception in place for zeroes. Analog parameters cannot be changed at runtime, a restart of InputPlumber is required to take effect.secondary_rebind.yamlLimits to this PR:
Other topics influencing implementation, not necessarily impacting the path out of WIP:
Like a lot of products in this class it is possible to acquire the serial number, firmware revision and other such data from the device registers. This could be used to uniquely assign profiles to a specific device or implement quirks for a given firmware. Should this be incorporated?
The Tartarus Pro is a left-handed device but someone, somewhere will somehow find a way to use 2 of them. Should we embrace this or defend against it?
Assuming this type of device can be included, I propose the following to navigate out of WIP:
feat: add mouse wheel support #514merged to support scroll wheel remapping