feat(hotas-virpil): add protocol parser, button matrix, and axis curves#63
Open
EffortlessSteven wants to merge 2 commits intomainfrom
Open
feat(hotas-virpil): add protocol parser, button matrix, and axis curves#63EffortlessSteven wants to merge 2 commits intomainfrom
EffortlessSteven wants to merge 2 commits intomainfrom
Conversation
- Virpil USB HID protocol parsing for Alpha, WarBRD, MongoosT, ACE Pedals - Button matrix resolver for shifted/layered inputs - Axis curve reading and application 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! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (4)
✨ 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 QodoAdd VIRPIL axis curves, button matrix, and unified protocol support
WalkthroughsDescription• Adds axis curve parsing and application for VIRPIL device firmware response curves • Implements button matrix resolver supporting shifted/layered inputs • Introduces unified protocol types and parsers for grip, base, and throttle devices • Exports new public API for axis curves, button matrix, and device protocol handling Diagramflowchart LR
A["Axis Curves Module"] -->|"Control points & interpolation"| B["apply_curve"]
C["Button Matrix Module"] -->|"Physical to logical mapping"| D["resolve_shifted"]
E["Protocol Extensions"] -->|"Device family detection"| F["VirpilProtocol"]
F -->|"Unified parsing"| G["GripState/BaseState/ThrottleState"]
A --> H["Public API"]
C --> H
F --> H
File Changes1. crates/flight-hotas-virpil/src/axis_curves.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
+141
to
+153
| pub fn reverse(&self, button_id: u8) -> Option<(u8, u8, u8)> { | ||
| if button_id == 0 { | ||
| return None; | ||
| } | ||
| let id = button_id as u16 - 1; | ||
| let layer = (id / self.buttons_per_layer) as u8; | ||
| if layer >= self.shift_layers { | ||
| return None; | ||
| } | ||
| let within_layer = (id % self.buttons_per_layer) as u8; | ||
| let row = within_layer / self.cols; | ||
| let col = within_layer % self.cols; | ||
| Some((row, col, layer)) |
There was a problem hiding this comment.
1. Buttonmatrix reverse can panic 🐞 Bug ⛯ Reliability
ButtonMatrix::new() allows rows==0 or cols==0, which makes buttons_per_layer==0; reverse() then divides/modulos by buttons_per_layer and cols, causing a division-by-zero panic for any non-zero button_id. This is a reliability issue because it can crash consumers given an easy-to-construct invalid matrix.
Agent Prompt
## Issue description
`ButtonMatrix::reverse()` can panic due to division/modulo by zero because `ButtonMatrix::new()` permits `rows == 0` or `cols == 0`.
## Issue Context
The public docs state `rows`/`cols` must be ≥ 1, but this is not enforced. A consumer can easily construct an invalid matrix and trigger a runtime crash by calling `reverse()` with any `button_id != 0`.
## Fix Focus Areas
- crates/flight-hotas-virpil/src/button_matrix.rs[71-80]
- crates/flight-hotas-virpil/src/button_matrix.rs[141-153]
## Suggested direction
- Change `pub fn new(...) -> Result<Self, ButtonMatrixError>` and validate `rows != 0 && cols != 0` and `total_buttons <= 255` (call `validate()` internally).
- If you want to keep `new(...) -> Self`, then:
- introduce `debug_assert!(rows > 0 && cols > 0)` and
- harden `reverse()` with early returns when `self.buttons_per_layer == 0 || self.cols == 0`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Adds Virpil VPC protocol utilities to flight-hotas-virpil, aiming to provide higher-level device identification plus reusable helpers for interpreting reports and firmware configuration.
Changes:
- Introduces
VirpilDeviceFamily+VirpilProtocolhandle and unified report parsers (parse_grip_report,parse_base_report,parse_throttle_report). - Adds
ButtonMatrixutility for resolving (row, col, layer) → logical button ID mapping. - Adds
AxisCurveparsing/application utilities for Virpil firmware response curves and re-exports new APIs from the crate root.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| crates/flight-hotas-virpil/src/protocol.rs | Adds device family classification, a PID-based protocol handle, and unified report parsing types/functions + tests. |
| crates/flight-hotas-virpil/src/lib.rs | Exposes new modules and re-exports the new protocol/button-matrix/axis-curve APIs. |
| crates/flight-hotas-virpil/src/button_matrix.rs | New button matrix resolver with shift-layer support and reverse mapping + tests. |
| crates/flight-hotas-virpil/src/axis_curves.rs | New axis-curve wire-format parser and curve application logic + tests. |
Comment on lines
+196
to
+202
| // Below the first point | ||
| if input <= points[0].x { | ||
| return points[0].y; | ||
| } | ||
| // Above the last point | ||
| if input >= points[points.len() - 1].x { | ||
| return points[points.len() - 1].y; |
There was a problem hiding this comment.
apply_curve’s docstring says values below the first control point or above the last are “extrapolated from the nearest segment”, but the implementation clamps by returning the first/last point’s y (constant extension). Either implement true linear extrapolation using the first/last segment slope, or update the documentation to match the current behavior.
Suggested change
| // Below the first point | |
| if input <= points[0].x { | |
| return points[0].y; | |
| } | |
| // Above the last point | |
| if input >= points[points.len() - 1].x { | |
| return points[points.len() - 1].y; | |
| // Below the first point: extrapolate from the first segment | |
| if input <= points[0].x { | |
| let p0 = &points[0]; | |
| let p1 = &points[1]; | |
| let dx = p1.x - p0.x; | |
| if dx.abs() < 1e-12 { | |
| return p0.y; | |
| } | |
| let t = (input - p0.x) / dx; | |
| let result = p0.y + t * (p1.y - p0.y); | |
| return result.clamp(0.0, 1.0); | |
| } | |
| // Above the last point: extrapolate from the last segment | |
| if input >= points[points.len() - 1].x { | |
| let last = points.len() - 1; | |
| let p0 = &points[last - 1]; | |
| let p1 = &points[last]; | |
| let dx = p1.x - p0.x; | |
| if dx.abs() < 1e-12 { | |
| return p1.y; | |
| } | |
| let t = (input - p0.x) / dx; | |
| let result = p0.y + t * (p1.y - p0.y); | |
| return result.clamp(0.0, 1.0); |
Comment on lines
+338
to
+344
| // ─── Unified report types ───────────────────────────────────────────────────── | ||
|
|
||
| use thiserror::Error; | ||
|
|
||
| /// Error from the unified report parsers. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Error)] | ||
| pub enum VirpilParseError { |
There was a problem hiding this comment.
use thiserror::Error; is placed mid-file, unlike the rest of this crate where use items are grouped at the top of the module. Consider moving this import up with the other use statements to keep the module layout consistent and easier to scan.
Comment on lines
+349
to
+456
| /// Unified grip/stick state: normalised axes + button bitmap. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct GripState { | ||
| /// Normalised axis values `[0.0, 1.0]`. Typically 5 axes (X, Y, Z, SZ, SL). | ||
| pub axes: Vec<f32>, | ||
| /// Raw button bytes (LSB-first per byte). | ||
| pub buttons_raw: Vec<u8>, | ||
| } | ||
|
|
||
| impl GripState { | ||
| /// Return `true` if button `n` (1-indexed) is pressed. | ||
| pub fn is_pressed(&self, n: u8) -> bool { | ||
| if n == 0 { | ||
| return false; | ||
| } | ||
| let idx = (n - 1) as usize; | ||
| let byte = idx / 8; | ||
| let bit = idx % 8; | ||
| self.buttons_raw | ||
| .get(byte) | ||
| .is_some_and(|b| (b >> bit) & 1 == 1) | ||
| } | ||
| } | ||
|
|
||
| /// Unified base/gimbal state: normalised axes + button bitmap. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct BaseState { | ||
| /// Normalised axis values `[0.0, 1.0]`. Typically 5 axes (X, Y, Z, SZ, SL). | ||
| pub axes: Vec<f32>, | ||
| /// Raw button bytes (LSB-first per byte). | ||
| pub buttons_raw: Vec<u8>, | ||
| } | ||
|
|
||
| /// Unified throttle state: normalised axes + button bitmap. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct ThrottleState { | ||
| /// Normalised axis values `[0.0, 1.0]`. Typically 6 axes. | ||
| pub axes: Vec<f32>, | ||
| /// Raw button bytes (LSB-first per byte). | ||
| pub buttons_raw: Vec<u8>, | ||
| } | ||
|
|
||
| // ─── Unified parse functions ────────────────────────────────────────────────── | ||
|
|
||
| const GRIP_AXIS_COUNT: usize = 5; | ||
| const GRIP_BUTTON_BYTES: usize = 4; | ||
| const GRIP_MIN_BYTES: usize = 1 + GRIP_AXIS_COUNT * 2 + GRIP_BUTTON_BYTES; // 15 | ||
|
|
||
| const BASE_AXIS_COUNT: usize = 5; | ||
| const BASE_BUTTON_BYTES: usize = 4; | ||
| const BASE_MIN_BYTES: usize = 1 + BASE_AXIS_COUNT * 2 + BASE_BUTTON_BYTES; // 15 | ||
|
|
||
| const THROTTLE_AXIS_COUNT: usize = 6; | ||
| const THROTTLE_BUTTON_BYTES: usize = 10; | ||
| const THROTTLE_MIN_BYTES: usize = 1 + THROTTLE_AXIS_COUNT * 2 + THROTTLE_BUTTON_BYTES; // 23 | ||
|
|
||
| /// Parse a generic grip/stick HID report into [`GripState`]. | ||
| /// | ||
| /// Compatible with Alpha, Alpha Prime, MongoosT-50CM3 grips (15-byte reports). | ||
| pub fn parse_grip_report(data: &[u8]) -> Result<GripState, VirpilParseError> { | ||
| if data.len() < GRIP_MIN_BYTES { | ||
| return Err(VirpilParseError::TooShort(data.len())); | ||
| } | ||
| let payload = &data[1..]; | ||
| let mut axes = Vec::with_capacity(GRIP_AXIS_COUNT); | ||
| for i in 0..GRIP_AXIS_COUNT { | ||
| let raw = u16::from_le_bytes([payload[i * 2], payload[i * 2 + 1]]); | ||
| axes.push(normalize_axis(raw)); | ||
| } | ||
| let btn_start = 1 + GRIP_AXIS_COUNT * 2; | ||
| let buttons_raw = data[btn_start..btn_start + GRIP_BUTTON_BYTES].to_vec(); | ||
| Ok(GripState { axes, buttons_raw }) | ||
| } | ||
|
|
||
| /// Parse a generic base/gimbal HID report into [`BaseState`]. | ||
| /// | ||
| /// Compatible with WarBRD, WarBRD-D bases (15-byte reports, same format as grips). | ||
| pub fn parse_base_report(data: &[u8]) -> Result<BaseState, VirpilParseError> { | ||
| if data.len() < BASE_MIN_BYTES { | ||
| return Err(VirpilParseError::TooShort(data.len())); | ||
| } | ||
| let payload = &data[1..]; | ||
| let mut axes = Vec::with_capacity(BASE_AXIS_COUNT); | ||
| for i in 0..BASE_AXIS_COUNT { | ||
| let raw = u16::from_le_bytes([payload[i * 2], payload[i * 2 + 1]]); | ||
| axes.push(normalize_axis(raw)); | ||
| } | ||
| let btn_start = 1 + BASE_AXIS_COUNT * 2; | ||
| let buttons_raw = data[btn_start..btn_start + BASE_BUTTON_BYTES].to_vec(); | ||
| Ok(BaseState { axes, buttons_raw }) | ||
| } | ||
|
|
||
| /// Parse a generic throttle HID report into [`ThrottleState`]. | ||
| /// | ||
| /// Compatible with VPC Throttle CM3 (23-byte reports). | ||
| pub fn parse_throttle_report(data: &[u8]) -> Result<ThrottleState, VirpilParseError> { | ||
| if data.len() < THROTTLE_MIN_BYTES { | ||
| return Err(VirpilParseError::TooShort(data.len())); | ||
| } | ||
| let payload = &data[1..]; | ||
| let mut axes = Vec::with_capacity(THROTTLE_AXIS_COUNT); | ||
| for i in 0..THROTTLE_AXIS_COUNT { | ||
| let raw = u16::from_le_bytes([payload[i * 2], payload[i * 2 + 1]]); | ||
| axes.push(normalize_axis(raw)); | ||
| } | ||
| let btn_start = 1 + THROTTLE_AXIS_COUNT * 2; | ||
| let buttons_raw = data[btn_start..btn_start + THROTTLE_BUTTON_BYTES].to_vec(); | ||
| Ok(ThrottleState { axes, buttons_raw }) |
There was a problem hiding this comment.
The unified parse_*_report functions allocate (Vec::with_capacity + to_vec) on every parse. Existing device-specific parsers in this crate generally avoid heap allocation by using fixed-size arrays in Copy state structs. If these unified parsers are used in a per-frame/per-tick input path, consider switching axes/buttons_raw to fixed-size arrays (or returning slices into the input buffer) to avoid repeated allocations and copies.
Comment on lines
+141
to
+153
| pub fn reverse(&self, button_id: u8) -> Option<(u8, u8, u8)> { | ||
| if button_id == 0 { | ||
| return None; | ||
| } | ||
| let id = button_id as u16 - 1; | ||
| let layer = (id / self.buttons_per_layer) as u8; | ||
| if layer >= self.shift_layers { | ||
| return None; | ||
| } | ||
| let within_layer = (id % self.buttons_per_layer) as u8; | ||
| let row = within_layer / self.cols; | ||
| let col = within_layer % self.cols; | ||
| Some((row, col, layer)) |
There was a problem hiding this comment.
ButtonMatrix::reverse can panic due to division by zero if rows == 0 or cols == 0 (then buttons_per_layer == 0 and/or self.cols == 0). Since ButtonMatrix::new currently allows 0 for rows/cols and validate() doesn’t reject it, this is reachable from public API. Enforce rows >= 1 and cols >= 1 at construction (prefer returning Result<Self, ButtonMatrixError>), and/or make validate() fail for zero dimensions before reverse() can be used.
Comment on lines
+59
to
+79
| /// Create a new button matrix. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `rows` — number of physical rows in the matrix (must be ≥ 1). | ||
| /// * `cols` — number of physical columns in the matrix (must be ≥ 1). | ||
| /// * `shift_layers` — total number of layers including base (1–4). | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns [`ButtonMatrixError::DimensionsTooLarge`] if the total button | ||
| /// count (`rows × cols × layers`) exceeds 255. | ||
| pub fn new(rows: u8, cols: u8, shift_layers: u8) -> Self { | ||
| let layers = shift_layers.clamp(1, MAX_SHIFT_LAYERS); | ||
| let buttons_per_layer = rows as u16 * cols as u16; | ||
| Self { | ||
| rows, | ||
| cols, | ||
| shift_layers: layers, | ||
| buttons_per_layer, | ||
| } |
There was a problem hiding this comment.
The docs for ButtonMatrix::new describe error cases (and ButtonMatrixError includes dimension errors), but new always returns Self and silently clamps shift_layers. This makes it easy to create an invalid matrix (e.g., dimensions too large or zero-sized) without noticing. Consider changing new to return Result<Self, ButtonMatrixError> (or updating the docs to match the current behavior and requiring callers to call validate() explicitly).
Comment on lines
+21
to
+24
| //! let matrix = ButtonMatrix::new(8, 16, 2); | ||
| //! assert_eq!(matrix.resolve(0, 0), Some(1)); | ||
| //! assert_eq!(matrix.resolve(0, 1), Some(2)); | ||
| //! assert_eq!(matrix.resolve_shifted(0, 0, 1), Some(129)); |
There was a problem hiding this comment.
The module-level example constructs ButtonMatrix::new(8, 16, 2), but that configuration implies 256 logical buttons, which exceeds the u8 ID space and would fail validate() (and resolve_shifted will return None for some positions in the second layer). Please adjust the example to use dimensions that fit within the supported range, or update the API to support u16 button IDs if 256+ is intended.
Suggested change
| //! let matrix = ButtonMatrix::new(8, 16, 2); | |
| //! assert_eq!(matrix.resolve(0, 0), Some(1)); | |
| //! assert_eq!(matrix.resolve(0, 1), Some(2)); | |
| //! assert_eq!(matrix.resolve_shifted(0, 0, 1), Some(129)); | |
| //! let matrix = ButtonMatrix::new(8, 15, 2); | |
| //! assert_eq!(matrix.resolve(0, 0), Some(1)); | |
| //! assert_eq!(matrix.resolve(0, 1), Some(2)); | |
| //! assert_eq!(matrix.resolve_shifted(0, 0, 1), Some(121)); |
- ButtonMatrix::new() now returns Result and rejects zero rows/cols - Fix module doc example: use 8x15x2 to avoid u8 overflow - Fix apply_curve docs to match clamping behavior - Move thiserror import to top of protocol.rs - Replace Vec with fixed-size arrays in parsed report structs 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
Implements Virpil device protocol support for OpenFlight.
Devices Covered
Changes
Tests
264 tests passing: 234 unit + 25 property + 4 snapshot + 1 doctest