feat(hotas): enhance VKB and Virpil prosumer protocol support#70
Open
EffortlessSteven wants to merge 3 commits intomainfrom
Open
feat(hotas): enhance VKB and Virpil prosumer protocol support#70EffortlessSteven wants to merge 3 commits intomainfrom
EffortlessSteven wants to merge 3 commits intomainfrom
Conversation
VKB crate enhancements: - Add T-Rudder Mk.IV/V pedal parser (3-axis, hall-effect sensors) - Add axis calibration data structures (per-axis min/center/max/deadzone) - Add VkbDeviceInfo table with 12 device entries for unified lookup - Add T-Rudder and STECS Modern Throttle to VkbDeviceFamily enum - Add T-Rudder report layout constant - Wire T-Rudder profile PID (community-estimated 0x0132) Virpil crate enhancements: - Add WarBRD, WarBRD-D, and MongoosT-50CM3 device profiles - Add multi-device detection module (base+grip+throttle+pedals correlation) - Add DeviceCategory enum for grouping devices by role - Add SetupSummary with HOTAS/full-setup detection helpers All new code includes comprehensive unit tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoEnhance VKB and Virpil HOTAS protocol support with pedals and multi-device detection
WalkthroughsDescription• Add VKB T-Rudder pedal parser with 3-axis support and calibration framework • Add Virpil multi-device detection module for setup categorization • Add WarBRD, WarBRD-D, and MongoosT-50CM3 stick profiles to Virpil • Expand VKB device info table with 12 entries for unified PID lookup Diagramflowchart LR
VKB["VKB Enhancements"]
VKB --> TR["T-Rudder Parser<br/>3-axis pedals"]
VKB --> CAL["Axis Calibration<br/>min/center/max/deadzone"]
VKB --> DT["Device Info Table<br/>12 PID entries"]
VIRPIL["Virpil Enhancements"]
VIRPIL --> PROF["New Stick Profiles<br/>WarBRD/MongoosT"]
VIRPIL --> MD["Multi-Device Detection<br/>Category classification"]
VIRPIL --> SS["Setup Summary<br/>HOTAS/full-setup helpers"]
File Changes1. crates/flight-hotas-vkb/src/t_rudder.rs
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
157
to
163
| /// Return the expected report layout for a given device family. | ||
| pub fn report_layout_for_family(family: VkbDeviceFamily) -> VkbJoystickReportLayout { | ||
| match family { | ||
| VkbDeviceFamily::SemThq => VKB_SEM_THQ_LAYOUT, | ||
| VkbDeviceFamily::TRudder => VKB_T_RUDDER_LAYOUT, | ||
| _ => VKB_JOYSTICK_STANDARD_LAYOUT, | ||
| } |
There was a problem hiding this comment.
1. Stecs layout not mapped 🐞 Bug ✓ Correctness
VkbDeviceFamily::from_pid can return StecsModernThrottle, but report_layout_for_family has no arm for it and falls back to VKB_JOYSTICK_STANDARD_LAYOUT (6 axes + hat, 21-byte payload). This produces an incorrect layout for STECS Modern Throttle relative to the crate’s own STECS parser and can cause mis-parsing or incorrect validation in downstream users of this API.
Agent Prompt
### Issue description
`report_layout_for_family()` does not handle `VkbDeviceFamily::StecsModernThrottle`, so callers get the standard stick layout, which includes hats and 6 axes.
### Issue Context
The crate already has a dedicated STECS Modern Throttle parser (`stecs_modern`) with a different axis set and report layout expectations.
### Fix Focus Areas
- crates/flight-hotas-vkb/src/protocol.rs[157-163]
- crates/flight-hotas-vkb/src/protocol.rs[126-155]
- crates/flight-hotas-vkb/src/stecs_modern.rs[30-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e585ef774
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match family { | ||
| VkbDeviceFamily::SemThq => VKB_SEM_THQ_LAYOUT, | ||
| VkbDeviceFamily::TRudder => VKB_T_RUDDER_LAYOUT, | ||
| _ => VKB_JOYSTICK_STANDARD_LAYOUT, |
There was a problem hiding this comment.
report_layout_for_family has no VkbDeviceFamily::StecsModernThrottle arm, so STECS devices fall through to VKB_JOYSTICK_STANDARD_LAYOUT (hat byte + 21-byte payload). In the same module, STECS entries in VKB_DEVICE_TABLE are declared with hat_count: 0 and min_report_bytes: 20, so callers that use the family layout for parsing/validation will compute the wrong offsets and can reject or misinterpret STECS reports.
Useful? React with 👍 / 👎.
Comment on lines
+283
to
+286
| axis_count: 6, | ||
| max_buttons: 64, | ||
| hat_count: 0, | ||
| min_report_bytes: 20, |
There was a problem hiding this comment.
The STECS Modern Mini/Max table rows advertise axis_count: 6 and min_report_bytes: 20, but parse_stecs_mt_report (stecs_modern.rs) parses four axes and accepts 17 bytes total (16-byte payload after report ID). This inconsistency makes vkb_device_info unreliable for sizing axis storage or report-length prechecks, which can cause valid STECS input to be rejected or mapped incorrectly when metadata-driven paths are used.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds expanded VKB and VIRPIL HOTAS device/protocol support within their respective crates, including new device-specific parsers, device lookup metadata, and multi-device setup detection utilities.
Changes:
- Introduces a VKB T-Rudder HID report parser plus associated report layout, PID plumbing, and profile updates.
- Adds VKB axis calibration types (per-axis + per-device) with normalization helpers and unit tests.
- Extends VIRPIL with additional stick profiles and a multi-device detection module (category classification + setup summary helpers).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/flight-hotas-vkb/src/t_rudder.rs | New VKB T-Rudder HID report parser + tests. |
| crates/flight-hotas-vkb/src/protocol.rs | Adds T-Rudder/STECS Modern family support, a VKB device info table, and T-Rudder report layout. |
| crates/flight-hotas-vkb/src/profiles.rs | Wires T-Rudder PID into the built-in profile registry. |
| crates/flight-hotas-vkb/src/lib.rs | Exposes new VKB modules and re-exports new public types/constants. |
| crates/flight-hotas-vkb/src/calibration.rs | New calibration structures and normalization logic + tests. |
| crates/flight-hotas-virpil/src/profiles.rs | Adds WarBRD / WarBRD-D / MongoosT-50CM3 stick profiles + lookup tests. |
| crates/flight-hotas-virpil/src/multi_device.rs | New VIRPIL multi-device categorization and setup detection utilities + tests. |
| crates/flight-hotas-virpil/src/lib.rs | Registers the new VIRPIL multi_device module and re-exports its API. |
Comment on lines
+91
to
+96
| if norm < self.deadzone { | ||
| 0.0 | ||
| } else { | ||
| // Rescale above deadzone to fill [0.0, 1.0] | ||
| ((norm - self.deadzone) / (1.0 - self.deadzone)).clamp(0.0, 1.0) | ||
| } |
There was a problem hiding this comment.
deadzone is documented as 0.0..=0.5, but it’s not validated/clamped. If a caller constructs AxisCalibration with deadzone >= 1.0 (or negative), the rescale path divides by (1.0 - deadzone) which can yield inf/NaN and propagate through clamp(). Consider clamping deadzone to a safe range (e.g., 0.0.. < 1.0) inside normalize_*, or adding a constructor/validation method and making the fields private to enforce the invariant.
Comment on lines
1054
to
1061
| /// Note: The Mk.V PID is not yet confirmed; this profile uses the Mk.IV layout | ||
| /// which is expected to be identical. | ||
| pub fn t_rudder_profile() -> VkbDeviceProfile { | ||
| VkbDeviceProfile { | ||
| device_name: "VKB T-Rudder Mk.IV", | ||
| vid: VKB_VENDOR_ID, | ||
| // T-Rudder PID not in device_support.rs yet; leave empty for now. | ||
| // Users should match by descriptor discovery or explicit configuration. | ||
| pids: &[], | ||
| pids: &[VKB_T_RUDDER_MK5_PID], | ||
| axes: &T_RUDDER_AXES, |
There was a problem hiding this comment.
t_rudder_profile() advertises Mk.IV in device_name/notes but matches by VKB_T_RUDDER_MK5_PID (Mk.V community estimate). This is likely to confuse users and downstream UI/logging. Consider renaming the profile/device name and notes to “Mk.IV/Mk.V” (or “Mk.V (est.)”) to reflect the PID being matched.
Comment on lines
+168
to
+176
| /// Complete VID/PID entry for a VKB device. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct VkbDeviceInfo { | ||
| /// USB Product ID. | ||
| pub pid: u16, | ||
| /// Device family. | ||
| pub family: VkbDeviceFamily, | ||
| /// Human-readable product name. | ||
| pub name: &'static str, |
There was a problem hiding this comment.
The comment “Complete VID/PID entry for a VKB device” is misleading: VkbDeviceInfo only stores pid (VID is implicitly VKB_VENDOR_ID). Consider adjusting the docstring to avoid implying VID is stored, or add a vid field if callers are expected to treat this as a full VID/PID tuple.
Comment on lines
+125
to
+140
| // ─── Shared helpers (same as protocol.rs) ───────────────────────────────────── | ||
|
|
||
| fn le_u16(bytes: &[u8], offset: usize) -> u16 { | ||
| let low = bytes.get(offset).copied().unwrap_or(0); | ||
| let high = bytes.get(offset + 1).copied().unwrap_or(0); | ||
| u16::from_le_bytes([low, high]) | ||
| } | ||
|
|
||
| fn normalize_u16(raw: u16) -> f32 { | ||
| (raw as f32 / u16::MAX as f32).clamp(0.0, 1.0) | ||
| } | ||
|
|
||
| fn normalize_signed(raw: u16) -> f32 { | ||
| ((raw as f32 / 32767.5) - 1.0).clamp(-1.0, 1.0) | ||
| } | ||
|
|
There was a problem hiding this comment.
This file redefines le_u16, normalize_u16, and normalize_signed, which already exist (with identical implementations) in protocol.rs and input.rs. Adding a third copy increases the chance of these helpers drifting over time. Consider moving these helpers into a shared pub(crate) utility module and reusing them from all parsers.
Suggested change
| // ─── Shared helpers (same as protocol.rs) ───────────────────────────────────── | |
| fn le_u16(bytes: &[u8], offset: usize) -> u16 { | |
| let low = bytes.get(offset).copied().unwrap_or(0); | |
| let high = bytes.get(offset + 1).copied().unwrap_or(0); | |
| u16::from_le_bytes([low, high]) | |
| } | |
| fn normalize_u16(raw: u16) -> f32 { | |
| (raw as f32 / u16::MAX as f32).clamp(0.0, 1.0) | |
| } | |
| fn normalize_signed(raw: u16) -> f32 { | |
| ((raw as f32 / 32767.5) - 1.0).clamp(-1.0, 1.0) | |
| } | |
| // ─── Shared helpers (reused from protocol.rs) ───────────────────────────────── | |
| use crate::protocol::{le_u16, normalize_signed, normalize_u16}; |
Comment on lines
+374
to
+410
| pub static MONGOOST_AXES: &[AxisDescriptor] = &[ | ||
| AxisDescriptor { | ||
| index: 0, | ||
| role: AxisRole::StickX, | ||
| label: "X (roll)", | ||
| centred: true, | ||
| }, | ||
| AxisDescriptor { | ||
| index: 1, | ||
| role: AxisRole::StickY, | ||
| label: "Y (pitch)", | ||
| centred: true, | ||
| }, | ||
| AxisDescriptor { | ||
| index: 2, | ||
| role: AxisRole::Twist, | ||
| label: "Z (twist)", | ||
| centred: true, | ||
| }, | ||
| AxisDescriptor { | ||
| index: 3, | ||
| role: AxisRole::SecondaryRotary, | ||
| label: "SZ (secondary rotary)", | ||
| centred: false, | ||
| }, | ||
| AxisDescriptor { | ||
| index: 4, | ||
| role: AxisRole::SlewLever, | ||
| label: "SL (slew lever)", | ||
| centred: false, | ||
| }, | ||
| ]; | ||
|
|
||
| static MONGOOST_HATS: &[HatDescriptor] = &[HatDescriptor { | ||
| label: "Main hat", | ||
| hat_type: HatType::EightWay, | ||
| }]; |
There was a problem hiding this comment.
MONGOOST_AXES/MONGOOST_HATS are byte-for-byte identical to WARBRD_AXES/WARBRD_HATS (and the docs explicitly say the layouts are identical). Consider reusing the same slice(s) (e.g., aliasing MONGOOST_AXES to WARBRD_AXES) to avoid duplicated definitions that could drift if one is updated later.
Suggested change
| pub static MONGOOST_AXES: &[AxisDescriptor] = &[ | |
| AxisDescriptor { | |
| index: 0, | |
| role: AxisRole::StickX, | |
| label: "X (roll)", | |
| centred: true, | |
| }, | |
| AxisDescriptor { | |
| index: 1, | |
| role: AxisRole::StickY, | |
| label: "Y (pitch)", | |
| centred: true, | |
| }, | |
| AxisDescriptor { | |
| index: 2, | |
| role: AxisRole::Twist, | |
| label: "Z (twist)", | |
| centred: true, | |
| }, | |
| AxisDescriptor { | |
| index: 3, | |
| role: AxisRole::SecondaryRotary, | |
| label: "SZ (secondary rotary)", | |
| centred: false, | |
| }, | |
| AxisDescriptor { | |
| index: 4, | |
| role: AxisRole::SlewLever, | |
| label: "SL (slew lever)", | |
| centred: false, | |
| }, | |
| ]; | |
| static MONGOOST_HATS: &[HatDescriptor] = &[HatDescriptor { | |
| label: "Main hat", | |
| hat_type: HatType::EightWay, | |
| }]; | |
| pub static MONGOOST_AXES: &[AxisDescriptor] = WARBRD_AXES; | |
| static MONGOOST_HATS: &[HatDescriptor] = WARBRD_HATS; |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add StecsModernThrottle arm to report_layout_for_family with new VKB_STECS_MODERN_LAYOUT constant (4 axes, 2 button words, 16 bytes) - Fix STECS Modern Mini/Max metadata: axis_count 6->4, min_report_bytes 20->17 to match parse_stecs_mt_report expectations - Clamp deadzone to 0.0..0.999 in normalize_unsigned and normalize_signed to prevent inf/NaN from division by zero when deadzone == 1.0 - Rename T-Rudder profile from Mk.IV to Mk.IV/V since it matches VKB_T_RUDDER_MK5_PID; update notes accordingly - Fix VkbDeviceInfo docstring: struct stores PID only, not VID/PID - Deduplicate le_u16/normalize_u16/normalize_signed helpers: make them pub(crate) in protocol.rs and import in t_rudder.rs - Replace duplicate MONGOOST_AXES/MONGOOST_HATS with aliases to WARBRD_AXES/WARBRD_HATS (identical axis layout per shared VPC firmware) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enhances VKB and Virpil HOTAS crates with additional protocol support, device profiles, and utilities.
VKB Crate (flight-hotas-vkb)
Virpil Crate (flight-hotas-virpil)
Testing
All new code includes comprehensive unit tests. All existing tests continue to pass.