fix: auto-detect hidraw input interface in debug tool - #19
Conversation
|
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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds runtime interface configuration for the debug tool, introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
Remove hardcoded IFACE_INPUT=0 and IFACE_CONFIG=1 constants.
Add auto-detection that probes interfaces {0, 2, 3} for valid input
reports, with --input and --config CLI overrides.
a3287f9 to
5574d21
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tools/test_debug_iface.cpp (1)
6-6: Consider using a testing framework or returning status codes.The
CHECKmacro usingstd::exit(1)works but makes it impossible to test error paths (like invalid values) sinceDebugOptions::parsealso callsstd::exit(1).For a small utility test this is acceptable, but if tests grow, consider a framework that can capture exit calls or refactoring
parseto return an error type instead of exiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/test_debug_iface.cpp` at line 6, The test harness currently hard-exits via the CHECK macro and DebugOptions::parse, preventing testing of error paths; refactor so parse returns an error status (e.g., bool/enum/std::optional/Expected) instead of calling std::exit(1), update all callers to handle and assert on that return, and change the CHECK macro in test_debug_iface.cpp to record failures (set a local failure flag or return a non-zero status) instead of calling std::exit so multiple assertions and invalid-input cases can be exercised; alternatively, replace the custom harness with a lightweight test framework (e.g., Catch2/GoogleTest) to capture failures and exit behavior.include/vader5/debug_options.hpp (1)
23-26: Consider printing the actual invalid value in error message.The error message shows the flag name but not the invalid value, which would help users debug their command.
📝 Improved error message
- } catch (const std::exception&) { - std::cerr << "Invalid value for " << arg << "\n"; + } catch (const std::exception& e) { + std::cerr << "Invalid value for " << arg << ": " << argv[i] << "\n"; std::exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/vader5/debug_options.hpp` around lines 23 - 26, The catch block catching std::exception in include/vader5/debug_options.hpp currently logs only the flag name (arg); update the catch to capture the exception (e.g., catch (const std::exception& e)) and augment the error message to include the actual invalid token/value that was being parsed (the variable holding the parsed value) along with the flag name and the exception message (e.what()) so the output reads something like "Invalid value for <arg>: '<value>' - <exception message>" to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/vader5/debug_options.hpp`:
- Around line 18-22: The argument-parsing branch that handles "--input" and
"--config" silently ignores these flags when they are last because it checks "i
+ 1 < argc" and does nothing on failure; update the parsing logic around the
loop over argv (use the same argv/argc and loop variable i) to detect the
missing value case and report a clear error (e.g., print an error message and
exit or throw) when "--input" or "--config" is provided without a following
value instead of silently skipping, ensuring you still assign to
opts.input_iface and opts.config_iface when a value exists.
---
Nitpick comments:
In `@include/vader5/debug_options.hpp`:
- Around line 23-26: The catch block catching std::exception in
include/vader5/debug_options.hpp currently logs only the flag name (arg); update
the catch to capture the exception (e.g., catch (const std::exception& e)) and
augment the error message to include the actual invalid token/value that was
being parsed (the variable holding the parsed value) along with the flag name
and the exception message (e.what()) so the output reads something like "Invalid
value for <arg>: '<value>' - <exception message>" to aid debugging.
In `@src/tools/test_debug_iface.cpp`:
- Line 6: The test harness currently hard-exits via the CHECK macro and
DebugOptions::parse, preventing testing of error paths; refactor so parse
returns an error status (e.g., bool/enum/std::optional/Expected) instead of
calling std::exit(1), update all callers to handle and assert on that return,
and change the CHECK macro in test_debug_iface.cpp to record failures (set a
local failure flag or return a non-zero status) instead of calling std::exit so
multiple assertions and invalid-input cases can be exercised; alternatively,
replace the custom harness with a lightweight test framework (e.g.,
Catch2/GoogleTest) to capture failures and exit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cf0d953-0937-4f62-98c7-37417255aa83
📒 Files selected for processing (4)
CMakeLists.txtinclude/vader5/debug_options.hppsrc/tools/debug.cppsrc/tools/test_debug_iface.cpp
| if (arg == "--input" && i + 1 < argc) { | ||
| opts.input_iface = std::stoi(argv[++i]); | ||
| } else if (arg == "--config" && i + 1 < argc) { | ||
| opts.config_iface = std::stoi(argv[++i]); | ||
| } |
There was a problem hiding this comment.
Missing value for --input/--config is silently ignored.
If a user passes --input or --config as the last argument without a value, the condition i + 1 < argc fails silently and the flag is ignored. This could confuse users who misspell or forget the value.
🛠️ Proposed fix to report an error for missing values
try {
if (arg == "--input" && i + 1 < argc) {
opts.input_iface = std::stoi(argv[++i]);
+ } else if (arg == "--input") {
+ std::cerr << "Error: --input requires a value\n";
+ std::exit(1);
} else if (arg == "--config" && i + 1 < argc) {
opts.config_iface = std::stoi(argv[++i]);
+ } else if (arg == "--config") {
+ std::cerr << "Error: --config requires a value\n";
+ std::exit(1);
}
} catch (const std::exception&) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (arg == "--input" && i + 1 < argc) { | |
| opts.input_iface = std::stoi(argv[++i]); | |
| } else if (arg == "--config" && i + 1 < argc) { | |
| opts.config_iface = std::stoi(argv[++i]); | |
| } | |
| if (arg == "--input" && i + 1 < argc) { | |
| opts.input_iface = std::stoi(argv[++i]); | |
| } else if (arg == "--input") { | |
| std::cerr << "Error: --input requires a value\n"; | |
| std::exit(1); | |
| } else if (arg == "--config" && i + 1 < argc) { | |
| opts.config_iface = std::stoi(argv[++i]); | |
| } else if (arg == "--config") { | |
| std::cerr << "Error: --config requires a value\n"; | |
| std::exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/vader5/debug_options.hpp` around lines 18 - 22, The argument-parsing
branch that handles "--input" and "--config" silently ignores these flags when
they are last because it checks "i + 1 < argc" and does nothing on failure;
update the parsing logic around the loop over argv (use the same argv/argc and
loop variable i) to detect the missing value case and report a clear error
(e.g., print an error message and exit or throw) when "--input" or "--config" is
provided without a following value instead of silently skipping, ensuring you
still assign to opts.input_iface and opts.config_iface when a value exists.
1 participant
Summary
--input Nand--config NCLI args for manual overrideChanges
include/vader5/debug_options.hpp: CLI argument parsing with input validationsrc/tools/debug.cpp: Replace hardcodedIFACE_INPUT=0with auto-detection; probe interfaces 0,2,3 for valid HID reportssrc/tools/test_debug_iface.cpp: Test suite for CLI argument parsingCMakeLists.txt: Add test targetTest plan
test-debug-ifacepasses — verifies CLI parsing (defaults, overrides, unknown args)vader5-debugwith a controller on a non-standard interfacevader5-debug --input 2to verify CLI overrideCloses #17
Summary by CodeRabbit
Release Notes
New Features
--input,--config) for explicit interface selection.Tests