feat(hotas-winwing): add protocol parser, LED control, and detent system#66
Open
EffortlessSteven wants to merge 3 commits intomainfrom
Open
feat(hotas-winwing): add protocol parser, LED control, and detent system#66EffortlessSteven wants to merge 3 commits intomainfrom
EffortlessSteven wants to merge 3 commits intomainfrom
Conversation
- Winwing USB HID protocol device detection for Orion, Super Taurus, F18/F16 panels - LED and display control for panel backlight and UFC - Throttle detent handling with magnetic positions - 260 tests passing covering all device families 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 WinWing device detection, LED control, and detent system
WalkthroughsDescription• Add device type detection and protocol handler for 9 WinWing device models • Implement LED and display control with batch report building API • Add magnetic detent system for throttle snap-to-position behavior • Export new public APIs and update module structure Diagramflowchart LR
A["USB PID"] -->|DeviceType::from_pid| B["DeviceType enum"]
B -->|WinwingProtocol| C["Device capabilities"]
C -->|has_leds/has_display/has_detents| D["Feature queries"]
E["LedCommand/DisplayCommand"] -->|build_led_report| F["Feature report bytes"]
E -->|build_display_report| F
G["WinwingDetentConfig"] -->|detect_detent| H["ActiveDetent result"]
H -->|magnetic snap| I["Snapped axis value"]
File Changes1. crates/flight-hotas-winwing/src/detent_system.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
+539
to
+596
| /// F-18 UFC / IFEI panel (button-only, display segments). | ||
| F18Panel, | ||
| /// F-16EX ICP panel (buttons, switches, 2 axes). | ||
| F16Panel, | ||
| /// TFRP rudder pedals (3 axes). | ||
| TfrpRudder, | ||
| /// Skywalker metal rudder pedals. | ||
| SkywalkerRudder, | ||
| } | ||
|
|
||
| impl DeviceType { | ||
| /// Detect the device type from a USB Product ID. | ||
| /// | ||
| /// Returns `None` if the PID is not recognised. | ||
| pub fn from_pid(pid: u16) -> Option<Self> { | ||
| match pid { | ||
| 0xBE60 => Some(Self::OrionStick), | ||
| 0xBE61 => Some(Self::OrionThrottle), | ||
| 0xBE63 => Some(Self::Orion2Stick), | ||
| 0xBE62 => Some(Self::Orion2Throttle), | ||
| 0xBD64 => Some(Self::SuperTaurus), | ||
| 0xBEDE => Some(Self::F18Panel), | ||
| 0xBEA8 => Some(Self::F16Panel), | ||
| 0xBE64 => Some(Self::TfrpRudder), | ||
| 0xBEF0 => Some(Self::SkywalkerRudder), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// Returns the expected HID input report length for this device. | ||
| pub fn report_length(&self) -> usize { | ||
| match self { | ||
| Self::OrionStick | Self::Orion2Stick => 12, | ||
| Self::OrionThrottle | Self::Orion2Throttle => 24, | ||
| Self::SuperTaurus => 13, | ||
| Self::F18Panel => 6, | ||
| Self::F16Panel => 10, | ||
| Self::TfrpRudder => 8, | ||
| Self::SkywalkerRudder => 8, | ||
| } | ||
| } | ||
|
|
||
| /// Returns `true` if this device supports LED / backlight control. | ||
| pub fn has_leds(&self) -> bool { | ||
| matches!( | ||
| self, | ||
| Self::OrionThrottle | ||
| | Self::Orion2Throttle | ||
| | Self::SuperTaurus | ||
| | Self::F18Panel | ||
| | Self::F16Panel | ||
| ) | ||
| } | ||
|
|
||
| /// Returns `true` if this device has a display (UFC/IFEI/ICP). | ||
| pub fn has_display(&self) -> bool { | ||
| matches!(self, Self::F18Panel | Self::F16Panel) | ||
| } |
There was a problem hiding this comment.
1. Pid 0xbea8 inconsistent 🐞 Bug ✓ Correctness
DeviceType::from_pid maps PID 0xBEA8 to F16Panel and advertises display/LED capabilities, but the rest of the crate documents and parses 0xBEA8 as the F-16EX Grip. This inconsistency will make WinwingProtocol report wrong device name/capabilities for the existing supported 0xBEA8 device, leading callers to attempt panel-only operations (display/backlight) on the grip.
Agent Prompt
### Issue description
`DeviceType::from_pid(0xBEA8)` currently returns `F16Panel` and reports `has_display()`/`has_leds()` as true, but the rest of the crate (docs, parser module, profiles) treats PID 0xBEA8 as the F-16EX Grip.
### Issue Context
This inconsistency will cause callers using `WinwingProtocol` for a 0xBEA8 device to get contradictory identity/capability information compared to the crate’s existing parsing/profile behavior.
### Fix Focus Areas
- crates/flight-hotas-winwing/src/protocol.rs[523-596]
- crates/flight-hotas-winwing/src/lib.rs[4-21]
- crates/flight-hotas-winwing/src/f16ex_stick.rs[4-23]
- crates/flight-hotas-winwing/src/profiles.rs[254-265]
ⓘ 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 higher-level WinWing device support to flight-hotas-winwing by introducing PID-based device identification, batch-oriented LED/display feature-report builders, and a magnetic detent “snap” model for throttles.
Changes:
- Add
DeviceType+WinwingProtocolfor PID-based identification and capability querying. - Introduce
led_controlbatch APIs to build backlight/display feature reports from high-level commands. - Add
detent_systemwith configurable magnetic detents and detection/snap behavior (with presets + tests).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| crates/flight-hotas-winwing/src/protocol.rs | Adds device-type detection/capabilities and a protocol wrapper; extends tests. |
| crates/flight-hotas-winwing/src/lib.rs | Wires new modules and re-exports new detent/LED/protocol APIs. |
| crates/flight-hotas-winwing/src/led_control.rs | New batch-oriented LED/display command API + tests. |
| crates/flight-hotas-winwing/src/detent_system.rs | New magnetic detent configuration/detection logic + presets + tests. |
Comment on lines
+82
to
+84
| pub struct WinwingDetentConfig { | ||
| detents: Vec<MagneticDetent>, | ||
| } |
There was a problem hiding this comment.
WinwingDetentConfig naming is inconsistent with the crate’s existing WinWing* public types. Consider renaming to WinWingDetentConfig (and updating exports) to keep the public API consistent.
Comment on lines
+44
to
+57
| pub fn contains(&self, value: f32) -> bool { | ||
| (value - self.centre).abs() <= self.snap_radius | ||
| } | ||
|
|
||
| /// Apply the magnetic detent effect to `value`. | ||
| /// | ||
| /// If `value` is within the snap zone, it is pulled toward `centre` | ||
| /// by `strength`. If outside the zone, `value` is returned unchanged. | ||
| pub fn apply(&self, value: f32) -> f32 { | ||
| if !self.contains(value) { | ||
| return value; | ||
| } | ||
| let delta = self.centre - value; | ||
| value + delta * self.strength |
There was a problem hiding this comment.
MagneticDetent::contains will treat values outside the documented normalized range [0.0, 1.0] as inside the detent zone (e.g., negative values near an idle detent). That can lead to apply() producing out-of-range snapped values. Consider clamping value to [0.0, 1.0] (and possibly clamping the output of apply()), or updating the docs/tests to explicitly allow out-of-range inputs.
Suggested change
| pub fn contains(&self, value: f32) -> bool { | |
| (value - self.centre).abs() <= self.snap_radius | |
| } | |
| /// Apply the magnetic detent effect to `value`. | |
| /// | |
| /// If `value` is within the snap zone, it is pulled toward `centre` | |
| /// by `strength`. If outside the zone, `value` is returned unchanged. | |
| pub fn apply(&self, value: f32) -> f32 { | |
| if !self.contains(value) { | |
| return value; | |
| } | |
| let delta = self.centre - value; | |
| value + delta * self.strength | |
| /// | |
| /// The input is clamped to the normalised `[0.0, 1.0]` range before | |
| /// the containment check, so callers do not need to pre-clamp. | |
| pub fn contains(&self, value: f32) -> bool { | |
| let v = value.clamp(0.0, 1.0); | |
| (v - self.centre).abs() <= self.snap_radius | |
| } | |
| /// Apply the magnetic detent effect to `value`. | |
| /// | |
| /// If `value` is within the snap zone, it is pulled toward `centre` | |
| /// by `strength`. If outside the zone, the (clamped) input is | |
| /// returned unchanged. Both input and output are clamped to the | |
| /// normalised `[0.0, 1.0]` range. | |
| pub fn apply(&self, value: f32) -> f32 { | |
| let clamped_in = value.clamp(0.0, 1.0); | |
| if !self.contains(clamped_in) { | |
| return clamped_in; | |
| } | |
| let delta = self.centre - clamped_in; | |
| let snapped = clamped_in + delta * self.strength; | |
| snapped.clamp(0.0, 1.0) |
Comment on lines
+49
to
+55
| pub fn build_led_report(commands: &[LedCommand]) -> Result<Vec<Vec<u8>>, ProtocolError> { | ||
| let mut reports = Vec::with_capacity(commands.len()); | ||
| for cmd in commands { | ||
| let frame = match cmd { | ||
| LedCommand::SetIntensity { | ||
| panel_id, | ||
| button_index, |
There was a problem hiding this comment.
build_led_report re-encodes payload layouts that already exist as helpers in crate::protocol (e.g., build_backlight_single_command, build_backlight_all_rgb_command). Duplicating the encoding logic increases the chance of protocol drift; consider delegating to those existing builders and only handling the batch-to-Vec<Vec> conversion here.
Comment on lines
+131
to
+139
| let truncated = if text.len() > 16 { | ||
| &text[..16] | ||
| } else { | ||
| text.as_str() | ||
| }; | ||
| let mut payload = Vec::with_capacity(2 + truncated.len()); | ||
| payload.push(*panel_id); | ||
| payload.push(*field_index); | ||
| payload.extend_from_slice(truncated.as_bytes()); |
There was a problem hiding this comment.
&text[..16] slices by byte index on a String and will panic if text contains non-ASCII/multi-byte UTF-8 (even though the docs say ASCII). To avoid panics, truncate using text.as_bytes() (or validate text.is_ascii() and handle invalid input explicitly) rather than slicing the String by a hard-coded index.
Suggested change
| let truncated = if text.len() > 16 { | |
| &text[..16] | |
| } else { | |
| text.as_str() | |
| }; | |
| let mut payload = Vec::with_capacity(2 + truncated.len()); | |
| payload.push(*panel_id); | |
| payload.push(*field_index); | |
| payload.extend_from_slice(truncated.as_bytes()); | |
| let bytes = text.as_bytes(); | |
| let truncated_bytes = if bytes.len() > 16 { | |
| &bytes[..16] | |
| } else { | |
| bytes | |
| }; | |
| let mut payload = Vec::with_capacity(2 + truncated_bytes.len()); | |
| payload.push(*panel_id); | |
| payload.push(*field_index); | |
| payload.extend_from_slice(truncated_bytes); |
| // ── Device identification ───────────────────────────────────────────────────── | ||
|
|
||
| /// WinWing USB Vendor ID (shared across all products). | ||
| pub const WINWING_VID: u16 = 0x4098; |
There was a problem hiding this comment.
WINWING_VID is now defined in this module, but the crate already defines WINWING_VID in src/lib.rs and WINWING_VENDOR_ID in src/input.rs (all 0x4098). Having multiple public constants for the same VID is likely to drift; consider removing this one and re-exporting/using the single canonical constant instead.
Suggested change
| pub const WINWING_VID: u16 = 0x4098; | |
| pub use crate::WINWING_VID; |
Comment on lines
+559
to
+563
| 0xBD64 => Some(Self::SuperTaurus), | ||
| 0xBEDE => Some(Self::F18Panel), | ||
| 0xBEA8 => Some(Self::F16Panel), | ||
| 0xBE64 => Some(Self::TfrpRudder), | ||
| 0xBEF0 => Some(Self::SkywalkerRudder), |
There was a problem hiding this comment.
PID 0xBEA8 is mapped to DeviceType::F16Panel, and has_display() returns true for that type. Elsewhere in this crate 0xBEA8 is documented/parsed as the F-16EX Grip (10-byte report, no display), so this mapping/capability set appears incorrect and could misroute device handling. Consider renaming the variant and/or adjusting the PID mapping and has_display/has_leds capability flags to match the existing f16ex_stick device.
Comment on lines
+628
to
+633
| pub struct WinwingProtocol { | ||
| device: DeviceType, | ||
| pid: u16, | ||
| } | ||
|
|
||
| impl WinwingProtocol { |
There was a problem hiding this comment.
Type name WinwingProtocol is inconsistent with the existing public API style in this crate (WinWingError, WinWingDevice, etc.). Consider renaming to WinWingProtocol for consistency before this becomes a stabilized public API.
Suggested change
| pub struct WinwingProtocol { | |
| device: DeviceType, | |
| pid: u16, | |
| } | |
| impl WinwingProtocol { | |
| pub struct WinWingProtocol { | |
| device: DeviceType, | |
| pid: u16, | |
| } | |
| impl WinWingProtocol { |
- Fix PID 0xBEA8 mapping from F16Panel to F16ExGrip - Clamp MagneticDetent inputs to [0.0, 1.0] range - Fix text slicing panic on non-ASCII via as_bytes() - Rename WinwingDetentConfig → WinWingDetentConfig - Rename WinwingProtocol → WinWingProtocol - Remove duplicate WINWING_VID constant - Delegate LED encoding to protocol builders Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 Winwing device protocol support for OpenFlight.
Devices Covered
Changes
Tests
260 tests passing — covers protocol parsing, LED commands, detent positions, and all existing device families.