fix: use correct VID/PID when emulate_elite is disabled#18
Merged
BANANASJIM merged 1 commit intomainfrom Mar 8, 2026
Merged
Conversation
Pass emulate_elite flag to Uinput::create() so the virtual gamepad uses standard VID/PID (0x37d7:0x2401) instead of Xbox Elite (0x045e:0x0b00) when emulate_elite = false. Closes #16
📝 WalkthroughWalkthroughThis PR implements the ability to conditionally emulate the Xbox Elite Controller by adding an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tools/test_uinput_elite.cpp (1)
17-44: This still doesn't exercise the bug-fixed path.These checks stop at
Config::load(), so a regression insrc/gamepad.cppLine 219 orsrc/uinput.cppLines 135-136 would still pass. Consider extracting VID/PID selection into a small pure helper and asserting that helper for default/true/false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/test_uinput_elite.cpp` around lines 17 - 44, The tests currently only call Config::load() and don't exercise the VID/PID selection logic in the gamepad/uinput code; extract the selection logic into a pure helper (e.g., select_elite_vid_pid or choose_elite_device) that takes the Config or the emulate_elite flag and returns the VID/PID tuple, update gamepad.cpp/uinput.cpp to call that helper, and modify the tests in test_uinput_elite.cpp to call the new helper directly and assert the returned VID/PID for the default, true, and false cases so the bug-fixed path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 70-77: The test binary test-uinput-elite fails when run from the
build dir because src/tools/test_uinput_elite.cpp expects fixtures at a
source-path; define a compile-time macro pointing to the repository fixture
directory and expose it to that target so the test can build paths from
VADER5_TEST_CONFIG_DIR. Add a CMake directive for the test target
(test-uinput-elite) that provides VADER5_TEST_CONFIG_DIR (e.g., via
target_compile_definitions or by configuring a header) set to the source fixture
directory containing config/test-elite-*.toml, so
src/tools/test_uinput_elite.cpp can reliably locate fixtures at runtime.
In `@include/vader5/uinput.hpp`:
- Around line 22-24: The public factory signature Uinput::create(std::span<const
std::optional<int>> ext_mappings, bool emulate_elite = true, const char* name =
"Vader 5 Pro Virtual Gamepad") is error-prone because callers like
Uinput::create(mappings, "Custom Name") will pass the string into the bool
parameter; change the API to keep the name argument before emulate_elite or add
an overload that takes (ext_mappings, const char* name) to preserve
backward-friendly usage. Update the declaration in include/vader5/uinput.hpp and
the implementation in src/uinput.cpp to use the new parameter order
(ext_mappings, const char* name, bool emulate_elite = true) or add the overload,
and then update all call sites to match the new ordering or overload to avoid
accidental boolean conversion of string literals.
---
Nitpick comments:
In `@src/tools/test_uinput_elite.cpp`:
- Around line 17-44: The tests currently only call Config::load() and don't
exercise the VID/PID selection logic in the gamepad/uinput code; extract the
selection logic into a pure helper (e.g., select_elite_vid_pid or
choose_elite_device) that takes the Config or the emulate_elite flag and returns
the VID/PID tuple, update gamepad.cpp/uinput.cpp to call that helper, and modify
the tests in test_uinput_elite.cpp to call the new helper directly and assert
the returned VID/PID for the default, true, and false cases so the bug-fixed
path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c867c85-5c39-4a3d-b9e3-e40206a8cd9d
📒 Files selected for processing (8)
CMakeLists.txtconfig/test-elite-default.tomlconfig/test-elite-false.tomlconfig/test-elite-true.tomlinclude/vader5/uinput.hppsrc/gamepad.cppsrc/tools/test_uinput_elite.cppsrc/uinput.cpp
Comment on lines
+70
to
+77
| add_executable(test-uinput-elite | ||
| src/tools/test_uinput_elite.cpp | ||
| src/config.cpp | ||
| src/keycodes.cpp | ||
| ) | ||
| set_target_properties(test-uinput-elite PROPERTIES CXX_CLANG_TIDY "") | ||
| target_include_directories(test-uinput-elite PRIVATE include) | ||
| target_link_libraries(test-uinput-elite PRIVATE tomlplusplus::tomlplusplus) |
There was a problem hiding this comment.
Make the fixture directory explicit for test-uinput-elite.
src/tools/test_uinput_elite.cpp loads config/test-elite-*.toml with relative paths, but this target doesn't copy those files or pass the source config dir. Running the binary from the build directory will fail before any assertions.
🧪 One way to keep fixture lookup stable
add_executable(test-uinput-elite
src/tools/test_uinput_elite.cpp
src/config.cpp
src/keycodes.cpp
)
set_target_properties(test-uinput-elite PROPERTIES CXX_CLANG_TIDY "")
target_include_directories(test-uinput-elite PRIVATE include)
+target_compile_definitions(test-uinput-elite PRIVATE
+ VADER5_TEST_CONFIG_DIR="${CMAKE_SOURCE_DIR}/config"
+)
target_link_libraries(test-uinput-elite PRIVATE tomlplusplus::tomlplusplus)Then build the paths from VADER5_TEST_CONFIG_DIR inside src/tools/test_uinput_elite.cpp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 70 - 77, The test binary test-uinput-elite fails
when run from the build dir because src/tools/test_uinput_elite.cpp expects
fixtures at a source-path; define a compile-time macro pointing to the
repository fixture directory and expose it to that target so the test can build
paths from VADER5_TEST_CONFIG_DIR. Add a CMake directive for the test target
(test-uinput-elite) that provides VADER5_TEST_CONFIG_DIR (e.g., via
target_compile_definitions or by configuring a header) set to the source fixture
directory containing config/test-elite-*.toml, so
src/tools/test_uinput_elite.cpp can reliably locate fixtures at runtime.
Comment on lines
22
to
24
| static auto create(std::span<const std::optional<int>> ext_mappings, | ||
| bool emulate_elite = true, | ||
| const char* name = "Vader 5 Pro Virtual Gamepad") -> Result<Uinput>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List every Uinput::create call site for manual inspection.
rg -n -C2 'Uinput::create\s*\('Repository: BANANASJIM/flydigi-vader5
Length of output: 622
🏁 Script executed:
# Search more broadly for Uinput::create references and the implementation details
rg -n 'Uinput::create' --type cpp --type hppRepository: BANANASJIM/flydigi-vader5
Length of output: 96
🏁 Script executed:
# Show lines around the implementation in uinput.cpp
sed -n '60,70p' src/uinput.cppRepository: BANANASJIM/flydigi-vader5
Length of output: 470
🏁 Script executed:
# Search more broadly without file type specification
rg 'Uinput::create' -nRepository: BANANASJIM/flydigi-vader5
Length of output: 247
🏁 Script executed:
# Check for any other references to Uinput in headers
rg 'Uinput' include/vader5/uinput.hpp -n | head -20Repository: BANANASJIM/flydigi-vader5
Length of output: 486
🏁 Script executed:
# Verify no other build/test files reference Uinput::create
find . -name "*.cpp" -o -name "*.hpp" -o -name "*.h" | xargs rg -l 'Uinput' 2>/dev/null | head -20Repository: BANANASJIM/flydigi-vader5
Length of output: 160
🏁 Script executed:
# Verify the call site in gamepad.cpp and nearby context to ensure compatibility
sed -n '215,225p' src/gamepad.cppRepository: BANANASJIM/flydigi-vader5
Length of output: 435
Keep the existing name slot ahead of emulate_elite or add an overload.
Line 23 makes Uinput::create(mappings, "Custom Name") silently wrong: the string converts to true, and name falls back to the default. That's an easy API trap for any future caller.
♻️ Safer signature
- static auto create(std::span<const std::optional<int>> ext_mappings,
- bool emulate_elite = true,
- const char* name = "Vader 5 Pro Virtual Gamepad") -> Result<Uinput>;
+ static auto create(std::span<const std::optional<int>> ext_mappings,
+ const char* name = "Vader 5 Pro Virtual Gamepad",
+ bool emulate_elite = true) -> Result<Uinput>;Update src/uinput.cpp and any call sites to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/vader5/uinput.hpp` around lines 22 - 24, The public factory signature
Uinput::create(std::span<const std::optional<int>> ext_mappings, bool
emulate_elite = true, const char* name = "Vader 5 Pro Virtual Gamepad") is
error-prone because callers like Uinput::create(mappings, "Custom Name") will
pass the string into the bool parameter; change the API to keep the name
argument before emulate_elite or add an overload that takes (ext_mappings, const
char* name) to preserve backward-friendly usage. Update the declaration in
include/vader5/uinput.hpp and the implementation in src/uinput.cpp to use the
new parameter order (ext_mappings, const char* name, bool emulate_elite = true)
or add the overload, and then update all call sites to match the new ordering or
overload to avoid accidental boolean conversion of string literals.
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
1 participant
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
emulate_eliteflag toUinput::create()so the virtual gamepad uses standard VID/PID (0x37d7:0x2401) instead of Xbox Elite (0x045e:0x0b00) whenemulate_elite = falseChanges
include/vader5/uinput.hpp: Addbool emulate_eliteparameter toUinput::create()src/uinput.cpp: Select VID/PID based onemulate_eliteflagsrc/gamepad.cpp: Passcfg.emulate_elitetoUinput::create()src/tools/test_uinput_elite.cpp: Test suite for config parsing of emulate_eliteconfig/test-elite-{default,true,false}.toml: Test configsTest plan
test-uinput-elitepasses — verifies config parsing for all 3 cases (absent/true/false)test-remappasses — no regressionsemulate_elite = false, verify Steam no longer shows Xbox EliteCloses #16
Summary by CodeRabbit
New Features
Tests