psyqo Multi-tap driver and example update #1815
Conversation
|
I really didn't know what sort of access methods would make sense for the analog data fields here. I can scrap this and try again if this one's too painful to look at, otherwise, open to suggestions. |
|
Yeah no I was just trying to avoid doing too much during the holidays :P |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes enhance the Changes
Sequence DiagramsequenceDiagram
participant Pad as Pad Device
participant AdvancedPad as AdvancedPad Class
participant MultitapScene as Multitap Scene
Pad ->> AdvancedPad: Provide Pad Data
AdvancedPad ->> AdvancedPad: Process Pad Type
AdvancedPad ->> AdvancedPad: Extract Analog Inputs
AdvancedPad ->> MultitapScene: Return Pad Information
MultitapScene ->> MultitapScene: Display Pad Details
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/mips/psyqo/examples/multitap/multitap.cpp (2)
112-112: Maintain consistent usage for printing pad connection info.
While this change properly calls
136-153: Potential buffer overflow risk inprintPadStatus.
Usingsprintfcan risk overflowingm_textBuffer. Although the current usage appears safe due to the small size (32 bytes) and short output strings, consider replacingsprintfwithsnprintfto ensure safety if future modifications expand the printed text.- sprintf(m_textBuffer, "%02X ", input.getAdc(pad, i)); + snprintf(m_textBuffer, sizeof(m_textBuffer), "%02X ", input.getAdc(pad, i));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/psyqo/advancedpad.hh(1 hunks)src/mips/psyqo/examples/multitap/multitap.cpp(6 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/examples/multitap/multitap.cpp
[warning] 156-200: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
🔇 Additional comments (9)
src/mips/psyqo/examples/multitap/multitap.cpp (4)
35-35: Include statement seems fine.
No issues here regarding functionality or maintainability.
64-64: Method signature addition is appropriate.
The new method signature extends the class's functionality and reads coherently with the rest of the code.
215-215: Pointer notation for active pad is straightforward.
No concerns here, as it cleanly indicates which pad is selected.
227-228: Check for cohesive printing sequence.
These calls toprintPadStatusandprintPadTypework well together. Confirm that skipping either call when the pad isn't connected remains consistent with your UI flow; you may want to conditionally print the type only if connected.src/mips/psyqo/advancedpad.hh (5)
145-184: Convenient specialized getters for analog inputs.
These four methods (getAdc0,getAdc1,getAdc2,getAdc3) improve code readability. Validate thorough test coverage, especially for edge cases when no analog data is present.
185-207: Unified analog getter is well-designed.
ThegetAdcmethod elegantly delegates to the specialized getters and prevents boilerplate in the calling code.
209-221:getHalfwordis straightforward.
The method reduces code duplication. Make sure the modulo operation is the intended approach for indexing the 2D array, as out-of-range values might silently wrap.
223-232:getPadTypematches usage in multitap example.
It cleanly extracts the higher byte for the pad type. Confirm correctness of shifting logic (e.g.,>> 8) for all supported devices.
234-247: MovingCommandto private is good encapsulation.
Hiding the enum is beneficial unless external code requires direct access. If future external usage is anticipated, consider making it protected or adding proper accessor methods.
| void MultitapTestScene::printPadType(psyqo::AdvancedPad::Pad pad, int column, const char *name) { | ||
| auto &input = multitapTest.m_input; | ||
| char m_textBuffer[16] = {'\0'}; | ||
| const auto padType = input.getPadType(pad); | ||
|
|
||
| print(column + 0, 11, true, name); | ||
| switch (padType) { | ||
| case psyqo::AdvancedPad::PadType::Mouse: | ||
| sprintf(m_textBuffer, "Mouse"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::NegCon: | ||
| sprintf(m_textBuffer, "NegCon"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::KonamiLightgun: | ||
| sprintf(m_textBuffer, "KonamiLightgun"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::DigitalPad: | ||
| sprintf(m_textBuffer, "DigitalPad"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::AnalogStick: | ||
| sprintf(m_textBuffer, "AnalogStick"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::NamcoLightgun: | ||
| sprintf(m_textBuffer, "NamcoLightgun"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::AnalogPad: | ||
| sprintf(m_textBuffer, "AnalogPad"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::Multitap: | ||
| sprintf(m_textBuffer, "Multitap"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::Jogcon: | ||
| sprintf(m_textBuffer, "Jogcon"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::ConfigMode: | ||
| sprintf(m_textBuffer, "ConfigMode"); | ||
| break; | ||
| case psyqo::AdvancedPad::PadType::None: | ||
| sprintf(m_textBuffer, "None"); | ||
| break; | ||
| default: | ||
| sprintf(m_textBuffer, "Unknown"); | ||
| break; | ||
| } | ||
| print(column + 10, 11, true, m_textBuffer); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce cyclomatic complexity in printPadType.
This method’s complexity triggers a warning. As more pad types or logic are added, it can become unwieldy. A lookup table or map-based approach could drastically simplify this switch statement.
-switch (padType) {
// ...
- case psyqo::AdvancedPad::PadType::NegCon:
- sprintf(m_textBuffer, "NegCon");
- break;
...
-default:
- sprintf(m_textBuffer, "Unknown");
- break;
+static const struct {
+ psyqo::AdvancedPad::PadType type;
+ const char* name;
+} padTypeMap[] = {
+ { psyqo::AdvancedPad::PadType::Mouse, "Mouse" },
+ { psyqo::AdvancedPad::PadType::NegCon, "NegCon" },
+ // ...
+ { psyqo::AdvancedPad::PadType::None, "None" },
+};
+
+const auto it = std::find_if(
+ std::begin(padTypeMap), std::end(padTypeMap),
+ [padType](auto &entry) { return entry.type == padType; }
+);
+sprintf(m_textBuffer, "%s", (it != std::end(padTypeMap)) ? it->name : "Unknown");Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 156-200: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/mips/psyqo/advancedpad.hh (3)
145-184: Enhance documentation for ADC getter methods.The documentation for the ADC getter methods could be more helpful by including:
- Value ranges (e.g., 0-255)
- Typical use cases (e.g., analog stick position, pressure sensitivity)
- Mapping of values to physical controls for different pad types
185-207: Consider adding bounds checking for the index parameter.The method silently returns 0 for invalid indices, which could mask errors. Consider:
- Adding a precondition check for the index parameter
- Throwing an exception or returning an optional/error code for invalid indices
209-221: Add safety notes to getHalfword documentation.Since this method provides raw access to pad data, the documentation should:
- Warn about the low-level nature of the method
- Explain the data format for each index
- Recommend using the higher-level methods when possible
src/mips/psyqo/examples/multitap/multitap.cpp (2)
101-111: Consider moving color constants to class scope.The WHITE and GRAY color constants are defined as static within the method. Consider moving them to class scope to:
- Avoid recreation on each call
- Allow reuse in other methods
- Make them configurable if needed
113-122: LGTM! Consider moving pad names to class scope.The implementation is clean and efficient. Consider moving the padNames array to class scope to:
- Make it configurable
- Avoid recreation on each call
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/psyqo/advancedpad.hh(2 hunks)src/mips/psyqo/examples/multitap/multitap.cpp(6 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/examples/multitap/multitap.cpp
[warning] 163-206: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: toolchain
- GitHub Check: cross-arm64
🔇 Additional comments (3)
src/mips/psyqo/advancedpad.hh (1)
234-247: LGTM! Good encapsulation of implementation details.Moving the Command enum to private section is a good practice as these are internal implementation details.
src/mips/psyqo/examples/multitap/multitap.cpp (2)
163-206: Refactor suggestion from previous review still applies.The method's cyclomatic complexity is high. The previous review's suggestion to use a lookup table would improve maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 163-206: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
220-222: LGTM! Clean and well-organized implementation.The frame method effectively uses the new helper methods, providing a clear separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/psyqo/advancedpad.hh(2 hunks)src/mips/psyqo/examples/multitap/multitap.cpp(6 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/examples/multitap/multitap.cpp
[warning] 163-206: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: aur-build
- GitHub Check: asan
🔇 Additional comments (6)
src/mips/psyqo/advancedpad.hh (2)
145-185: Well-documented and efficiently implemented analog input getters!The implementation provides clear documentation about the meaning of values (e.g., "00h=Left, 80h=Center, FFh=Right") and uses efficient bit manipulation to extract the values.
242-255: Good encapsulation of low-level commands!Moving the Command enum to private section improves encapsulation by hiding implementation details.
src/mips/psyqo/examples/multitap/multitap.cpp (4)
105-112: Clean implementation of variadic print method!The implementation correctly handles variable arguments with proper va_start/va_end usage.
219-222: Clean and focused frame implementation!The method has a clear separation of concerns, delegating to specific print methods.
163-206: 🛠️ Refactor suggestionRefactor to reduce complexity and prevent potential issues.
The method has high cyclomatic complexity and potential safety issues:
- Large switch statement increases complexity
- padTypeStr could theoretically be used uninitialized if switch falls through
Consider using a static lookup table:
+ static const struct { + PadType type; + const char* name; + } padTypeMap[] = { + { PadType::Mouse, "Mouse" }, + { PadType::NegCon, "NegCon" }, + { PadType::KonamiLightgun, "KonamiLightgun" }, + { PadType::DigitalPad, "DigitalPad" }, + { PadType::AnalogStick, "AnalogStick" }, + { PadType::NamcoLightgun, "NamcoLightgun" }, + { PadType::AnalogPad, "AnalogPad" }, + { PadType::Multitap, "Multitap" }, + { PadType::Jogcon, "Jogcon" }, + { PadType::ConfigMode, "ConfigMode" }, + { PadType::None, "None" }, + }; + + const auto it = std::find_if(std::begin(padTypeMap), std::end(padTypeMap), + [padType](const auto& entry) { return entry.type == padType; }); + + const char* padTypeStr = (it != std::end(padTypeMap)) ? it->name : "Unknown";Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 163-206: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 13, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
125-161: Add bounds checking for adcBytes calculation.The calculation of adcBytes from halfWords lacks bounds checking. While unlikely, corrupted pad data could lead to buffer overflow.
Consider adding explicit bounds check:
- const auto adcBytes = (halfWords - 1) * 2; + const auto adcBytes = std::min((halfWords - 1) * 2, 8u); // Limit to maximum possible ADC bytes
src/mips/psyqo/advancedpad.hh
Outdated
| /** | ||
| * @brief Returns the state of an Analog Input. | ||
| * | ||
| * @details See the specific Analog Input functions for details. | ||
| * The index is modulo 4, so it will wrap around if it is greater than 3. | ||
| * | ||
| * @param pad The pad to query. | ||
| * @param index The index of the Analog Input. | ||
| * @return The state of the Analog Input as an unsigned 8-bit value(0-255). | ||
| */ | ||
| uint8_t getAdc(Pad pad, unsigned int index) const { | ||
| switch (index) { | ||
| case 0: | ||
| return getAdc0(pad); | ||
| case 1: | ||
| return getAdc1(pad); | ||
| case 2: | ||
| return getAdc2(pad); | ||
| case 3: | ||
| return getAdc3(pad); | ||
| default: | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix documentation or implementation mismatch for index wrapping.
The documentation states "The index is modulo 4, so it will wrap around if it is greater than 3", but the implementation returns 0 for indices > 3 instead of wrapping.
Consider one of these fixes:
- switch (index) {
+ switch (index % 4) {or update the documentation to reflect the current behavior:
- * The index is modulo 4, so it will wrap around if it is greater than 3.
+ * Indices greater than 3 will return 0.📝 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.
| /** | |
| * @brief Returns the state of an Analog Input. | |
| * | |
| * @details See the specific Analog Input functions for details. | |
| * The index is modulo 4, so it will wrap around if it is greater than 3. | |
| * | |
| * @param pad The pad to query. | |
| * @param index The index of the Analog Input. | |
| * @return The state of the Analog Input as an unsigned 8-bit value(0-255). | |
| */ | |
| uint8_t getAdc(Pad pad, unsigned int index) const { | |
| switch (index) { | |
| case 0: | |
| return getAdc0(pad); | |
| case 1: | |
| return getAdc1(pad); | |
| case 2: | |
| return getAdc2(pad); | |
| case 3: | |
| return getAdc3(pad); | |
| default: | |
| return 0; | |
| } | |
| } | |
| /** | |
| * @brief Returns the state of an Analog Input. | |
| * | |
| * @details See the specific Analog Input functions for details. | |
| * The index is modulo 4, so it will wrap around if it is greater than 3. | |
| * | |
| * @param pad The pad to query. | |
| * @param index The index of the Analog Input. | |
| * @return The state of the Analog Input as an unsigned 8-bit value(0-255). | |
| */ | |
| uint8_t getAdc(Pad pad, unsigned int index) const { | |
| switch (index % 4) { | |
| case 0: | |
| return getAdc0(pad); | |
| case 1: | |
| return getAdc1(pad); | |
| case 2: | |
| return getAdc2(pad); | |
| case 3: | |
| return getAdc3(pad); | |
| default: | |
| return 0; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/mips/psyqo/advancedpad.hh (1)
212-229:⚠️ Potential issueFix documentation or implementation mismatch for index wrapping.
The documentation states "The index is modulo 4, so it will wrap around if it is greater than 3", but the implementation returns undefined behavior for indices > 3 due to direct array access.
Consider adding bounds checking:
- uint16_t getHalfword(Pad pad, unsigned int index) const { return m_padData[pad][index % 4]; } + uint16_t getHalfword(Pad pad, unsigned int index) const { + const auto safeIndex = index % 4; + return m_padData[pad][safeIndex]; + }
🧹 Nitpick comments (2)
src/mips/psyqo/advancedpad.hh (2)
197-210: Consider using array indexing for better maintainability.The switch case could be simplified using array indexing, which would be more maintainable and potentially more efficient.
- uint8_t getAdc(Pad pad, unsigned int index) const { - switch (index) { - case 0: - return getAdc0(pad); - case 1: - return getAdc1(pad); - case 2: - return getAdc2(pad); - case 3: - return getAdc3(pad); - default: - return 0; - } - } + uint8_t getAdc(Pad pad, unsigned int index) const { + static constexpr auto getters[] = { + &AdvancedPad::getAdc0, + &AdvancedPad::getAdc1, + &AdvancedPad::getAdc2, + &AdvancedPad::getAdc3 + }; + return index < 4 ? (this->*getters[index])(pad) : 0; + }
231-239: Enhance documentation for getPadType method.The documentation could be more helpful by:
- Referencing the
PadTypeenum values that could be returned- Explaining how this relates to
isPadConnected- Noting any special cases or invalid states
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/advancedpad.hh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: coverage
🔇 Additional comments (2)
src/mips/psyqo/advancedpad.hh (2)
5-5: LGTM!Copyright year correctly updated to 2025.
242-255: LGTM! Good encapsulation of implementation details.Moving the
Commandenum to private scope is a good practice as these are internal implementation details that shouldn't be exposed to users of the class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mips/psyqo/advancedpad.hh (1)
147-187: Consider optimizing the ADC access methods.The implementation is correct and well-documented. However, consider these potential optimizations:
- The switch statement could be replaced with array indexing for better performance
- The individual getAdc0-3 methods could be implemented in terms of getAdc for better maintainability
- uint8_t getAdc(Pad pad, unsigned int index) const { - switch (index) { - case 0: - return getAdc0(pad); - case 1: - return getAdc1(pad); - case 2: - return getAdc2(pad); - case 3: - return getAdc3(pad); - default: - return 0; - } - } + uint8_t getAdc(Pad pad, unsigned int index) const { + static const auto getters[] = { + &AdvancedPad::getAdc0, + &AdvancedPad::getAdc1, + &AdvancedPad::getAdc2, + &AdvancedPad::getAdc3 + }; + return index < 4 ? (this->*getters[index])(pad) : 0; + }Also applies to: 189-212
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/psyqo/advancedpad.hh(4 hunks)src/mips/psyqo/src/advancedpad.cpp(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-openbios
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: aur-build
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: asan
- GitHub Check: build
- GitHub Check: toolchain
🔇 Additional comments (7)
src/mips/psyqo/src/advancedpad.cpp (2)
117-117: LGTM! Consistent enum class handling.All array accesses properly use
static_cast<unsigned>for the enum class values.Also applies to: 123-123, 127-128, 138-138
51-58: LGTM! Proper use of scoped enum.The pad processing correctly uses the new
Pad::scope for enum values.src/mips/psyqo/advancedpad.hh (5)
5-5: LGTM!Copyright year correctly updated to 2025.
45-45: Great improvements to type safety and encapsulation!
- Converting
Padto a scoped enum class with explicit unsigned type improves type safety- Moving
Commandenum to private section properly encapsulates implementation detailsAlso applies to: 246-259
131-131: LGTM!Methods correctly updated to handle the new enum class type using static_cast.
Also applies to: 143-145
233-243: LGTM!The getPadType method correctly extracts and returns the pad type from the raw data.
214-231: Verify the index wrapping behavior.The implementation now correctly matches the documentation using the modulo operator. However, let's verify that this change doesn't break any existing code.
✅ Verification successful
Index wrapping implementation verified as safe and correct
The modulo operator implementation matches the documented behavior exactly, and no existing code in the codebase uses this method, making this change completely safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to getHalfword with index parameter rg -p "getHalfword.*\([^,]+,\s*(\d+|[^)]+)\)" --type cppLength of output: 213
Script:
#!/bin/bash # Search for any derived classes ast-grep --pattern 'class $_ : $$$AdvancedPad$$$' # Search for test files fd -e cpp -e cc test fd -e cpp -e cc -e hh -e h mock # Search for any indirect usage patterns rg -p "getHalfword" --type cpp -A 2 -B 2Length of output: 3634
Script:
#!/bin/bash # Check the padtest example cat src/mips/psyqo/examples/padtest/padtest.cppLength of output: 6689
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mips/psyqo/advancedpad.hh (1)
205-205: Return Enum Type forgetPadTypeTo enhance type safety and clarity, consider returning
PadTypeinstead ofuint8_tingetPadType.Apply this diff to update the return type:
- uint8_t getPadType(Pad pad) const { return m_padData[static_cast<unsigned>(pad)][0] >> 8; } + PadType getPadType(Pad pad) const { return static_cast<PadType>(m_padData[static_cast<unsigned>(pad)][0] >> 8); }Ensure that all callers handle the
PadTypeenum appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/advancedpad.hh(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: asan
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: cross-arm64
- GitHub Check: coverage
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (6)
src/mips/psyqo/advancedpad.hh (6)
45-45: Improved Type Safety with Scoped EnumChanging
Padto a scopedenum classenhances type safety by preventing implicit conversions. This is a good practice.
131-131: Ensure Consistent Use ofstatic_castUpdating
isPadConnectedto usestatic_cast<unsigned>(pad)ensures compatibility with the newPadenum class. Good job maintaining consistency.
185-186: Clarify Documentation Regarding Index WrappingThe comment states, "The index is modulo 4, so it will wrap around if it is greater than 3." However, in
getAdc, indices greater than3return0instead of wrapping.Ensure the documentation accurately reflects the implementation in both methods for consistency.
208-221: Visibility Change ofCommandEnumMoving the
Commandenum to the private section restricts its access, which is appropriate if it's not intended for public use.
161-174: 🛠️ Refactor suggestionConsistency in Handling Invalid Indices in
getAdcThe
getAdcmethod returns0for indices greater than3, whilegetHalfwordwraps the index using modulo4. This inconsistency might confuse users.Consider aligning the behavior by applying the modulo operation to the
indexingetAdc:- switch (index) { + switch (index % 4) {Alternatively, update the documentation to reflect that indices greater than
3will return0.Likely invalid or redundant comment.
143-145: Verify Button Indexing LogicEnsure that the button indexing in
isButtonPressedcorrectly maps to theButtonenum values. Verify that each button corresponds to the intended bit position.Run the following script to check the correctness of button bit positions:
✅ Verification successful
Button Indexing Implementation is Correct
The button indexing logic in
isButtonPressedcorrectly maps to theButtonenum values. Each button value (0-15) corresponds to its intended bit position in the 16-bit pad data, with proper active-low signal handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that Button enum values correspond to correct bit positions. # Expected: Button values should range from 0 to 15 and map to the appropriate bits. # Extract Button enum values ast-grep --pattern $'enum Button {\n $$$\n};' \ | grep -oP '\w+\s*=\s*\d+' \ | awk -F'=' '{print $1, $2}' \ | while read -r button value; do bit=$((1 << value)) printf "%s (value: %s) should correspond to bit: 0x%X\n" "$button" "$value" "$bit" doneLength of output: 122
Script:
#!/bin/bash # Let's first get the Button enum declaration with context echo "=== Button enum declaration ===" rg -U "enum\s+(?:class\s+)?Button" src/mips/psyqo/advancedpad.hh -A 20 echo -e "\n=== Pad data structure and related definitions ===" rg "m_padData" src/mips/psyqo/advancedpad.hh -B 2 -A 2 echo -e "\n=== Any documentation about button mapping ===" rg -B 2 -A 2 "Button" src/mips/psyqo/advancedpad.hhLength of output: 2950
uint8_t makes more sense here, because the function could return unknown and undocumented PadType values. Silly wabbit! |
Add getters for adc+pad type, update multitap example