SIO0 overhaul #1673
SIO0 overhaul #1673
Conversation
|
This one is ready for review. Moved a lot of things around to split the card and pad code away from the sio class. |
|
You're making Linux sad :) |
|
Looks like there's still an asan error, with a mismatched new/delete type. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1673 +/- ##
=======================================
Coverage ? 9.87%
=======================================
Files ? 448
Lines ? 133030
Branches ? 0
=======================================
Hits ? 13143
Misses ? 119887
Partials ? 0 ☔ View full report in Codecov by Sentry. |
src/gui/widgets/memcard_manager.h
Outdated
| std::memcpy(data.get(), g_emulator->m_sio->getMcdData(1), SIO::c_cardSize); | ||
| std::memcpy(data.get() + SIO::c_cardSize, g_emulator->m_sio->getMcdData(2), SIO::c_cardSize); | ||
| std::unique_ptr<uint8_t[]> data = std::make_unique<uint8_t[]>(MemoryCards::c_cardSize * 2); | ||
| std::memcpy(data.get(), g_emulator->m_memoryCards->getMcdData(0), MemoryCards::c_cardSize); |
There was a problem hiding this comment.
If we're here doing some refactoring, we might as well use proper enums for the memory cards indices, maybe?
| // skip id 8 | ||
| // skip id 9 | ||
| // skip id 10 | ||
| // skip id 11 | ||
| typedef Protobuf::FieldRef<Protobuf::UInt32, TYPESTRING("pad_state"), 12> SIOPadState; | ||
| // skip id 12 |
There was a problem hiding this comment.
So, uuuuh, what happens if a savestate is done while an SIO read is in progress? This doesn't seem we're reloading into a proper state, able to continue gracefully. Or we never did, maybe, for memory cards at least?
There was a problem hiding this comment.
I'll revisit the save state code. Admittedly, I had plans to eventually redo the SIO side of pads, so this escaped my focus. Thank you for catching this. I know pad issues have been noted regarding savestates back in #1173 maybe I can knock this one out in the process.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe pull request introduces a comprehensive refactoring of memory card and serial input/output (SIO) management in the PCSX emulator. The changes centralize memory card operations into a new Changes
Sequence DiagramsequenceDiagram
participant Emulator
participant MemoryCards
participant MemoryCard
participant SIO
Emulator->>MemoryCards: loadMcds(args)
MemoryCards->>MemoryCard: createMcd(path)
MemoryCards->>SIO: writePad/writeCard
MemoryCard-->>MemoryCards: return block info
MemoryCards-->>Emulator: memory card loaded
Poem
🪧 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: 3
🧹 Nitpick comments (13)
src/core/sio.h (1)
45-45: Consider defaulted destructor.
A trivial destructor could be marked as= default;to clearly convey no resources need custom cleanup.- ~SIO() {} + ~SIO() = default;src/core/sio.cc (2)
108-126: Code duplication warning (writeCard vs. writePad).
writeCardreferences logic very similar towritePad. Moving common steps (port index resolution, device check, ack logic) into a shared helper could mitigate duplication.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 108-126: ❌ New issue: Code Duplication
The module contains 2 functions with similar structure: PCSX::SIO::writeCard,PCSX::SIO::writePad. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
[notice] 108-145: ✅ No longer an issue: Complex Method
PCSX::SIO::writePad is no longer above the threshold for cyclomatic complexity. 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.
187-187: Consider narrowing logging.
sio write8 %x\nmight be a frequent log spam in normal runs. A debug-level or conditional logging might help performance.src/core/memorycard.h (2)
109-116: PocketStation command expansions well-labeled.
Each function is named by the command code (tickPS_GetDirIndex, etc.). This is quite descriptive. Just watch for code duplication if these commands share large blocks of logic.
186-189: Duplicated constants.
MemoryCards::c_sectorSize,c_blockSize, andc_cardSizeduplicate the same numeric values as inMemoryCard. Consider referencing a single source of truth to avoid confusion if these ever need changing.src/gui/widgets/memcard_manager.cc (2)
127-143: Undo system with memory cards.
Reverting to older states by shuttling raw memory is effective but can be memory-heavy. Consider limiting or compressing the undo buffer if large memory cards are changed frequently.
432-432: Exporting icon to PNG.
Naming the file asiconN.pngis straightforward. Potentially prompt the user for a path or confirm overwrites for multiple exports in the same folder.src/core/sstate.cc (1)
89-104: Consider splitting out memory card SIO fields initialization to maintain readability.While these lines effectively integrate memory card data into the save state, the method
constructSaveState()is still quite large and somewhat cumbersome to maintain. Consider extracting memory card SIO-related initialization to a dedicated function or helper object for clarity and better modularity.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 83-104: ✅ Getting better: Large Method
PCSX::SaveStates::constructSaveState decreases from 138 to 134 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.src/core/memorycard.cc (2)
255-302: Excessive branching incopyMcdFile.With a cyclomatic complexity of 11, this function has multiple nested conditionals and loops. Consider extracting repeated or branching logic into well-named helper functions that handle partial tasks, thus enhancing clarity and maintainability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 255-302: ❌ New issue: Complex Method
PCSX::MemoryCards::copyMcdFile has a cyclomatic complexity of 11, 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.
731-731: FunctioncreateMcdat the upper refactoring threshold.While not severely problematic, static analysis flags that the function’s logical complexity is at the threshold (cyclomatic complexity of 9). Future additions could push it over. Consider carefully grouping or extracting repetitive code to keep it manageable.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 731-731: ✅ No longer an issue: Complex Method
PCSX::MemoryCard::createMcd is no longer above the threshold for cyclomatic complexity. 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] 731-731: ❌ New issue: Complex Method
PCSX::MemoryCards::createMcd has a cyclomatic complexity of 9, 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.
[notice] 731-731: ✅ No longer an issue: Bumpy Road Ahead
PCSX::MemoryCard::createMcd is no longer above the threshold for logical blocks with deeply nested code. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 731-731: ❌ New issue: Bumpy Road Ahead
PCSX::MemoryCards::createMcd has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.src/core/pad.cc (1)
214-220: Large buffer size allocation.
static constexpr size_t c_padBufferSize = 0x1010;might be more than required for typical pad operations. Evaluate if smaller allocations or a dynamic approach can be used without impacting performance.src/core/psxemulator.h (1)
268-268: Confirm appropriate destruction order.
StoringMemoryCardsas aunique_ptrin the emulator is good for ownership clarity. However, ensure thatm_memoryCardsis properly reset or closed before other dependent components are destructed.src/core/sstate.h (1)
139-139: Remove or clarify skipped IDs if unneeded.
These skip comments (e.g. “// skip id 1”) may cause confusion later if they’re never repurposed. Remove them if you no longer need these placeholders.Also applies to: 144-145, 150-150
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/core/memorycard.cc(10 hunks)src/core/memorycard.h(4 hunks)src/core/pad.cc(4 hunks)src/core/pad.h(1 hunks)src/core/psxemulator.cc(1 hunks)src/core/psxemulator.h(2 hunks)src/core/sio.cc(7 hunks)src/core/sio.h(2 hunks)src/core/sstate.cc(1 hunks)src/core/sstate.h(2 hunks)src/gui/widgets/memcard_manager.cc(15 hunks)src/gui/widgets/memcard_manager.h(3 hunks)src/main/main.cc(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
src/core/sio.cc
[warning] 108-126: ❌ New issue: Code Duplication
The module contains 2 functions with similar structure: PCSX::SIO::writeCard,PCSX::SIO::writePad. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
[notice] 108-145: ✅ No longer an issue: Complex Method
PCSX::SIO::writePad is no longer above the threshold for cyclomatic complexity. 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.
[notice] 152-177: ✅ No longer an issue: Complex Method
PCSX::SIO::transmitData is no longer above the threshold for cyclomatic complexity. 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.
[notice] 152-177: ✅ No longer an issue: Bumpy Road Ahead
PCSX::SIO::transmitData is no longer above the threshold for logical blocks with deeply nested code
[notice] 152-177: ✅ No longer an issue: Deep, Nested Complexity
PCSX::SIO::transmitData is no longer above the threshold for nested complexity depth
src/core/sstate.cc
[notice] 83-104: ✅ Getting better: Large Method
PCSX::SaveStates::constructSaveState decreases from 138 to 134 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
src/core/memorycard.cc
[warning] 46-174: ❌ New issue: Complex Method
PCSX::MemoryCards::getMcdBlockInfo has a cyclomatic complexity of 31, 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] 255-302: ❌ New issue: Complex Method
PCSX::MemoryCards::copyMcdFile has a cyclomatic complexity of 11, 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] 46-174: ❌ New issue: Bumpy Road Ahead
PCSX::MemoryCards::getMcdBlockInfo has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 514-514: ❌ Getting worse: Code Duplication
introduced similar code in: PCSX::MemoryCards::findFirstFree,PCSX::MemoryCards::getFreeSpace. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
[notice] 731-731: ✅ No longer an issue: Complex Method
PCSX::MemoryCard::createMcd is no longer above the threshold for cyclomatic complexity. 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] 731-731: ❌ New issue: Complex Method
PCSX::MemoryCards::createMcd has a cyclomatic complexity of 9, 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.
[notice] 731-731: ✅ No longer an issue: Bumpy Road Ahead
PCSX::MemoryCard::createMcd is no longer above the threshold for logical blocks with deeply nested code. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
[warning] 731-731: ❌ New issue: Bumpy Road Ahead
PCSX::MemoryCards::createMcd has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
🔇 Additional comments (47)
src/gui/widgets/memcard_manager.h (4)
30-30: Prefer narrower include scopes.
Including"core/memorycard.h"is logically correct. However, keep an eye on compile/link times if there's an opportunity to forward-declare classes and move includes to the .cpp file.
60-65: Consistent naming for icon-related helper methods.
All methods dealing with icons (getIconRGBA8888,drawIcon,exportPNG,copyToClipboard,getPocketstationIcon) maintain a clear naming pattern. Good job keeping them grouped for clarity.
70-73: Ensure boundary checks or exception handling for raw memory operations.
Usingstd::memcpyon two separately allocated buffers is fine. It's assumed thatMemoryCards::c_cardSize * 2can hold 2 memory cards. Verify that these calls and the destinations (getMcdData(MemoryCard::Which::One/Two)) always match the declared size to avoid accidental overflows.
82-82: Enum usage aligns with past feedback.
UsingMemoryCard::Whichinstead of an integer is consistent and less error-prone when referencing memory cards.src/core/sio.h (1)
168-169: Write methods for cards and pads unify SIO responsibilities.
The newwriteCardandwritePadmethods returnuint8_tfor data read-back. This is a clarity improvement over the previous approach of returningvoid.src/core/sio.cc (7)
47-47: Deferred toggling of pocketstation mode removed.
init()now only sets up pads and callsreset(). Ensure that any prior toggling logic of pocketstation mode is truly unnecessary.
87-89: Check correctness of transmit-ready condition.
(txEnabled && txFinished && !txDataEmpty)inisTransmitReady()might change existing logic. Verify that the data flow still triggers transmissions as intended, especially for corner cases where TX has data but is flagged finished.
96-105: Reinitialize data on reset.
Clearing FIFOs, resetting status flags, and deselecting devices ensure fresh states upon reset. Good defensive design. Just ensure other SIO members aren’t left uninitialized accidentally.
128-145: Clean approach for pad transceive.
writePadparallelswriteCard. The early device ignoring is a neat fallback. Consider logging or debugging output if the device is not connected.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 108-145: ✅ No longer an issue: Complex Method
PCSX::SIO::writePad is no longer above the threshold for cyclomatic complexity. 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.
152-177: Simplify device selection intransmitData().
Switching onm_currentDeviceto handle either Pad, MemoryCard, or ignoring input is clear. The final push tom_rxFIFOis done consistently. Just confirm that other device types cannot appear in production or handle them with a fallback.🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[notice] 152-177: ✅ No longer an issue: Complex Method
PCSX::SIO::transmitData is no longer above the threshold for cyclomatic complexity. 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.
[notice] 152-177: ✅ No longer an issue: Bumpy Road Ahead
PCSX::SIO::transmitData is no longer above the threshold for logical blocks with deeply nested code
[notice] 152-177: ✅ No longer an issue: Deep, Nested Complexity
PCSX::SIO::transmitData is no longer above the threshold for nested complexity depth
205-206: Validate port switching logic and device deselection.
- Lines 205–206:
port_changedindicates a toggled port bit. Ensure correctness when dynamically switching from Port1 to Port2.- Lines 219–220: Both memory card and pad are deselected on deselect or port change, which is correct.
- Line 234:
reset()uponRESETin control flags can disrupt ongoing transmissions. Confirm correct flow in the actual hardware model.- Line 244: The final check for
was_ready == falseensures immediate transmit if lines are now ready.
Looks consistent, but thorough testing is advised.Also applies to: 210-210, 216-216, 219-220, 234-234, 244-244
259-259: No issues found.
Reading byte from FIFO is straightforward, with the SIO status updated properly.src/core/memorycard.h (14)
24-26: Check for minimal includes.
Including"core/psxemulator.h"and"core/sstate.h"is likely essential here as the code strongly referencesg_emulatorand save states. Forward declarations may reduce compilation time if not all details of these headers are needed immediately.
31-41: Enum-based card selection is neat.
DefiningMemoryCard::Whichclarifies usage. This addresses prior feedback about using typed enums instead of bare integers for memory card indices.
53-58: Reset method consolidates state.
deselect()followed by reinitializingm_directoryFlagensures a clean reset for the memory card’s state machine. Good practice.
61-61: Commit method usage.
commit()is declared but not shown here. Verify it robustly handles finalizing saves or notifying the emulator. Ensure it’s being invoked where needed.
104-106: Constants alignment.
c_sectorSize,c_blockSize, andc_cardSizedefine the card geometry. Double-check that the entire codebase references these constants consistently to avoid mismatch in read/write calls.
Line range hint
117-137: Good usage of friend classes for direct memory access.
Exposing memory card internals toSIOandSaveStates::constructSaveState()is acceptable for low-level hardware emulation. Just keep an eye on encapsulation so that only essential internals are friend-accessible.
139-141: Clear doc comment.
The heading forMemoryCardsclarifies it as a higher-level manager for multiple memory cards, bridging the GUI and emulator. Nicely done.
142-147: Deselect all memory cards.
MemoryCards::deselect()callsdeselect()on each. Straightforward. Good for ensuring no leftover states.
149-152:reset()for both memory cards.
Similar approach—ensures both are in a pristine state withm_directoryFlagreset. Good consistency with the individualMemoryCard::reset().
154-184:McdBlockstruct captures essential fields.
Title, ID, name, icon data, etc., all in one place. Thereset()method is thorough. TheisChained()andisErased()checks are straightforward. Keep up the clarity.
190-203: Memory card file manipulations.
copyMcdFile,eraseMcdFile, andfindFirstFreereflect a comprehensive set of operations. Just ensure you gracefully handle errors (e.g. block not found or corrupted data).
[approve]
204-219: File operations and path retrieval.
loadMcds,saveMcd,createMcd, andgetMcdPathincorporate a mix of file I/O and internal memory pointers. Ensure robust error handling for I/O failures.
226-231:isCardInserted()complements dynamic memory card states.
The logic references emulator settings. Seems consistent with your approach. Integration tests recommended to confirm hot-plugging works as expected.
232-237:resetCardandsetPocketstationEnabled.
Exposes straightforward methods to manipulate card states. The arraym_memoryCard[2]is well-structured to store both cards.src/gui/widgets/memcard_manager.cc (11)
28-28: Magic Enum usage.
magic_enumusage for enumerations is excellent to avoid manual conversions. Confirm the library remains consistently included across the project and is licensed compatibly.
67-79: Import/export selection by enumerations.
When a user chooses import/export on memory card 1 or 2, you setm_memoryCardImportExportIndextoMemoryCard::Which::One/Two. This is more robust than using integer IDs.
93-97: Loading memory card in place.
loadMcd(...)intogetMcdData(...)is direct. Confirm that overwriting data in memory is safe. Possibly consider prompting to back up existing data if there's an unintended overwrite scenario.
111-111: Export to file.
Retrieving the card data fromgetMcdDataand directly writing 128 * 1024 bytes is straightforward. Confirm that partial or short writes are handled (e.g.out->failed()).
171-181: Toggling PocketStation.
setPocketstationEnabledis called for each card. Looks correct. Ensure the user’s setting persists if the emulator restarts (save to config or a persistent storage).
Line range hint
194-296: Detailed usage table in a lambda.
Drawing the blocks in a table with actions (erase/copy/move/export icon) is well-structured. The free-space check ensures moves/copies won't fail mid-transfer. Great user experience.
323-327: Tabbed interface for Card 1 and 2.
Switching between the memory cards in tabs and calling the samedrawlambda is a reusable design.
338-338: No immediate issues withdrawIcon.
The method properly updates the OpenGL texture for the current frame. Confirm performance is acceptable for frequent re-drawing of icons.
365-367: Ensure valid pointer arithmetic for PocketStation icons.
titleFrameoffset calculations rely on reading data from memory. Confirm that the offsets, especially with(titleFrame[0x2] & 0xf), are always in range and won't cause out-of-bounds access.
391-391: Flexible approach to alpha shift.
getIconRGBA8888sets up masks and shifts for RGBA. Good approach for cross-platform usage.
438-438: Clipboard copy.
clip::set_imagewith the icon image is a nice convenience feature. Thorough testing recommended across platforms.src/core/pad.cc (4)
68-72: Deselect method is concise and clear.The loop gracefully deselects each pad. Good approach for resetting pad states simultaneously.
74-75: Possible out-of-bounds risk intransceive(...).This directly indexes
m_pads[index]. Ensure that0 <= index < m_pads.size(). Consider gracefully handling out-of-range inputs with a safe return or log message.
177-182:Pad::deselecteffectively resets pad state.This method sets internal indices and states to zero or idle. Looks correct and straightforward.
64-64: Check index validity inisPadConnected.The code checks if
padexceeds the array size. That’s good, but ensurepadis also non-negative. Otherwise, negative input might lead to unintended array access.Run this script to confirm no negative values are passed throughout the codebase:
src/core/pad.h (3)
32-32: Forward declaration ofclass SIO.Having a forward declaration here is fine if you’re planning to reference
SIOwithout including its header, but confirm that you don’t need additional declarations or an actual include ifSIOis used beyond pointers/references.
40-44:InputDeviceabstraction.These pure virtual methods nicely decouple pad state retrieval from the rest of the system, aligning well with SOLID principles.
49-50: Newdeselectandtransceivemethods.Replacing the old polling approach with a single
transceivemethod plus adeselectcall clarifies responsibilities. This likely simplifies the SIO-level logic.src/core/psxemulator.cc (1)
69-69: Initialization ofm_memoryCards.This pointer-based approach is acceptable, given the single responsibility for lifetime management in the
Emulatorconstructor/destructor. Confirm that no additional initialization sequence is needed before usage.src/core/psxemulator.h (1)
81-81: Forward declaration looks appropriate.
This forward declaration helps reduce coupling between headers. Keep it up!src/main/main.cc (1)
234-235: Handle or log errors from loadMcds.
WhenloadMcdsfails to load memory cards, it’s unclear whether exceptions or error codes are returned. Consider adding robust error handling or logging here to avoid silent failures.
| } | ||
|
|
||
| // Poor man's SJIS to ASCII conversion | ||
| if (c >= 0x8281 && c <= 0x829a) { | ||
| c = (c - 0x8281) + 'a'; | ||
| } else if (c >= 0x824f && c <= 0x827a) { | ||
| c = (c - 0x824f) + '0'; | ||
| } else if (c == 0x8140) { | ||
| c = ' '; | ||
| } else if (c == 0x8143) { | ||
| c = ','; | ||
| } else if (c == 0x8144) { | ||
| c = '.'; | ||
| } else if (c == 0x8146) { | ||
| c = ':'; | ||
| } else if (c == 0x8147) { | ||
| c = ';'; | ||
| } else if (c == 0x8148) { | ||
| c = '?'; | ||
| } else if (c == 0x8149) { | ||
| c = '!'; | ||
| } else if (c == 0x815e) { | ||
| c = '/'; | ||
| } else if (c == 0x8168) { | ||
| c = '"'; | ||
| } else if (c == 0x8169) { | ||
| c = '('; | ||
| } else if (c == 0x816a) { | ||
| c = ')'; | ||
| } else if (c == 0x816d) { | ||
| c = '['; | ||
| } else if (c == 0x816e) { | ||
| c = ']'; | ||
| } else if (c == 0x817c) { | ||
| c = '-'; | ||
| } else if (c > 0x7e) { | ||
| c = '?'; | ||
| } | ||
|
|
||
| ta += c; | ||
| } | ||
|
|
||
| info.titleUtf8 = Sjis::toUtf8(ts); | ||
|
|
||
| // Read CLUT | ||
| ptr = reinterpret_cast<uint8_t *>(data) + block * c_blockSize + 0x60; | ||
| std::memcpy(clut, ptr, 16 * sizeof(uint16_t)); | ||
|
|
||
| // Icons can have 1 to 3 frames of animation | ||
| for (uint32_t i = 0; i < info.iconCount; i++) { | ||
| uint16_t *icon = &info.icon[i * 16 * 16]; | ||
| ptr = reinterpret_cast<uint8_t *>(data) + block * c_blockSize + 128 + 128 * i; // icon data | ||
|
|
||
| // Fetch each pixel, store it in the icon array in ABBBBBGGGGGRRRRR with the alpha bit set to 1 | ||
| for (x = 0; x < 16 * 16; x++) { | ||
| const uint8_t entry = (uint8_t)*ptr; | ||
| icon[x++] = clut[entry & 0xf] | (1 << 15); | ||
| icon[x] = clut[entry >> 4] | (1 << 15); | ||
| ptr++; | ||
| } | ||
| } | ||
|
|
||
| // Parse directory frame info | ||
| const auto directoryFrame = (uint8_t *)data + block * c_sectorSize; | ||
| uint32_t allocState = 0; | ||
| allocState |= directoryFrame[0]; | ||
| allocState |= directoryFrame[1] << 8; | ||
| allocState |= directoryFrame[2] << 16; | ||
| allocState |= directoryFrame[3] << 24; | ||
| info.allocState = allocState; | ||
|
|
||
| char tmp[17]; | ||
| memset(tmp, 0, sizeof(tmp)); | ||
| std::strncpy(tmp, (const char *)&directoryFrame[0xa], 12); | ||
| info.id = tmp; | ||
| memset(tmp, 0, sizeof(tmp)); | ||
| std::strncpy(tmp, (const char *)&directoryFrame[0x16], 16); | ||
| info.name = tmp; | ||
|
|
||
| uint32_t fileSize = 0; | ||
| fileSize |= directoryFrame[4]; | ||
| fileSize |= directoryFrame[5] << 8; | ||
| fileSize |= directoryFrame[6] << 16; | ||
| fileSize |= directoryFrame[7] << 24; | ||
| info.fileSize = fileSize; | ||
|
|
||
| uint16_t nextBlock = 0; | ||
| nextBlock |= directoryFrame[8]; | ||
| nextBlock |= directoryFrame[9] << 8; | ||
| info.nextBlock = nextBlock == 0xffff ? -1 : (nextBlock + 1); | ||
|
|
||
| // Check if the block is marked as free in the directory frame and adjust the name/filename if so | ||
| if (info.isErased()) { | ||
| info.reset(); | ||
| info.allocState = 0xa0; | ||
| info.titleAscii = "Free Block"; | ||
| info.titleSjis = "Free Block"; | ||
| info.titleUtf8 = "Free Block"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
High cyclomatic complexity in getMcdBlockInfo.
Static analysis indicates a cyclomatic complexity of 31. This method contains multiple loops and conditionals, making it harder to maintain. Refactor the logic into smaller helper functions to reduce complexity and improve readability.
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (main)
[warning] 46-174: ❌ New issue: Complex Method
PCSX::MemoryCards::getMcdBlockInfo has a cyclomatic complexity of 31, 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] 46-174: ❌ New issue: Bumpy Road Ahead
PCSX::MemoryCards::getMcdBlockInfo has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
| if (argPath2.has_value()) { | ||
| settings.get<PCSX::Emulator::SettingMcd2>() = argPath1.value(); | ||
| } |
There was a problem hiding this comment.
Possible bug: using argPath1.value() instead of argPath2.value().
Here, if argPath2 is provided, you're still assigning argPath1 to SettingMcd2. This likely overwrites the path for memory card 2 with card 1’s path.
- settings.get<PCSX::Emulator::SettingMcd2>() = argPath1.value();
+ settings.get<PCSX::Emulator::SettingMcd2>() = argPath2.value();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (argPath2.has_value()) { | |
| settings.get<PCSX::Emulator::SettingMcd2>() = argPath1.value(); | |
| } | |
| if (argPath2.has_value()) { | |
| settings.get<PCSX::Emulator::SettingMcd2>() = argPath2.value(); | |
| } |
| typedef Protobuf::Message<TYPESTRING("SIO"), SIOStatusReg, SIOModeReg, SIOCtrlReg, SIOBaudReg, SIOCurrentDevice, | ||
| SIOMCD1TempBuffer, SIOMCD1DirectoryFlag, SIOMCD1ChecksumIn, SIOMCD1ChecksumOut, | ||
| SIOMCD1CommandTicks, SIOMCD1CurrentCommand, SIOMCD1Sector, SIOMCD1DataOffset, | ||
| SIOMCD2TempBuffer, SIOMCD2DirectoryFlag, SIOMCD2ChecksumIn, SIOMCD2ChecksumOut, | ||
| SIOMCD2CommandTicks, SIOMCD2CurrentCommand, SIOMCD2Sector, SIOMCD2DataOffset> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reassess embedding memory card fields in SIO.
You’re factoring memory card logic out of the SIO class, yet these fields still reside in the SIO Protobuf definition. Confirm whether you want to keep them here or migrate them to a more suitable location to reflect the new architecture.
I can propose a revised Protobuf layout if you’d like me to open a new issue.
DraftPR, trying to shore up various changes I had worked up in preparation of adding multitap support. My initial branch became sorely outdated and conflicted, this branch attempts to cherry pick some of these changes and bring the branch back in line with head. Needs some additional cleanup; get rid of vectors, mentions of multitap, test various things, etc.Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the release notes:
Release Notes
Memory Card Management
MemoryCardsclass to centralize memory card operationsPad and SIO Interactions
Emulator State Management
User Interface
These changes represent a significant refactoring of the memory card and device interaction systems, focusing on improved reliability and maintainability.