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
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mips/psyqo/advancedpad.hh (1)
161-176: Consider simplifying the index selection logic.The switch statement could be simplified using array indexing for better maintainability.
- switch (index) { - case 0: - return m_padData[padIndex].adc[0]; - case 1: - return m_padData[padIndex].adc[1]; - case 2: - return m_padData[padIndex].adc[2]; - case 3: - return m_padData[padIndex].adc[3]; - default: - return 0; - } + return index <= 3 ? m_padData[padIndex].adc[index] : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mips/psyqo/advancedpad.hh(5 hunks)src/mips/psyqo/src/advancedpad.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mips/psyqo/src/advancedpad.cpp
⏰ 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: coverage
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: aur-build
- GitHub Check: build
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
src/mips/psyqo/advancedpad.hh (4)
5-5: LGTM: Copyright year update is appropriate.
45-45: Well-structured enum improvements!Good changes:
- Converting
Padto a scoped enum class improves type safety- Explicit unsigned type prevents potential sign issues
- Moving
Commandto private section properly encapsulates implementation detailsAlso applies to: 212-225
209-209: Appropriate use of uint8_t for getPadType.Good decision to use
uint8_tinstead ofPadTypeenum as the return type, as it allows handling unknown pad types that might not be documented in the enum.
240-248: Well-designed data structure!The
PadDataunion effectively combines:
- Structured access through named fields
- Raw access through packed array
- Efficient memory layout
Also applies to: 250-250
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mips/psyqo/advancedpad.hh (2)
188-198: Consider using PadType enum with unknown value handling.While returning
uint8_tallows for handling unknown pad types, consider using thePadTypeenum with an additionalUnknownvalue to maintain type safety while supporting undocumented values:- uint8_t getPadType(Pad pad) const { return m_padData[static_cast<unsigned>(pad)].padType; } + PadType getPadType(Pad pad) const { + uint8_t type = m_padData[static_cast<unsigned>(pad)].padType; + switch (type) { + case Mouse: return PadType::Mouse; + case NegCon: return PadType::NegCon; + // ... other known types ... + default: return PadType::Unknown; + } + }
Line range hint
251-274: Consider adding a named constant for clarity.The bitwise AND with 7 ensures the pad index stays within range, but consider adding a named constant to make the code more self-documenting:
+ static constexpr unsigned PAD_INDEX_MASK = 0x7; // 8 possible pad values (0-7) // operators implementation - return pad = static_cast<psyqo::AdvancedPad::Pad>((static_cast<unsigned>(pad) + 1) & 7); + return pad = static_cast<psyqo::AdvancedPad::Pad>((static_cast<unsigned>(pad) + 1) & PAD_INDEX_MASK);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/advancedpad.hh(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (6)
src/mips/psyqo/advancedpad.hh (6)
5-5: LGTM: Copyright year is up to date.
45-45: Great improvement: Enhanced type safety with enum class.The conversion to
enum classwith explicit unsigned type prevents accidental implicit conversions and makes the code more type-safe. The explicitstatic_castusage throughout the code properly handles this change.
147-165: LGTM: Well-implemented analog input retrieval.The
getAdcmethod is well-documented and includes proper bounds checking for the index parameter.
167-186: 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", which matches the implementation using
index % 4. However, this is inconsistent with the behavior ofgetAdcwhich returns 0 for indices > 3.
201-214: LGTM: Improved encapsulation of command constants.Moving the
Commandenum to private scope is a good practice as these are implementation details not needed by clients of the class.
229-237: LGTM: Well-designed data structure.The
PadDataunion elegantly provides both structured access to individual fields and raw access to packed data, which is useful for different access patterns while maintaining memory efficiency.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1815 +/- ##
==========================================
- Coverage 9.26% 9.25% -0.01%
==========================================
Files 468 468
Lines 144113 144115 +2
==========================================
Hits 13345 13345
- Misses 130768 130770 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/mips/psyqo/examples/multitap/multitap.cpp (1)
163-209: 🛠️ Refactor suggestionReduce cyclomatic complexity in
printPadType.This method's complexity has been flagged by static analysis. As noted in a previous review, consider using a lookup table approach to simplify the switch statement.
Here's a simplified implementation using a lookup table:
-void MultitapTestScene::printPadType(psyqo::AdvancedPad::Pad pad, int column, const char *name) { - const auto padType = multitapTest.m_input.getPadType(pad); - const char *padTypeStr; - - switch (padType) { - case psyqo::AdvancedPad::PadType::Mouse: - padTypeStr = "Mouse"; - break; - // ... other cases ... - default: - padTypeStr = "Unknown"; - break; - } - print(column + 0, 13, true, name); - print(column + 10, 13, true, "%s %02x", padTypeStr, padType); +namespace { + struct PadTypeInfo { + psyqo::AdvancedPad::PadType type; + const char* name; + }; + + constexpr PadTypeInfo padTypeMap[] = { + {psyqo::AdvancedPad::PadType::Mouse, "Mouse"}, + {psyqo::AdvancedPad::PadType::NegCon, "NegCon"}, + {psyqo::AdvancedPad::PadType::KonamiLightgun, "KonamiLightgun"}, + {psyqo::AdvancedPad::PadType::DigitalPad, "DigitalPad"}, + {psyqo::AdvancedPad::PadType::AnalogStick, "AnalogStick"}, + {psyqo::AdvancedPad::PadType::NamcoLightgun, "NamcoLightgun"}, + {psyqo::AdvancedPad::PadType::AnalogPad, "AnalogPad"}, + {psyqo::AdvancedPad::PadType::Multitap, "Multitap"}, + {psyqo::AdvancedPad::PadType::Jogcon, "Jogcon"}, + {psyqo::AdvancedPad::PadType::Fishingcon, "Fishingcon"}, + {psyqo::AdvancedPad::PadType::ConfigMode, "ConfigMode"}, + {psyqo::AdvancedPad::PadType::None, "None"}, + }; +} + +void MultitapTestScene::printPadType(psyqo::AdvancedPad::Pad pad, int column, const char *name) { + const auto padType = multitapTest.m_input.getPadType(pad); + const char* padTypeStr = "Unknown"; + + for (const auto& info : padTypeMap) { + if (info.type == padType) { + padTypeStr = info.name; + break; + } + } + + print(column + 0, 13, true, name); + print(column + 10, 13, true, "%s %02x", padTypeStr, padType); }🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 163-209: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 14, 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.
🧹 Nitpick comments (1)
src/mips/psyqo/examples/multitap/multitap.cpp (1)
115-122: Enhance type safety with const correctness.Consider making the loop variable
constsince it's not modified within the loop.- for (int i = 0; i < 8; i++) { + for (const int i = 0; i < 8; i++) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mips/psyqo/advancedpad.hh(6 hunks)src/mips/psyqo/examples/multitap/multitap.cpp(6 hunks)src/mips/psyqo/src/advancedpad.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mips/psyqo/src/advancedpad.cpp
- src/mips/psyqo/advancedpad.hh
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/examples/multitap/multitap.cpp
[warning] 163-209: ❌ New issue: Complex Method
MultitapTestScene::printPadType has a cyclomatic complexity of 14, 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: aur-build
- GitHub Check: toolchain
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: asan
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (4)
src/mips/psyqo/examples/multitap/multitap.cpp (4)
5-5: LGTM: Copyright year updated correctly.The copyright year has been updated to reflect the current year (2025).
61-68: LGTM: Well-structured helper methods and constants.The new helper methods provide clear functionality for printing pad information, and the constants are well-organized with descriptive names.
125-161: LGTM: Well-structured pad status printing with ADC support.The method effectively organizes button status printing and handles ADC data with clear comments explaining the bit manipulation.
48-48: Verify font rendering behavior with default template parameter.The change from
Font<1>toFont<>removes the explicit size parameter. Please verify that this doesn't affect the font rendering in the application.
|
The interact fishing controller doesn't seem to get along with the multitap. Probably because of the number of halfwords, but also perhaps the controller isn't able to communicate at 1mhz. Additionally, I may need to look back at the pointer math in the pad driver and put some better bounds checking on the array, or revise the approach completely. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/mips/psyqo/src/advancedpad.cpp (3)
5-5: Update copyright year to match current date.The copyright year is set to 2025, which is ahead of the current date. Consider updating it to 2024 to avoid potential confusion.
-Copyright (c) 2025 PCSX-Redux authors +Copyright (c) 2024 PCSX-Redux authors
152-283: Consider refactoring the readPad method to reduce complexity.The
readPadmethod has high cyclomatic complexity (29, threshold is 9) which makes it harder to maintain and test. Consider breaking it down into smaller, focused methods:
- Split multitap and default pad handling into separate methods
- Extract the pad data processing logic
- Create a dedicated method for error handling
Would you like me to propose a detailed refactoring plan to reduce the complexity?
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 156-208: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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.
190-195: Make the TODO comment more actionable.While the comment explains the LED behavior issue well, it would be more helpful to:
- Create a tracking issue for implementing config mode
- Document the specific conditions that trigger this behavior
- Add acceptance criteria for the implementation
Would you like me to help create a tracking issue for implementing the config mode feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/src/advancedpad.cpp(8 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/advancedpad.cpp
[warning] 156-208: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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.
[warning] 156-208: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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 (12)
- GitHub Check: macos-arm-build-and-test
- GitHub Check: macos-intel-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: aur-build
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: toolchain
- GitHub Check: asan
- GitHub Check: macos-build-and-test-toolchain
🔇 Additional comments (2)
src/mips/psyqo/src/advancedpad.cpp (2)
51-58: LGTM! Improved type safety and correctness.The changes enhance type safety through:
- Using scoped enums for pad identifiers
- Proper casting from enum to array index
- More appropriate use of uint16_t for button states
Also applies to: 116-130
156-156: LGTM! Good use of constexpr.Using constexpr for padDataWidth improves maintainability and compile-time safety.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 156-208: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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.
You're mistaken, the current year is 2025. Could you please propose a detailed refactoring plan to reduce the complexity? |
|
You're mistaken, the current year is 2025. Could you please propose a detailed refactoring plan to reduce the complexity? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@johnbaumann Here's a detailed refactoring plan for the // 1. Split pad type detection into a separate method
private:
enum class PadMode {
Default,
Multitap
};
PadMode detectPadMode(uint16_t* const padData) const {
// Extract pad mode detection logic
return (padData[0] == 0x5a53) ? PadMode::Multitap : PadMode::Default;
}
// 2. Extract pad data processing for default mode
bool processDefaultPad(uint16_t* const padData) {
// Move default pad processing logic here
// Return success/failure status
}
// 3. Extract multitap processing
bool processMultitapPad(uint16_t* const padData) {
// Move multitap processing logic here
// Return success/failure status
}
// 4. Create a dedicated error handler
void handlePadError(uint16_t* const padData) {
// Move error handling logic here
m_error = true;
// ... rest of error handling
}
// 5. Extract pad data validation
bool validatePadData(const uint16_t* const padData) const {
// Move validation logic here
return /* validation result */;
}
// 6. Main method becomes a high-level orchestrator
public:
void readPad() {
uint16_t padData[32];
if (!initializePadRead(padData)) {
handlePadError(padData);
return;
}
const auto mode = detectPadMode(padData);
bool success = false;
switch (mode) {
case PadMode::Default:
success = processDefaultPad(padData);
break;
case PadMode::Multitap:
success = processMultitapPad(padData);
break;
}
if (!success) {
handlePadError(padData);
}
}Key benefits of this refactoring:
Would you like me to elaborate on any specific part of this refactoring plan? 🏁 Script executed: Length of output: 9748 @johnbaumann Here's a detailed refactoring plan that respects the hardware-specific nature of the code: class AdvancedPad {
private:
// 1. Extract port configuration into a dedicated method
void configurePort(uint8_t port) {
SIO::Ctrl = (port * SIO::Control::CTRL_PORTSEL) | SIO::Control::CTRL_DTR;
SIO::Baud = 0x88; // 250kHz
flushRxBuffer();
SIO::Ctrl |= (SIO::Control::CTRL_TXEN | SIO::Control::CTRL_ACKIRQEN);
busyLoop(100); // Required delay for pad stability
}
// 2. Extract pad type detection
PadType detectPadType(uint8_t dataIn, uint8_t port) {
if (dataIn == PadType::Multitap) {
return PadType::Multitap;
}
return static_cast<PadType>(dataIn);
}
// 3. Extract multitap data processing
void processMultitapData(uint8_t dataIn, unsigned ticks, uint8_t* padData) {
unsigned pad_index = ticks >= 11 ? (ticks - 3) / 8 : 0;
switch (ticks) {
case 4: case 12: case 20: case 28: // Connection status
padData[(padDataWidth * pad_index) + 0] = dataIn != 0x5a;
break;
case 3: case 11: case 19: case 27: // ID low byte
padData[(padDataWidth * pad_index) + 1] = dataIn;
break;
default:
if (ticks > 4) { // Data bytes
padData[(padDataWidth * pad_index) + ((ticks - 3) % 8)] = dataIn;
}
}
}
// 4. Extract default pad data processing
void processDefaultPadData(uint8_t dataIn, unsigned ticks, uint8_t* padData) {
switch (ticks) {
case 2: // Connection status
padData[0] = dataIn != 0x5a;
break;
default:
if (ticks > 2) { // Data bytes
padData[ticks - 1] = dataIn;
}
}
}
// 5. Extract communication cycle
bool performCommunicationCycle(uint8_t& dataIn, uint8_t dataOut) {
SIO::Ctrl |= SIO::Control::CTRL_ERRRES;
CPU::IReg.clear(CPU::IRQ::Controller);
dataIn = transceive(dataOut);
return waitForAck();
}
// 6. Main method becomes more structured
void readPad() {
uint8_t portDevType[2] = {PadType::None, PadType::None};
uint8_t port = m_portToProbe;
for (unsigned i = 0; i < m_portsToProbeByVSync; i++) {
configurePort(port);
uint8_t* padData = reinterpret_cast<uint8_t*>(&m_padData[port * 4].packed[0]);
__builtin_memset(padData, 0xff, sizeof(m_padData[0]));
if (!readPortData(port, portDevType[port], padData)) {
handlePortError(port, padData);
}
SIO::Ctrl = 0; // End transmission
port ^= 1;
}
m_portToProbe = port;
}
// 7. Extract port reading logic
bool readPortData(uint8_t port, PadType& portType, uint8_t* padData) {
uint8_t dataIn;
unsigned max_ticks = 5;
for (unsigned ticks = 0; ticks < max_ticks; ticks++) {
uint8_t dataOut = (portType == PadType::Multitap)
? outputMultitap(ticks)
: outputDefault(ticks);
if (!performCommunicationCycle(dataIn, dataOut)) {
return false;
}
if (ticks == 1) {
portType = detectPadType(dataIn, port);
} else if (ticks == 2) {
if (!handleIdentificationPhase(dataIn, max_ticks, padData)) {
return false;
}
} else {
if (portType == PadType::Multitap) {
processMultitapData(dataIn, ticks, padData);
} else {
processDefaultPadData(dataIn, ticks, padData);
}
}
}
return true;
}
};Key benefits of this refactoring:
The refactoring maintains the original functionality while making the code more maintainable and easier to understand. Would you like me to elaborate on any specific part of this refactoring plan? ✏️ Learnings added
|
src/mips/psyqo/advancedpad.hh
Outdated
| @@ -143,7 +129,7 @@ class AdvancedPad { | |||
| * @param pad The pad to query. | |||
| * @return A boolean value indicating whether the pad is connected. | |||
| */ | |||
| bool isPadConnected(Pad pad) const { return (m_padData[pad][0] & 0xff) == 0; } | |||
| bool isPadConnected(Pad pad) const { return m_padData[static_cast<unsigned>(pad)].connected == 0; } | |||
There was a problem hiding this comment.
It might be prettier to use std::to_underlying(pad) instead of the static_cast throughout this file. It comes from <utility>, and it's been added in C++23.
There was a problem hiding this comment.
It's a bit shorter, but looks like we'd need to bump the whole SDK version for that change(currently on C++ 20). I'm ok with working around this with the casts for now if changing the minimum version is undesirable.
There was a problem hiding this comment.
Ah.
We could theoretically polyfill it somewhere in the codebase: https://godbolt.org/z/7r4T6nq3e
As you wish, really.
There was a problem hiding this comment.
I gave this a shot in the latest commit
src/mips/psyqo/src/advancedpad.cpp
Outdated
| uint32_t mask = 1; | ||
| uint32_t padData = m_padData[pad][1]; | ||
| uint32_t buttons = m_buttons[pad]; | ||
| uint16_t mask = 1; |
There was a problem hiding this comment.
Downgrading these to uint16_t will make for a worse codegen in the end. Was there a reason to do this?
There was a problem hiding this comment.
Ugh, I probably thought I was catching a past mistake on the data type.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mips/psyqo/src/advancedpad.cpp (1)
185-194: Consider implementing the TODOs in this PR or creating separate issues.There are several TODOs in the code:
- Line 185: Check SIO status for errors
- Line 190: Implement config mode
- Lines 191-193: Handle the case when pad returns 0x00 for idhi
Consider implementing these TODOs as part of this PR or create separate issues to track them.
Would you like me to help create issues for these TODOs or suggest implementations for any of them?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mips/psyqo/src/advancedpad.cpp(8 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/advancedpad.cpp
[warning] 168-269: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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.
[warning] 168-269: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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 (12)
- GitHub Check: macos-intel-build-and-test
- GitHub Check: macos-arm-build-and-test
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: toolchain
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: coverage
- GitHub Check: aur-build
- GitHub Check: asan
🔇 Additional comments (11)
src/mips/psyqo/src/advancedpad.cpp (11)
5-5: Copyright year updated to 2025.The copyright year has been updated from 2024 to 2025, which is appropriate for new contributions.
51-58: Properly using scoped enum class for pad identifiers.The code now uses the scoped enum
Pad::Pad1ainstead of raw integers, which improves type safety and code clarity.
62-68: Good extraction of common port configuration logic.The new
configurePortmethod effectively centralizes the SIO port configuration logic that was previously duplicated, improving maintainability and reducing the chance of inconsistencies.
77-77: Consistent variable naming with camelCase.Variable naming has been standardized from
data_outtodataOut, improving consistency with the codebase's naming conventions.Also applies to: 81-81, 84-84, 88-88
92-92: Consistent variable naming in outputMultitap.The variable naming changes to camelCase have been applied consistently across methods.
Also applies to: 97-97, 105-105, 120-120
124-125: Improved type safety with static_cast for enum class.The change to use
static_cast<unsigned>(pad)as an index is necessary when working with enum classes and ensures type safety.Also applies to: 136-138, 148-148
151-156: Good refactoring of transceive method.The extraction of the transceive functionality into a dedicated inline method improves code organization and reusability.
169-169: Consistent variable naming for maxTicks.Renamed
max_tickstomaxTicksfor consistency with camelCase naming convention used throughout the codebase.Also applies to: 178-178
211-241: Clearer organization of multitap data processing.The multitap data processing logic has been improved, with better index calculation and clearer condition for when to apply multitap processing.
264-270: Improved error handling in wait for ACK.The error handling when waiting for ACK has been improved, with better initialization of pad data on error.
287-291: Better timeout handling with constexpr.Good use of
static constexprfor the timeout value and improved condition check.
| static constexpr unsigned padDataWidth = sizeof(PadData); | ||
| const unsigned portsToProbeByVSync = m_portsToProbeByVSync; | ||
| uint8_t port = m_portToProbe; | ||
|
|
||
| for (unsigned i = 0; i < portsToProbeByVSync; i++) { | ||
| // Select enable on current port | ||
| SIO::Ctrl = (static_cast<uint16_t>(port) << 13) | SIO::Control::CTRL_DTR; | ||
|
|
||
| // Set baud 250kHz | ||
| SIO::Baud = 0x88; | ||
|
|
||
| flushRxBuffer(); | ||
| configurePort(port); | ||
|
|
||
| // Enable transmit and IRQ on ACK | ||
| SIO::Ctrl |= (SIO::Control::CTRL_TXEN | SIO::Control::CTRL_ACKIRQEN); | ||
|
|
||
| // Pads get finicky if we don't wait a bit here | ||
| busyLoop(100); | ||
|
|
||
| uint8_t *padData = reinterpret_cast<uint8_t *>(&m_padData[port * 4][0]); | ||
| uint8_t *padData = reinterpret_cast<uint8_t *>(&m_padData[port * 4].packed[0]); | ||
| __builtin_memset(padData, 0xff, sizeof(m_padData[0])); | ||
|
|
||
| for (unsigned ticks = 0, max_ticks = 5; ticks < max_ticks; ticks++) { | ||
| SIO::Ctrl |= SIO::Control::CTRL_ERRRES; // Clear error | ||
| CPU::IReg.clear(CPU::IRQ::Controller); // Clear IRQ | ||
|
|
||
| for (unsigned ticks = 0, maxTicks = 5; ticks < maxTicks; ticks++) { | ||
| if (portDevType[port] == PadType::Multitap) { | ||
| dataOut = outputMultitap(ticks); | ||
| } else { | ||
| dataOut = outputDefault(ticks); | ||
| } | ||
| dataIn = transceive(dataOut); | ||
| // To-do: Check SIO status for errors, abort if necessary | ||
|
|
||
| // Set port type and total number of half-words to read | ||
| if (ticks == 2) { | ||
| if (dataIn == 0x5a) { | ||
| if (dataIn == 0x5a || dataIn == 0x00) { // Verify idhi byte | ||
| // To-do: Implement config mode | ||
| // If the LED was previously forced off, as is the case with xloader, the pad will return 0x00 for | ||
| // idhi. The pad will reset after a few seconds of no activity, but we can't wait that long. For | ||
| // now, we can just ignore this and treat it as a normal pad | ||
|
|
||
| // Set number of half-words to read | ||
| max_ticks = portDevType[port] & 0x0f; | ||
| if (!max_ticks) { | ||
| max_ticks = 0x10; | ||
| maxTicks = portDevType[port] & 0x0f; | ||
| if (!maxTicks) { | ||
| maxTicks = 0x10; | ||
| } | ||
| max_ticks = (max_ticks * 2) + ticks + 1; | ||
| maxTicks = (maxTicks * 2) + ticks + 1; | ||
|
|
||
| padData[0] = 0; | ||
| } else { | ||
| // Derp? Unknown device type or bad data, stop reading | ||
| max_ticks = ticks; | ||
| maxTicks = ticks; | ||
| padData[0] = 0xff; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex readPad method to reduce cyclomatic complexity.
The readPad method is flagged by static analysis for high cyclomatic complexity (29, threshold is 9) and it's getting worse. This makes the code harder to maintain and test.
Consider breaking this method into smaller, more focused methods:
- Create separate methods for multitap and default pad handling
- Extract pad data processing logic
- Extract error handling
This would make the code more maintainable and easier to understand.
// Example refactoring approach (partial):
-void psyqo::AdvancedPad::readPad() {
+void psyqo::AdvancedPad::readPad() {
+ uint8_t portDevType[2] = {PadType::None, PadType::None};
+ const unsigned portsToProbeByVSync = m_portsToProbeByVSync;
+ uint8_t port = m_portToProbe;
+
+ for (unsigned i = 0; i < portsToProbeByVSync; i++) {
+ readSinglePort(port, portDevType);
+ port ^= 1;
+ }
+ m_portToProbe = port;
+}
+
+void psyqo::AdvancedPad::readSinglePort(uint8_t port, uint8_t* portDevType) {
+ configurePort(port);
+ uint8_t* padData = reinterpret_cast<uint8_t*>(&m_padData[port * 4].packed[0]);
+ __builtin_memset(padData, 0xff, sizeof(m_padData[0]));
+
+ unsigned maxTicks = 5;
+ for (unsigned ticks = 0; ticks < maxTicks; ticks++) {
+ bool shouldContinue = processPortTick(port, ticks, maxTicks, padData, portDevType);
+ if (!shouldContinue) break;
+ }
+
+ // End transmission
+ SIO::Ctrl = 0;
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 168-269: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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 (6)
src/mips/psyqo/utility-polyfill.h (1)
5-5: Update copyright year to match current date.The copyright year is set to 2025, but the current date is February 2025. This is a minor issue and might be intentional to cover future modifications in the current year.
-Copyright (c) 2025 PCSX-Redux authors +Copyright (c) 2023-2025 PCSX-Redux authorssrc/mips/psyqo/src/advancedpad.cpp (2)
5-5: Update copyright year to match current date.The copyright year is set to 2025, but the current date is February 2025. This is a minor issue and might be intentional to cover future modifications in the current year.
-Copyright (c) 2025 PCSX-Redux authors +Copyright (c) 2023-2025 PCSX-Redux authors
179-207: Consider improving variable names for better clarity.The variable
maxTicksis used in a confusing way - it's initially set to 5 but then potentially updated multiple times. This makes the code harder to follow.Consider renaming variables for clarity:
-for (unsigned ticks = 0, maxTicks = 5; ticks < maxTicks; ticks++) { +for (unsigned ticks = 0, initialMaxTicks = 5, maxTicks = initialMaxTicks; ticks < maxTicks; ticks++) {Or better yet, structuring the loop differently to make the flow more clear:
-for (unsigned ticks = 0, maxTicks = 5; ticks < maxTicks; ticks++) { +unsigned maxTicks = 5; +for (unsigned ticks = 0; ticks < maxTicks; ticks++) {src/mips/psyqo/advancedpad.hh (3)
5-5: Update copyright year to match current date.The copyright year is set to 2025, but the current date is February 2025. This is a minor issue and might be intentional to cover future modifications in the current year.
-Copyright (c) 2025 PCSX-Redux authors +Copyright (c) 2023-2025 PCSX-Redux authors
164-168: Consider returning PadType enum instead of uint8_t.The
getAdcmethod implementation is correct, but for better clarity, consider adding bounds checking with a more explicit error message.uint8_t getAdc(Pad pad, unsigned int index) const { const unsigned padIndex = toUnderlying(pad); - - return index <= 7 ? m_padData[padIndex].adc[index] : 0; + + if (index > 7) { + return 0; // Invalid index + } + + return m_padData[padIndex].adc[index]; }
199-199: Consider returning PadType enum instead of uint8_t.For better type safety and clarity, consider returning the PadType enum instead of uint8_t. This would make the API more self-documenting and reduce the chance of errors.
-uint8_t getPadType(Pad pad) const { return m_padData[toUnderlying(pad)].padType; } +PadType getPadType(Pad pad) const { return static_cast<PadType>(m_padData[toUnderlying(pad)].padType); }However, if this function needs to return unknown/undocumented PadType values, then using uint8_t might be more appropriate.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mips/psyqo/advancedpad.hh(7 hunks)src/mips/psyqo/src/advancedpad.cpp(9 hunks)src/mips/psyqo/utility-polyfill.h(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/mips/psyqo/src/advancedpad.cpp
[warning] 169-270: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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.
[warning] 169-270: ❌ Getting worse: Complex Method
psyqo::AdvancedPad::readPad increases in cyclomatic complexity from 28 to 29, 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 (11)
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: toolchain
- GitHub Check: aur-build
- GitHub Check: asan
- GitHub Check: build-openbios
- GitHub Check: coverage
- GitHub Check: cross-arm64
- GitHub Check: build
🔇 Additional comments (13)
src/mips/psyqo/utility-polyfill.h (1)
32-38: Good implementation of toUnderlying utility function.This is a clean implementation of a utility function that safely converts enum values to their underlying types. The use of the
Enumconcept ensures type safety by restricting the template to enum types only.src/mips/psyqo/src/advancedpad.cpp (6)
52-59: Good conversion to scoped enum references.The code correctly updates the references to use the new scoped enum format, which improves type safety and code clarity.
63-69: Excellent extraction of port configuration logic.The common port configuration logic has been extracted into a dedicated method, which improves code organization and maintainability by reducing duplication.
77-89: Consistent variable naming with dataOut.The variable naming has been updated from
data_outtodataOutfor better consistency throughout the codebase.
92-121: Consistent variable naming in outputMultitap.Similar to the previous method, the variable naming has been standardized to use camelCase.
124-150: Good use of toUnderlying for type safety.The code now properly uses the
toUnderlyingfunction when accessing array elements indexed by enum values, which improves type safety.
165-207: 🛠️ Refactor suggestionConsider refactoring the complex readPad method.
The
readPadmethod continues to have high cyclomatic complexity (29, threshold is 9) as flagged by static analysis. This makes the code harder to maintain and test.Consider breaking this method into smaller, more focused methods:
- Create separate methods for multitap and default pad handling
- Extract pad data processing logic
- Extract error handling
This would make the code more maintainable and easier to understand.
// Example refactoring approach (partial): -void psyqo::AdvancedPad::readPad() { +void psyqo::AdvancedPad::readPad() { + uint8_t portDevType[2] = {PadType::None, PadType::None}; + const unsigned portsToProbeByVSync = m_portsToProbeByVSync; + uint8_t port = m_portToProbe; + + for (unsigned i = 0; i < portsToProbeByVSync; i++) { + readSinglePort(port, portDevType); + port ^= 1; + } + m_portToProbe = port; +} + +void psyqo::AdvancedPad::readSinglePort(uint8_t port, uint8_t* portDevType) { + configurePort(port); + uint8_t* padData = reinterpret_cast<uint8_t *>(&m_padData[port * 4].packed[0]); + __builtin_memset(padData, 0xff, sizeof(m_padData[0])); + + unsigned maxTicks = 5; + for (unsigned ticks = 0; ticks < maxTicks; ticks++) { + bool shouldContinue = processPortTick(port, ticks, maxTicks, padData, portDevType); + if (!shouldContinue) break; + } + + // End transmission + SIO::Ctrl = 0; +}src/mips/psyqo/advancedpad.hh (6)
47-47: Good conversion to scoped enum class.Changing Pad from a regular enum to a scoped enum class improves type safety and reduces potential for naming conflicts.
134-134: Good use of toUnderlying in isPadConnected.The method has been updated to use toUnderlying when accessing the m_padData array, which ensures type safety.
146-148: Good use of toUnderlying in isButtonPressed.The method has been updated to use toUnderlying when accessing the m_padData array, which ensures type safety.
187-187: 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", and the implementation correctly uses modulo operator for wrapping.
232-241: Good use of union for structured data access.The change from a 2D array to a union with a structured view improves code clarity by providing named fields for the different components of pad data.
254-255: Good use of toUnderlying in operator overloads.The increment/decrement operators have been updated to use toUnderlying, which ensures type safety and compatibility with the new enum class.
Add getters for adc+pad type, update multitap example