Add flashing for SD-616F/T#267
Conversation
superg
left a comment
There was a problem hiding this comment.
So this is basically MT1339 flasher with different block size and 'custom' END marker.
Instead of copying functionality I suggest to reuse already available routines and just do firmware file preprocessing like I explained in the detailed comments.
Please talk to me if something is unclear, also let me know if you would rather have me separate out the function myself, I can prepare it for you so you just reuse it.
drive/flash_sd616.ixx
Outdated
| memcpy(&shifted_firmware_data, &firmware_data[0], block_size); | ||
| memcpy(&shifted_firmware_data[block_size], &firmware_data[(block_size)-0x400], 0x400); | ||
| memcpy(&shifted_firmware_data[block_size + 0x400], &firmware_data[block_size], block_size - 0x800); | ||
| memcpy(&shifted_firmware_data[block_size * 2], &firmware_data[(block_size * 2) - 0x800], 0x800); | ||
| memcpy(&shifted_firmware_data[block_size * 2 + 0x800], &firmware_data[block_size + 0x400], block_size - 0xc00); |
There was a problem hiding this comment.
This can be simplified to:
memcpy(&shifted_firmware_data[0], &firmware_data[0], block_size);
memcpy(&shifted_firmware_data[block_size], &firmware_data[block_size - 0x400], block_size - 0x400);
memcpy(&shifted_firmware_data[block_size * 2], &firmware_data[block_size * 2 - 0x800], 0x800);
memcpy(&shifted_firmware_data[block_size * 2 + 0x800], &firmware_data[block_size + 0x400], block_size - 0xc00);
There was a problem hiding this comment.
Removed the not needed copy
drive/flash_sd616.ixx
Outdated
| uint32_t offset = 0; | ||
| while(offset < shifted_firmware_data.size()) | ||
| { | ||
| uint32_t offset_next = offset + block_size; | ||
|
|
||
| LOGC_RF("[{:3}%] flashing: [{:08X} .. {:08X})", 100 * offset / (uint32_t)shifted_firmware_data.size(), offset, offset + read_size); | ||
|
|
||
| FLASH_SD616_Mode mode = offset == 0 ? FLASH_SD616_Mode::START : (offset_next < shifted_firmware_data.size() ? FLASH_SD616_Mode::CONTINUE : FLASH_SD616_Mode::END); | ||
|
|
||
| SPTD::Status status = cmd_flash_sd616(*ctx.sptd, &shifted_firmware_data[offset], read_size, 0x01, mode); | ||
| if(status.status_code) | ||
| throw_line("failed to flash firmware, SCSI ({})", SPTD::StatusMessage(status)); | ||
|
|
||
| offset = offset_next; | ||
| } | ||
|
|
||
| LOGC_RF(""); | ||
| LOGC("flashing success"); |
There was a problem hiding this comment.
This is a copy-paste of mt1339 flashing, instead of duplicating the code, separate out the code from redumper_flash_mt1339() function into another function in the same file and reuse this function, something like:
#include <span>
//...
export void flash_mt1339(SPTD &sptd, const std::span<const uint8_t> firmware_data, uint32_t block_size, FLASH_MT1339_Mode end_mode)
{
uint32_t offset = 0;
while(offset < firmware_data.size())
{
uint32_t size = std::min(block_size, (uint32_t)(firmware_data.size() - offset));
uint32_t offset_next = offset + size;
LOGC_RF("[{:3}%] flashing: [{:08X} .. {:08X})", 100 * offset / (uint32_t)firmware_data.size(), offset, offset + size);
FLASH_MT1339_Mode mode = offset == 0 ? FLASH_MT1339_Mode::START : (offset_next < firmware_data.size() ? FLASH_MT1339_Mode::CONTINUE : end_mode);
SPTD::Status status = cmd_flash_mt1339(sptd, &firmware_data[offset], size, 0x01, mode);
if(status.status_code)
throw_line("failed to flash firmware, SCSI ({})", SPTD::StatusMessage(status));
offset = offset_next;
}
LOGC_RF("");
LOGC("flashing success");
}
Now, in your code you call it:
import drive.flash.mt1339;
//...
flash_mt1339(*ctx.sptd, shifted_firmware_data, block_size, FLASH_MT1339_Mode::END_SAMSUNG);
And in original redumper_flash_mt1339() code you call it:
flash_mt1339(*ctx.sptd, firmware_data, 0xFC00, FLASH_MT1339_Mode::END);
There was a problem hiding this comment.
Reworked as two functions calling the flash function
scsi/cmd.ixx
Outdated
| return sptd.sendCommand(&cdb, sizeof(cdb), (void *)data, data_size, true); | ||
| } | ||
|
|
||
| export SPTD::Status cmd_flash_sd616(SPTD &sptd, const uint8_t *data, uint32_t data_size, uint8_t unknown1, FLASH_SD616_Mode mode) |
scsi/mmc.ixx
Outdated
| PLEXTOR_RESET = 0xEE, | ||
| ASUS_READ_CACHE = 0xF1, | ||
| MT1339_FLASH_FIRMWARE = 0xFF | ||
| SAMSUNG_FLASH_FIRMWARE = 0xFF |
There was a problem hiding this comment.
Revert this change, it's still MT1339.
There was a problem hiding this comment.
For what it's worth, other Samsung drives (Kreon has MT1309e) have been able to be flashed using the flash::mt1339 command.
There was a problem hiding this comment.
From what I looked into. Samsung used FF for flashing while LiteOn used DF an HL used FD for flashing the MT1339.
This only works for flashing Samsung drives.
scsi/mmc.ixx
Outdated
| { | ||
| START = 0x00, | ||
| CONTINUE = 0xFF, | ||
| END = 0x02 |
There was a problem hiding this comment.
remove this enum and add END_SAMSUNG = 0x02 to FLASH_MT1339_Mode enum.
scsi/mmc.ixx
Outdated
| END = 0x02 | ||
| }; | ||
|
|
||
| export struct CDB12_FlashSD616 |
drive/flash_sd616.ixx
Outdated
| constexpr uint32_t block_size = 0x10000; | ||
| uint32_t read_size = 0xFFFF; | ||
|
|
||
| std::array<uint8_t, 0x30000> shifted_firmware_data{}; |
There was a problem hiding this comment.
Please change this to std::vector, otherwise it's ~200Kb on the stack.
WalkthroughThis change refactors and generalizes the firmware flashing logic for MT1339 and SD616 drives. It renames modules and types from "mt1339" to "tsst", introduces a modular flashing function supporting multiple end modes, and adds a new flashing routine for SD616 drives. Command registration, help text, and related SCSI command structures are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Redumper
participant DriveFlashTsst
participant SPTD
participant SCSI
User->>Redumper: Selects "flash::mt1339" or "flash::sd616" command
Redumper->>DriveFlashTsst: Calls redumper_flash_mt1339 or redumper_flash_sd616
alt MT1339
DriveFlashTsst->>DriveFlashTsst: Calls flash_mt1339 (block_size=0xFC00, end_mode=END)
else SD616
DriveFlashTsst->>DriveFlashTsst: Rearranges firmware data
DriveFlashTsst->>DriveFlashTsst: Calls flash_mt1339 (block_size=0x10000, end_mode=END_SAMSUNG)
end
loop For each firmware block
DriveFlashTsst->>SPTD: Issue flash command (cmd_flash_tsst)
SPTD->>SCSI: Send command
SCSI-->>SPTD: Status
SPTD-->>DriveFlashTsst: Status
DriveFlashTsst-->>Redumper: Progress/Error
end
Redumper-->>User: Completion/Error message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 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)
🔇 Additional comments (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: 2
♻️ Duplicate comments (7)
scsi/mmc.ixx (3)
31-31: Address previous review feedback on operation code naming.A previous review comment suggested reverting this change to
MT1339_FLASH_FIRMWARE. While the response mentioned Samsung using different operation codes (FF vs DF/FD), please clarify if this rename is necessary or if the existing MT1339 constant should be reused.
836-841: Address previous review feedback on enum consolidation.A previous review comment suggested removing this enum and adding
END_SAMSUNG = 0x02to the existingFLASH_MT1339_Modeenum instead. Please clarify if separate enums are necessary or if consolidation is preferred.
843-854: Address previous review feedback on struct consolidation.A previous review comment suggested removing this struct. Please clarify if the different field layout (particularly the
unknown1field placement) necessitates a separate struct or if the existingCDB12_FlashMT1339can be reused.drive/flash_sd616.ixx (3)
34-34: Move large array from stack to heap.Using
std::array<uint8_t, 0x30000>puts ~192KB on the stack, which should be avoided.-std::array<uint8_t, 0x30000> shifted_firmware_data{}; +std::vector<uint8_t> shifted_firmware_data(0x30000);
35-39: Simplify and fix the data rearrangement logic.The current memcpy operations can be simplified and some appear incorrect compared to the suggested implementation in past reviews.
-memcpy(&shifted_firmware_data, &firmware_data[0], block_size); -memcpy(&shifted_firmware_data[block_size], &firmware_data[(block_size)-0x400], 0x400); -memcpy(&shifted_firmware_data[block_size + 0x400], &firmware_data[block_size], block_size - 0x800); -memcpy(&shifted_firmware_data[block_size * 2], &firmware_data[(block_size * 2) - 0x800], 0x800); -memcpy(&shifted_firmware_data[block_size * 2 + 0x800], &firmware_data[block_size + 0x400], block_size - 0xc00); +memcpy(&shifted_firmware_data[0], &firmware_data[0], block_size); +memcpy(&shifted_firmware_data[block_size], &firmware_data[block_size - 0x400], block_size - 0x400); +memcpy(&shifted_firmware_data[block_size * 2], &firmware_data[block_size * 2 - 0x800], 0x800); +memcpy(&shifted_firmware_data[block_size * 2 + 0x800], &firmware_data[block_size + 0x400], block_size - 0xc00);
41-58: Extract common flashing logic to avoid code duplication.This flashing loop duplicates logic from the MT1339 implementation. Consider extracting the common functionality as suggested in previous reviews.
The past review comment suggested creating a shared
flash_mt1339function that both implementations could use, with different end modes and block sizes. This would reduce code duplication and improve maintainability.scsi/cmd.ixx (1)
557-565: Consider the previous review feedback about necessity.A past review comment indicated this function is not needed. However, this appears to be the main feature for SD-616F/T flashing support as mentioned in the PR objectives. Please verify if this function is actually required or if the flashing can be handled through the existing
cmd_flash_mt1339function.
🧹 Nitpick comments (1)
drive/flash_sd616.ixx (1)
46-46: Use consistent variable in logging.The logging uses
read_sizebut should probably useblock_sizefor clarity and consistency.-LOGC_RF("[{:3}%] flashing: [{:08X} .. {:08X})", 100 * offset / (uint32_t)shifted_firmware_data.size(), offset, offset + read_size); +LOGC_RF("[{:3}%] flashing: [{:08X} .. {:08X})", 100 * offset / (uint32_t)shifted_firmware_data.size(), offset, offset + block_size);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CMakeLists.txt(1 hunks)drive/flash_sd616.ixx(1 hunks)options.ixx(1 hunks)redumper.ixx(2 hunks)scsi/cmd.ixx(1 hunks)scsi/mmc.ixx(2 hunks)
🔇 Additional comments (6)
options.ixx (1)
355-355: LGTM!The help text addition follows the established pattern and clearly describes the new Samsung SD-616F/T drive firmware flasher command.
CMakeLists.txt (1)
140-140: LGTM!The build configuration correctly includes the new flash_sd616.ixx module file, following the established pattern for similar modules.
redumper.ixx (2)
30-30: LGTM!The import statement correctly includes the new drive.flash.sd616 module following the established pattern.
152-152: LGTM!The command registration follows the same pattern as the MT1339 flasher with appropriate settings: requires drive but not ready state, no auto-selection, and no image name requirements, which are all correct for a firmware flashing operation.
drive/flash_sd616.ixx (1)
27-29: LGTM!The firmware reading and size validation logic is correct and appropriate for the expected 0x20000 byte firmware size.
scsi/cmd.ixx (1)
547-555: LGTM! Good refactoring of the flash command.The renaming to
SAMSUNG_FLASH_FIRMWAREand explicit field initialization improve code clarity and maintainability.
drive/flash_sd616.ixx
Outdated
| constexpr uint32_t block_size = 0x10000; | ||
| uint32_t read_size = 0xFFFF; |
There was a problem hiding this comment.
Fix variable inconsistency.
The read_size variable is set to 0xFFFF but block_size is 0x10000. This inconsistency could lead to issues as they're used in different places.
constexpr uint32_t block_size = 0x10000;
-uint32_t read_size = 0xFFFF;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drive/flash_sd616.ixx around lines 31 to 32, the variable read_size is set to
0xFFFF while block_size is 0x10000, causing inconsistency. To fix this, set
read_size to the same value as block_size (0x10000) to ensure consistent usage
and avoid potential issues.
drive/flash_sd616.ixx
Outdated
|
|
||
| FLASH_SD616_Mode mode = offset == 0 ? FLASH_SD616_Mode::START : (offset_next < shifted_firmware_data.size() ? FLASH_SD616_Mode::CONTINUE : FLASH_SD616_Mode::END); | ||
|
|
||
| SPTD::Status status = cmd_flash_sd616(*ctx.sptd, &shifted_firmware_data[offset], read_size, 0x01, mode); |
There was a problem hiding this comment.
Use block_size instead of read_size in flash command.
The command should use block_size (0x10000) rather than read_size (0xFFFF) for consistency with the block-based processing.
-SPTD::Status status = cmd_flash_sd616(*ctx.sptd, &shifted_firmware_data[offset], read_size, 0x01, mode);
+SPTD::Status status = cmd_flash_sd616(*ctx.sptd, &shifted_firmware_data[offset], block_size, 0x01, mode);📝 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.
| SPTD::Status status = cmd_flash_sd616(*ctx.sptd, &shifted_firmware_data[offset], read_size, 0x01, mode); | |
| SPTD::Status status = cmd_flash_sd616(*ctx.sptd, &shifted_firmware_data[offset], block_size, 0x01, mode); |
🤖 Prompt for AI Agents
In drive/flash_sd616.ixx at line 50, replace the use of read_size with
block_size in the call to cmd_flash_sd616 to ensure the flash command uses the
correct block size (0x10000) instead of the inconsistent read_size (0xFFFF).
This aligns the command with the block-based processing approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
drive/flash_tsst.ixx (2)
32-34: Consider using named constants for magic numbers.The magic numbers
0xFC00should be defined as named constants to improve code maintainability and readability.+ constexpr uint32_t DEFAULT_BLOCK_SIZE = 0xFC00; + constexpr uint32_t DEFAULT_READ_SIZE = 0xFC00; + - uint32_t block_size = 0xFC00; - uint32_t read_size = 0xFC00; + uint32_t block_size = DEFAULT_BLOCK_SIZE; + uint32_t read_size = DEFAULT_READ_SIZE;
36-39: Enhance error message with actual firmware size information.The error message could be more helpful by including the actual firmware size and expected sizes.
- throw_line("incorrect firmware size"); + throw_line("incorrect firmware size: {} bytes, expected 0x20000 (128KB) or 0x40000 (256KB)", firmware_data.size());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example.binis excluded by!**/*.bin
📒 Files selected for processing (7)
CMakeLists.txt(1 hunks)drive/flash_mt1339.ixx(0 hunks)drive/flash_tsst.ixx(1 hunks)options.ixx(1 hunks)redumper.ixx(2 hunks)scsi/cmd.ixx(1 hunks)scsi/mmc.ixx(2 hunks)
💤 Files with no reviewable changes (1)
- drive/flash_mt1339.ixx
✅ Files skipped from review due to trivial changes (2)
- CMakeLists.txt
- options.ixx
🚧 Files skipped from review as they are similar to previous changes (3)
- redumper.ixx
- scsi/mmc.ixx
- scsi/cmd.ixx
🔇 Additional comments (3)
drive/flash_tsst.ixx (3)
1-21: LGTM! Clean module structure and comprehensive imports.The module declaration follows C++20 conventions properly, and the imports cover all necessary dependencies for SCSI operations, file I/O, and logging.
57-73: LGTM! Well-structured flashing loop with proper error handling.The main flashing loop is well-implemented with:
- Proper bounds checking using
std::min- Clear progress reporting with percentage and address ranges
- Appropriate mode selection logic
- Good error handling with descriptive SCSI status messages
- Safe offset advancement
75-79: LGTM! Clean success handling and function termination.The success logging and return statement are appropriate and maintain consistency with the function's error handling pattern.
drive/flash_tsst.ixx
Outdated
| if(firmware_data.size() == 0x20000) | ||
| { | ||
| block_size = 0x10000; | ||
| read_size = 0xFFFF; | ||
| end_mode = FLASH_Tsst_Mode::END_128KB; | ||
|
|
||
| std::vector<uint8_t> shifted_firmware_data{}; | ||
| shifted_firmware_data.resize(0x30000); | ||
| std::copy(firmware_data.begin(), std::next(firmware_data.begin(), block_size), shifted_firmware_data.begin()); | ||
| std::copy(std::next(firmware_data.begin(), block_size), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), block_size + 0x400)); | ||
| std::copy(std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(firmware_data.begin(), (block_size * 2)), std::next(shifted_firmware_data.begin(), block_size * 2)); | ||
| std::copy(std::next(firmware_data.begin(), block_size + 0x400), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), (block_size * 2) + 0x800)); | ||
|
|
||
| firmware_data = shifted_firmware_data; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: Verify firmware rearrangement logic and add documentation.
This complex data rearrangement logic lacks documentation and uses multiple magic numbers. The specific offset calculations and copy operations need verification to ensure correctness.
Please verify the firmware rearrangement logic is correct and add documentation explaining:
- Why the 128KB firmware needs to be rearranged into 192KB
- The purpose of each copy operation and offset calculation
- The meaning of magic numbers (0x800, 0x400, etc.)
Consider refactoring with named constants:
+ constexpr uint32_t FIRMWARE_128KB_BLOCK_SIZE = 0x10000;
+ constexpr uint32_t FIRMWARE_128KB_READ_SIZE = 0xFFFF;
+ constexpr uint32_t SHIFTED_FIRMWARE_SIZE = 0x30000;
+ constexpr uint32_t SEGMENT_OFFSET_800 = 0x800;
+ constexpr uint32_t SEGMENT_OFFSET_400 = 0x400;
+
if(firmware_data.size() == 0x20000)
{
- block_size = 0x10000;
- read_size = 0xFFFF;
+ block_size = FIRMWARE_128KB_BLOCK_SIZE;
+ read_size = FIRMWARE_128KB_READ_SIZE;
end_mode = FLASH_Tsst_Mode::END_128KB;
std::vector<uint8_t> shifted_firmware_data{};
- shifted_firmware_data.resize(0x30000);
+ shifted_firmware_data.resize(SHIFTED_FIRMWARE_SIZE);
// Add comments explaining each copy operationRun this script to verify the rearrangement doesn't cause buffer overflows:
🏁 Script executed:
#!/bin/bash
# Verify the firmware rearrangement logic bounds
echo "Analyzing firmware rearrangement bounds:"
echo "Original firmware size: 0x20000 (131072 bytes)"
echo "Shifted firmware size: 0x30000 (196608 bytes)"
echo "Block size: 0x10000 (65536 bytes)"
echo ""
echo "Copy operations analysis:"
echo "1. Copy [0, 0x10000) to [0, 0x10000) - OK"
echo "2. Copy [0x10000, 0x1F800) to [0x10400, 0x1FC00) - Need verification"
echo "3. Copy [0x1F800, 0x20000) to [0x20000, 0x20800) - Need verification"
echo "4. Copy [0x10400, 0x1F800) to [0x20800, 0x2FC00) - Need verification"Length of output: 977
Critical: Document and refactor 128 KB firmware rearrangement in drive/flash_tsst.ixx
The four std::copy operations have been verified to stay within the 0x30000 buffer, but the logic remains opaque and littered with magic numbers. Please:
- Extract all magic values into
constexpr–named constants for block size, read size, total buffer size, and segment offsets. - Add a doc-comment above the
if (firmware_data.size() == …)explaining:- Why 128 KB firmware is expanded to 192 KB.
- What each segment represents.
- The rationale behind each offset (0x800, 0x400, etc.).
- Precede each
std::copywith an inline comment mapping source and destination ranges.
drive/flash_tsst.ixx:41–55
Suggested diff:
+constexpr uint32_t FIRMWARE_128KB_BLOCK_SIZE = 0x10000;
+constexpr uint32_t FIRMWARE_128KB_READ_SIZE = 0xFFFF;
+constexpr uint32_t SHIFTED_FIRMWARE_SIZE = 0x30000;
+constexpr uint32_t SEGMENT_GAP_SMALL = 0x400;
+constexpr uint32_t SEGMENT_GAP_LARGE = 0x800;
+
+// Transform 128 KB layout into 192 KB with interleaved padding:
+if (firmware_data.size() == FIRMWARE_128KB_BLOCK_SIZE * 2)
+{
+ block_size = FIRMWARE_128KB_BLOCK_SIZE;
+ read_size = FIRMWARE_128KB_READ_SIZE;
+ end_mode = FLASH_Tsst_Mode::END_128KB;
+
+ std::vector<uint8_t> shifted_firmware_data(SHIFTED_FIRMWARE_SIZE);
+ // 1) Copy first 0x10000 bytes to [0x00000…0x10000)
+ std::copy(firmware_data.begin(),
+ firmware_data.begin() + block_size,
+ shifted_firmware_data.begin());
+
+ // 2) Copy next (2*block_size – SEGMENT_GAP_LARGE) bytes to [0x10400…0x1FC00)
+ std::copy(firmware_data.begin() + block_size,
+ firmware_data.begin() + (block_size * 2) - SEGMENT_GAP_LARGE,
+ shifted_firmware_data.begin() + block_size + SEGMENT_GAP_SMALL);
+
+ // 3) Copy last SEGMENT_GAP_LARGE bytes of original to [0x20000…0x20800)
+ std::copy(firmware_data.begin() + (block_size * 2) - SEGMENT_GAP_LARGE,
+ firmware_data.begin() + (block_size * 2),
+ shifted_firmware_data.begin() + (block_size * 2));
+
+ // 4) Fill the gap: copy from [0x10400…0x1F800) into [0x20800…0x2FC00)
+ std::copy(firmware_data.begin() + block_size + SEGMENT_GAP_SMALL,
+ firmware_data.begin() + (block_size * 2) - SEGMENT_GAP_LARGE,
+ shifted_firmware_data.begin() + (block_size * 2) + SEGMENT_GAP_LARGE);
+
+ firmware_data = std::move(shifted_firmware_data);
+}🤖 Prompt for AI Agents
In drive/flash_tsst.ixx around lines 41 to 55, refactor the 128 KB firmware
rearrangement by replacing all magic numbers with clearly named constexpr
constants for block size, read size, total buffer size, and segment offsets. Add
a detailed doc-comment above the if statement explaining why the 128 KB firmware
is expanded to 192 KB, describing what each segment represents, and clarifying
the rationale behind each offset like 0x800 and 0x400. Additionally, add inline
comments before each std::copy call to explicitly map the source and destination
ranges being copied to improve code clarity and maintainability.
superg
left a comment
There was a problem hiding this comment.
I've just looked over it:
- we shouldn't delete flash::mt1339 command as it will require README and redump.org wiki updates
- you cannot make decision on how to flash based on the firmware file size, that would be a critical mistake if you try to flash 128Kb drive with 256Kb firmware because if there are no any protection mechanisms in place, you will brick the drive.
Please make the changes exactly how I asked in the first review, I will tidy it up and make renames as I see fit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
drive/flash_mt1339.ixx (1)
6-6: Remove unused includeThe
<iostream>header appears to be unused in this module. All logging is performed through the importedutils.loggermodule.-#include <iostream>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(1 hunks)drive/flash_mt1339.ixx(4 hunks)options.ixx(1 hunks)redumper.ixx(3 hunks)scsi/mmc.ixx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CMakeLists.txt
- options.ixx
- redumper.ixx
- scsi/mmc.ixx
🔇 Additional comments (3)
drive/flash_mt1339.ixx (3)
27-52: Well-structured refactoring for multi-device supportThe generalization of the flashing logic with configurable block size and end mode parameters enables support for multiple device variants while maintaining clean, reusable code.
54-57: LGTM! Maintains backward compatibilityThe wrapper function preserves the original interface while leveraging the new generic implementation.
64-64: Verify uninitialized buffer region is intentionalThe buffer is allocated with 0x30000 bytes but only 0x2FF00 bytes are written, leaving the last 0x100 bytes uninitialized (zero). Please confirm this padding is intentional for SD616 devices.
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)
drive/flash_mt1339.ixx(4 hunks)options.ixx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- options.ixx
🔇 Additional comments (4)
drive/flash_mt1339.ixx (4)
4-12: LGTM! Module refactoring aligns with generalization.The additional includes support the new data manipulation operations in
redumper_flash_sd616, and the module rename frommt1339totsstappropriately reflects the generalization to support multiple TSST drive types.
27-52: Excellent refactoring! Generic function improves code reusability.The extraction of common flashing logic into a parameterized function is well-designed. The dynamic mode selection correctly handles the three phases (START, CONTINUE, end_mode), and the use of
std::spanprovides safe access to firmware data without copying.
54-57: LGTM! Clean wrapper preserves backward compatibility.The thin wrapper approach maintains the existing interface while leveraging the new generic implementation. The parameters (0xFC00 block size, END mode) match the original hardcoded behavior.
63-64: Good! Bounds checking addresses previous security concern.The exact size validation properly addresses the previous review comment about missing bounds checking, preventing potential out-of-bounds access.
| std::vector<uint8_t> shifted_firmware_data{}; | ||
| shifted_firmware_data.resize(0x30000); | ||
| std::copy(firmware_data.begin(), std::next(firmware_data.begin(), block_size), shifted_firmware_data.begin()); | ||
| std::copy(std::next(firmware_data.begin(), block_size), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), block_size + 0x400)); | ||
| std::copy(std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(firmware_data.begin(), (block_size * 2)), std::next(shifted_firmware_data.begin(), block_size * 2)); | ||
| std::copy(std::next(firmware_data.begin(), block_size + 0x400), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), (block_size * 2) + 0x800)); | ||
| return flash_mt1339(*ctx.sptd, shifted_firmware_data, block_size, FLASH_Tsst_Mode::END_SAMSUNG); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for complex data rearrangement logic.
The firmware data rearrangement using multiple std::copy operations with magic numbers (0x800, 0x400) is difficult to understand without context. Consider adding comments explaining:
- Why SD616 requires this specific 128KB → 192KB transformation
- What each copy operation accomplishes
- The meaning of the magic offsets
+ // SD616 requires firmware data to be rearranged from 128KB to 192KB format:
+ // - First 64KB block: copy as-is
+ // - Second 64KB block: split and rearrange with specific offsets
std::vector<uint8_t> shifted_firmware_data{};
shifted_firmware_data.resize(0x30000);
+
+ // Copy first 64KB block unchanged
std::copy(firmware_data.begin(), std::next(firmware_data.begin(), block_size), shifted_firmware_data.begin());
+ // Copy second block segment with offset adjustments for SD616 format
std::copy(std::next(firmware_data.begin(), block_size), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), block_size + 0x400));📝 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.
| std::vector<uint8_t> shifted_firmware_data{}; | |
| shifted_firmware_data.resize(0x30000); | |
| std::copy(firmware_data.begin(), std::next(firmware_data.begin(), block_size), shifted_firmware_data.begin()); | |
| std::copy(std::next(firmware_data.begin(), block_size), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), block_size + 0x400)); | |
| std::copy(std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(firmware_data.begin(), (block_size * 2)), std::next(shifted_firmware_data.begin(), block_size * 2)); | |
| std::copy(std::next(firmware_data.begin(), block_size + 0x400), std::next(firmware_data.begin(), (block_size * 2) - 0x800), std::next(shifted_firmware_data.begin(), (block_size * 2) + 0x800)); | |
| return flash_mt1339(*ctx.sptd, shifted_firmware_data, block_size, FLASH_Tsst_Mode::END_SAMSUNG); | |
| // SD616 requires firmware data to be rearranged from 128KB to 192KB format: | |
| // - First 64KB block: copy as-is | |
| // - Second 64KB block: split and rearrange with specific offsets | |
| std::vector<uint8_t> shifted_firmware_data{}; | |
| shifted_firmware_data.resize(0x30000); | |
| // Copy first 64KB block unchanged | |
| std::copy(firmware_data.begin(), | |
| std::next(firmware_data.begin(), block_size), | |
| shifted_firmware_data.begin()); | |
| // Copy second block segment with offset adjustments for SD616 format | |
| std::copy(std::next(firmware_data.begin(), block_size), | |
| std::next(firmware_data.begin(), (block_size * 2) - 0x800), | |
| std::next(shifted_firmware_data.begin(), block_size + 0x400)); | |
| std::copy(std::next(firmware_data.begin(), (block_size * 2) - 0x800), | |
| std::next(firmware_data.begin(), (block_size * 2)), | |
| std::next(shifted_firmware_data.begin(), block_size * 2)); | |
| std::copy(std::next(firmware_data.begin(), block_size + 0x400), | |
| std::next(firmware_data.begin(), (block_size * 2) - 0x800), | |
| std::next(shifted_firmware_data.begin(), (block_size * 2) + 0x800)); | |
| return flash_mt1339(*ctx.sptd, | |
| shifted_firmware_data, | |
| block_size, | |
| FLASH_Tsst_Mode::END_SAMSUNG); |
🤖 Prompt for AI Agents
In drive/flash_mt1339.ixx around lines 66 to 72, the multiple std::copy
operations rearranging firmware_data use unexplained magic numbers and complex
indexing, making the logic hard to understand. Add clear comments explaining why
the SD616 requires this specific 128KB to 192KB transformation, describe the
purpose of each std::copy call, and clarify the meaning of the magic offsets
like 0x800 and 0x400 to improve code readability and maintainability.
Summary by CodeRabbit
New Features
Improvements