Conversation
WalkthroughThis change refactors the firmware flashing logic for supported drives by removing the combined MT1339/SD616 flashing module and introducing two new modules: one for TSST protocol flashing and another specifically for SD616. Associated enums and struct names are updated for consistency, and command-handler mappings are revised accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Context
participant Options
participant drive.flash.tsst
participant drive.flash.sd616
participant scsi.cmd
User->>CLI: Invoke "flash::mt1339" or "flash::sd616"
CLI->>Context: Prepare context
CLI->>Options: Parse options
alt flash::mt1339
CLI->>drive.flash.tsst: redumper_flash_tsst(ctx, options)
drive.flash.tsst->>scsi.cmd: flash_tsst(..., END_256KB)
else flash::sd616
CLI->>drive.flash.sd616: redumper_flash_sd616(ctx, options)
drive.flash.sd616->>drive.flash.tsst: flash_tsst(..., END_128KB)
drive.flash.tsst->>scsi.cmd: flash_tsst(...)
end
scsi.cmd-->>drive.flash.tsst: Status
drive.flash.tsst-->>CLI: Success/Failure
drive.flash.sd616-->>CLI: Success/Failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
drive/flash_sd616.ixx (1)
26-28: Verify firmware file size check.The size check validates against
0x20000(128KB) but the error message mentions "128Kb" (lowercase 'b'). While technically correct, consider using "128KB" for consistency with the hex constant and standard binary notation.- throw_line("failed to flash firmware, file is not 128Kb"); + throw_line("failed to flash firmware, file is not 128KB");drive/flash_tsst.ixx (2)
33-33: Verify mode selection logic.The ternary operator chain for mode selection is correct but dense. Consider breaking it down for better readability:
- FLASH_TSST_Mode mode = offset == 0 ? FLASH_TSST_Mode::START : (offset_next < firmware_data.size() ? FLASH_TSST_Mode::CONTINUE : end_mode); + FLASH_TSST_Mode mode; + if (offset == 0) { + mode = FLASH_TSST_Mode::START; + } else if (offset_next < firmware_data.size()) { + mode = FLASH_TSST_Mode::CONTINUE; + } else { + mode = end_mode; + }
51-51: Document the block size rationale.The comment mentions the block size is from "the original flasher" but doesn't specify which device or protocol. Consider adding more context about why this specific value (0xFC00 = 64512 bytes) was chosen.
- // block size is how much data is sent in one command, potentially it can vary but current value is taken from the original flasher + // block size is how much data is sent in one command, potentially it can vary + // current value (64512 bytes) is taken from the original MT1339 flasher
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(1 hunks)drive/flash_mt1339.ixx(0 hunks)drive/flash_sd616.ixx(1 hunks)drive/flash_tsst.ixx(1 hunks)options.ixx(1 hunks)redumper.ixx(2 hunks)scsi/cmd.ixx(1 hunks)scsi/mmc.ixx(1 hunks)
💤 Files with no reviewable changes (1)
- drive/flash_mt1339.ixx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (macos-14, 64)
- GitHub Check: build (ubuntu-24.04, 32)
- GitHub Check: build (ubuntu-24.04, 64)
- GitHub Check: build (windows-2022, 64)
- GitHub Check: build (windows-2022, 32)
🔇 Additional comments (12)
scsi/mmc.ixx (2)
815-821: LGTM! Improved naming consistency and clarity.The renaming from
FLASH_Tsst_ModetoFLASH_TSST_Modeimproves consistency, and the enum value changes (END_SAMSUNG→END_128KB,END→END_256KB) make the purpose more explicit and descriptive.
824-835: LGTM! Consistent struct naming.The struct rename from
CDB12_FlashTssttoCDB12_FlashTSSTmaintains consistency with the enum naming changes above.CMakeLists.txt (1)
139-140: LGTM! Good modularization approach.Replacing the single
drive/flash_mt1339.ixxwith separatedrive/flash_tsst.ixxanddrive/flash_sd616.ixxmodules improves separation of concerns and aligns with the refactoring objectives.redumper.ixx (3)
29-29: LGTM! Import aligns with new module structure.Adding the import for
drive.flash.sd616is consistent with the modular refactoring approach.
151-151: LGTM! Command handler updated correctly.The command handler for
"flash::mt1339"correctly usesredumper_flash_tsstfrom the new TSST flashing module.
139-139: Verify the dump command GENERATE flag change.The
dumpcommand's GENERATE flag changed fromfalsetotrue. This change is not mentioned in the AI summary and appears unrelated to the flashing refactoring. Please confirm this change is intentional.Likely an incorrect or invalid review comment.
options.ixx (1)
354-355: LGTM! Improved command descriptions.The updated descriptions are more concise and grammatically correct, changing from "drive firmware flasher Firmwares" to "flashes drive firmware".
scsi/cmd.ixx (1)
547-555: LGTM! Type names updated consistently.The function correctly uses the renamed types
FLASH_TSST_ModeandCDB12_FlashTSSTto maintain consistency with the changes inscsi/mmc.ixx. The function logic remains unchanged.drive/flash_sd616.ixx (2)
37-37: LGTM: Proper integration with flash_tsst.The call to
flash_tsstcorrectly uses the rearranged firmware data, appropriate block size (64KB), and the correct mode (END_128KB) for SD616 devices.
30-35: Critical: Confirm firmware data rearrangement aligns with SD616 specificationFile: drive/flash_sd616.ixx
Lines: 30–35// rearrange firmware data to match the expected layout std::vector<uint8_t> firmware_data(0x30000); std::copy(firmware_file.begin(), firmware_file.begin() + block_size, firmware_data.begin()); std::copy(firmware_file.begin() + block_size, firmware_file.begin() + block_size * 2 - 0x800, firmware_data.begin() + block_size + 0x400); std::copy(firmware_file.begin() + block_size * 2 - 0x800, firmware_file.begin() + block_size * 2, firmware_data.begin() + block_size * 2); std::copy(firmware_file.begin() + block_size + 0x400, firmware_file.begin() + block_size * 2 - 0x800, firmware_data.begin() + block_size * 2 + 0x800);Summary of observed layout (block_size = 0x10000):
- firmware_data total size: 0x30000 (192 KB)
- Copy 1: [0x00000, 0x10000) → [0x00000, 0x10000)
- Copy 2: [0x10000, 0x1F800) → [0x10400, 0x1FC00) (0xF800 bytes)
- Copy 3: [0x1F800, 0x20000) → [0x20000, 0x20800) (0x0800 bytes)
- Copy 4: [0x10400, 0x1F800) → [0x20800, 0x2FC00) (0xF400 bytes)
This produces three 1 KB gaps at:
• [0x10000, 0x10400)
• [0x1FC00, 0x20000)
• [0x2FC00, 0x30000)Please verify that these offsets, duplicate source ranges, and padding gaps exactly match the SD616 firmware memory map. If they do, add a reference to the layout spec or an inline diagram; otherwise adjust the offsets or merge copy ranges. Also ensure unit tests or validation vectors cover this rearrangement.
drive/flash_tsst.ixx (2)
24-44: LGTM: Well-structured flashing logic with proper error handling.The
flash_tsstfunction implements a robust flashing protocol with:
- Proper chunking of firmware data into configurable blocks
- Correct mode progression (START → CONTINUE → end_mode)
- Comprehensive error handling with SCSI status checks
- Progress logging for user feedback
The logic correctly handles edge cases where firmware size is smaller than block size.
46-56: LGTM: Clean wrapper function.The
redumper_flash_tsstfunction provides a clean interface for MT1339 flashing, properly delegating to the coreflash_tsstfunction with appropriate parameters.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Other