Test OmniDrive FW #332
Test OmniDrive FW #332
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds OmniDrive/raw DVD support, Nintendo-aware descrambling, new DVD/galois/scrambler modules, OmniDrive SCSI command, options and context flags, build updates, and extensive dump/split logic to handle raw vs FORM1 sector sizing and descrambling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dump as DVD Dump
participant SPTD as OmniDrive / SPTD
participant File as Filesystem
participant Split as DVD Split
participant Scrambler as DVD Scrambler
User->>Dump: start dump(ctx, options)
alt raw mode / OmniDrive
Dump->>SPTD: cmd_read_omnidrive(buffer, block_size, address, len, disc_type, raw_addressing, fua, descramble, subch, c2)
else FORM1 standard
Dump->>SPTD: standard read (FORM1-sized)
end
SPTD-->>Dump: sector blocks
Dump->>File: write .sdram/.raw or .iso using sector_size and lba_index
Dump-->>User: dump complete
User->>Split: run split/descramble (ctx.nintendo or dvd_raw)
Split->>File: read dumped sectors (sector_size-aware)
loop per-sector
Split->>Scrambler: descramble(sector, psn, key?)
Scrambler-->>Split: descrambled sector + success
Split->>File: write .iso user data / update metadata
end
Split-->>User: split/descramble complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dvd/dvd_dump.ixx (1)
991-1003: Filesystem search skips Nintendo discs due to sector format incompatibility.When
nintendois true,sector_sizebecomes 2064 (raw sector with header/ECC), butfilesystem_search_sizeassumes 2048-byte sectors with filesystem descriptors at offset 0. Nintendo raw sectors have descriptors at offset 12, causing the search to fail silently. While this prevents crashes, it disables filesystem trimming without explicit intent.Consider adding an explicit guard to skip filesystem search for Nintendo discs, or extract the 2048 bytes of actual data before passing to
filesystem_search_size.
🤖 Fix all issues with AI agents
In @.github/workflows/cmake.yml:
- Line 5: Restore the workflow's branch filter under the branches key to include
main (e.g., branches: [main]) instead of the temporary omnidrive-only override
and update the concurrency group string (the concurrency: group: value that
currently references 'main') so it consistently uses the intended branch or a
neutral identifier (e.g., use github.ref or 'omnidrive' if this workflow will
remain branch-specific); revert the temporary change before merging to main to
ensure pushes/releases from main still trigger the workflow.
In `@dvd/dvd_dump.ixx`:
- Around line 522-525: Remove the unconditional assignment that forces Nintendo
mode: delete the line "ctx.nintendo = true;" so the code respects the actual
optional flag instead of forcing OmniDrive/Nintendo paths; keep the existing
"bool nintendo = ctx.nintendo && *ctx.nintendo;" and, if needed, set
ctx.nintendo only when a proper detection/gating routine (e.g., an omnidrive or
detect_nintendo check) determines the disc is Nintendo rather than
unconditionally here.
In `@dvd/dvd_raw.ixx`:
- Line 25: Replace the unsigned 32-bit type with a signed 32-bit type for the
negative DVD offset: change the declaration of DVD_LBA_START to use int32_t
(keep constexpr and the same negative value -0x30000) so the constant's
signedness matches the CD code pattern (e.g., cd_common's int32_t LBA_START) and
removes the signedness inconsistency.
In `@dvd/dvd_scrambler.ixx`:
- Around line 69-73: The XOR-table wrap logic in dvd_scrambler.ixx incorrectly
subtracts (_TABLE_LENGTH - 1) causing an off-by-one shift; update the wrap
inside the loop that computes index (where index = offset + i - main_data_offset
and output[i] = data[i] ^ _TABLE[index]) to wrap correctly by subtracting
_TABLE_LENGTH (or use index %= _TABLE_LENGTH) when index >= _TABLE_LENGTH so
indices map back into the table without the +1 shift.
In `@dvd/dvd_split.ixx`:
- Around line 136-147: The output ISO is writing from sector.data() + 6 which
incorrectly includes CPR_MAI and drops the last 6 bytes of user payload; change
the write to use the DataFrame payload offset (write from sector.data() +
DataFrame::main_data and write FORM1_DATA_SIZE bytes) so the full main_data is
preserved, and add `#include` <cstddef> if missing; locate this in the loop that
reads into sector (std::vector<uint8_t> sector(DATA_FRAME_SIZE)), the
scrambler.descramble call, and the ofs.write invocation to update the offset.
- Around line 139-153: The branch that uses the “first ECC block” key is
unreachable because psn is an absolute value (initialized relative to
DVD_LBA_START) but is compared directly to ECC_FRAMES; change the condition to
use an LBA-relative counter: compute the logical LBA as (psn + DVD_LBA_START) or
maintain a separate counter (e.g., lba starting at 0 and incrementing each loop)
and replace the test if(psn < ECC_FRAMES) with if((psn + DVD_LBA_START) <
ECC_FRAMES) or if(lba < ECC_FRAMES) so scrambler.descramble(...) uses (psn >> 4
& 0xF) for the first ECC blocks and falls back to key afterward (affecting
variables psn, DVD_LBA_START, ECC_FRAMES, and the scrambler.descramble calls).
- Around line 135-155: The descramble calls on DVD_Scrambler
(scrambler.descramble(...)) can return false and leave the buffer scrambled, so
update dvd_split.ixx to check the boolean return value of both the initial
descramble(sector.data(), psn, DATA_FRAME_SIZE, 0) and the per-frame descramble
inside the loop (the calls that use psn >> 4 & 0xF or key) and never call
ofs.write((char *)(sector.data() + 6), ...) if descramble returned false;
instead handle failure by reporting/logging an error and aborting the split (or
returning an error code) to avoid writing corrupted data. Ensure the checks
reference the exact scrambler.descramble calls shown so they’re applied to both
the initial and looped descramble invocations.
In `@scsi/mmc.ixx`:
- Around line 881-896: The CDB12_ReadOmniDrive bitfields after transfer_blocks
currently sum to 9 bits (subchannels :2, c2 :1, reserved2 :6) which overflows
into a second byte and shifts control, making the CDB 13 bytes; fix by reducing
reserved2 to 5 bits so subchannels :2, c2 :1, reserved2 :5 occupy exactly one
byte and preserve the 12‑byte CDB (update the definition in struct
CDB12_ReadOmniDrive to use reserved2 :5).
superg
left a comment
superg
left a comment
There was a problem hiding this comment.
Overall looks promising.
On a high level yes, we will need a different extension for scrambled DVD file.
Left some comments.
dvd/dvd_dump.ixx
Outdated
| auto status = cmd_read(*ctx.sptd, drive_data.data(), FORM1_DATA_SIZE, lba + lba_shift, sectors_to_read, dump_mode == DumpMode::REFINE && refine_counter); | ||
| SPTD::Status status; | ||
| if(nintendo) | ||
| status = cmd_read_omnidrive(*ctx.sptd, drive_data.data(), DATA_FRAME_SIZE, lba + lba_shift, sectors_to_read, OmniDrive_DiscType::DVD); |
There was a problem hiding this comment.
As I mentioned on #redumper, what if we always use RECORDING_FRAME_SIZE but not filling the extra data if drive doesn't support it? (or filling it with generated data). This would align well with CD reading, read_sector_new() function internally does a bunch of things such as rearrange sector layout depending on what drive supports and rescrambles data if generic drive was used.
This will allow us to have one data format.
There was a problem hiding this comment.
This is a good idea, I will need help rearranging the dvd_dump main loop to better deal with the different formats.
I will first move cmd_read_omnidrive() to a separate read_raw() command that can later include reading from cache.
There was a problem hiding this comment.
The issue with this, is that the recording frame has the user data interleaved, which complicates a lot of code. Every 172 bytes of user data is followed by 10 bytes of parity data, and then after 12 of these rows, another 182 bytes of parity data. It is much easier dealing with data frames when we don't have to worry about the ECC.
We could instead save the DVD ECC data to a different file, rather than trying to keep the recording frames in the correct order. I don't think we have any drives that can get untouched ECC bytes anyway, some are rearranged and all(?) have processed ECC bytes.
There was a problem hiding this comment.
I don't like having a different file for ECC, it doesn't make sense. For CD we don't put data and ECC/EDC in separate files.
What's complicated, calculation of parity data? I think we can just leave zeroes in those portions for now and account for that.
There was a problem hiding this comment.
The complicated/annoying part is having to store and then read the user data bytes non-contiguously. My fork currently operates on everything being data frames. If the data frame is fragmented, simple operations require helper functions, and harder operations like descrambling become a nightmare.
The reason for a separate file is that we currently have no method for reading ECC as it is on-disc, the data we receive is post-processed / 'incorrect'. So all the effort of handling it correctly in redumper code is wasted.
There was a problem hiding this comment.
My suggestion is to stored the cache as-is (however it is laid out in the cache, interleaved with ECC or otherwise), and then the cache gets processed into data frames. These .cache files would be chipset-specific. It's better this way as the data we get does represents cache, not on-disc data.
We can store the recording frames correctly if OmniDrive fw gets a command to dump them properly.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@dvd/dvd_raw.ixx`:
- Around line 28-33: read_raw_dvd currently always calls cmd_read_omnidrive
without checking drive capability; add a guard that verifies the SPTD/drive
supports OmniDrive commands before calling cmd_read_omnidrive (e.g. via a
capability query on ctx.sptd or an existing isOmniDrive/is_omnidrive_capable
helper), and if the drive is not OmniDrive return an appropriate SPTD::Status
error instead of issuing the command; update read_raw_dvd (and/or the caller
redumper_dump_dvd if you prefer call-site guarding) to perform this check so
cmd_read_omnidrive is only invoked for OmniDrive-capable devices.
In `@dvd/dvd_scrambler.ixx`:
- Around line 52-57: The EDC verification reads frame->edc (DataFrame::edc at
offset 2060) unguarded and can access past the provided buffer when size <
offsetof(DataFrame, edc)+sizeof(edc); before calling endian_swap(frame->edc) and
DVD_EDC().update(...).final() add a size check mirroring process(): ensure size
>= offsetof(DataFrame, edc) + sizeof(frame->edc) (or >= offsetof(DataFrame,
edc)+4) and only perform the EDC check/assign unscrambled if that condition
holds; otherwise skip the EDC check so process(sector, sector, offset, size)
still handles scrambling for small buffers.
In `@dvd/dvd_split.ixx`:
- Around line 143-153: The inner declaration "bool success" inside the Nintendo
branch shadows the outer "bool success" declared earlier (so the outer variable
is unused); remove the inner "bool" and assign the result of
scrambler.descramble(...) to the existing success variable instead so later
logic uses the actual outcome. Locate the Nintendo conditional that uses
ctx.nintendo, main_data_offset, raw_fs.read, scrambler.descramble(sector.data(),
psn, key) and replace the shadowing declaration with an assignment to the outer
success variable.
♻️ Duplicate comments (1)
dvd/dvd_split.ixx (1)
168-171: Arithmetic error: condition uses subtraction instead of addition.The condition
psn - DVD_LBA_START < ECC_FRAMESis incorrect. Sincepsnstarts at-DVD_LBA_START, the LBA ispsn + DVD_LBA_START. The condition should use addition to check if LBA < ECC_FRAMES.🔧 Proposed fix
- if(nintendo && psn - DVD_LBA_START < ECC_FRAMES) + if(nintendo && psn + DVD_LBA_START < ECC_FRAMES)
🧹 Nitpick comments (1)
dvd/dvd_split.ixx (1)
162-178: Loop processes sector before checking if read succeeded.The current structure calls
descramble()on the buffer before checkingbytesRead. At EOF, this processes stale data and may log a spurious warning before returning. Consider restructuring to check the read result earlier.♻️ Suggested restructure
while(true) { raw_fs.read((char *)sector.data(), sector.size()); + bytesRead = raw_fs.gcount(); + if(bytesRead != sector.size()) + return; + psn += 1; // first ECC block has key (psn >> 4 & 0xF) // pressed discs have no key set during lead-in/lead-out - if(nintendo && psn - DVD_LBA_START < ECC_FRAMES) + if(nintendo && psn + DVD_LBA_START < ECC_FRAMES) success = scrambler.descramble(sector.data(), psn, psn >> 4 & 0xF); else success = scrambler.descramble(sector.data(), psn, key); if(!success) LOG("warning: descramble failed (LBA: {})", psn + DVD_LBA_START); - bytesRead = raw_fs.gcount(); - if(bytesRead == sector.size()) - iso_fs.write((char *)(sector.data() + main_data_offset), FORM1_DATA_SIZE); - else - return; + iso_fs.write((char *)(sector.data() + main_data_offset), FORM1_DATA_SIZE); }
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dvd/dvd_split.ixx`:
- Around line 160-171: The lead‑in descramble currently passes 0 to
scrambler.descramble which selects the keyed path despite the comment; change
the call to pass std::nullopt (or the function's equivalent no-key sentinel) so
scrambler.descramble uses the keyless path when deriving key from the sector,
e.g. update the call in the block that sets main_data_offset and calls
scrambler.descramble(psn, ...) to use std::nullopt and ensure the surrounding
code that computes key from sector (key, sum) remains unchanged.
♻️ Duplicate comments (2)
dvd/dvd_split.ixx (1)
166-190: Don’t write output after a failed descramble.
This still logs and continues to write data even when descramble fails. Same issue as previously reported.dvd/dvd_scrambler.ixx (1)
32-57: Guard EDC check to avoid out‑of‑bounds reads.
Same issue as previously reported:frame->edcis read without validatingsizeagainst the EDC offset.🔧 Proposed fix
- if(endian_swap(frame->edc) == DVD_EDC().update(sector, offsetof(DataFrame, edc)).final()) - unscrambled = true; + const uint32_t edc_offset = offsetof(DataFrame, edc); + if(size < edc_offset + sizeof(frame->edc)) + return true; + + if(endian_swap(frame->edc) == DVD_EDC().update(sector, edc_offset).final()) + unscrambled = true;
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drive.ixx (1)
313-317: Propagate OmniDrive info intoDriveConfig.
drive_get_configdrops the parsed OmniDrive data, so downstream gating/logging never triggers.🛠️ Proposed fix
drive_config.vendor_id = drive_query.vendor_id; drive_config.product_id = drive_query.product_id; drive_config.product_revision_level = drive_query.product_revision_level; drive_config.vendor_specific = drive_query.vendor_specific; + drive_config.omnidrive = drive_query.omnidrive;
🤖 Fix all issues with AI agents
In `@dvd/dvd_dump.ixx`:
- Around line 811-812: Pre-read uses read_entry(..., index = raw ? DVD_LBA_START
: 0, ...) which with DVD_LBA_START = -196608 gets clamped to 0 and fills
state_buffer with State::ERROR_SKIP, while later writes use index = lba -
DVD_LBA_START (positive offsets), so pre-read never matches written state; fix
by making the index domain consistent: change the pre-read call to use the same
shifted index used by the writer (e.g., use index = raw ?
(DVD_LBA_START_SHIFTED_OR_ADD) : 0), or convert both read_entry and the writer
to use signed offsets so the pre-read uses (lba - DVD_LBA_START) and will load
real data into state_buffer; update references in read_entry, the write loop
(index = lba - DVD_LBA_START), and any checks that count State::ERROR_SKIP
(errors.scsi) so they operate on the same indexing scheme.
In `@redumper.ixx`:
- Around line 456-458: The LOG call using ctx.drive_config.omnidrive currently
extracts only 4-bit nibbles (masks 0xF00/0xF0/0xF) producing wrong version
parts; update the extraction in that LOG statement to pull the three full bytes
from omnidrive by right-shifting the value to get the high/mid/low bytes and
masking each with 0xFF so the version prints as byte9.byte10.byte11 (refer to
ctx.drive_config.omnidrive and the LOG(...) line to modify).
In `@systems/gc.ixx`:
- Around line 32-49: In printInfo, avoid casting the unaligned header bytes to
Header*; instead read into header_data then std::memcpy into a locally declared
Header instance (e.g., Header header; std::memcpy(&header, header_data.data(),
sizeof(Header))) and use that local aligned header for checks against _GC_MAGIC
and field access; also ensure numeric uint8_t fields (header.disc_version,
header.disc_number) are cast to int when passed to std::format so they print as
numbers, and keep existing uses of normalize_string/erase_all for disc_id and
title.
In `@systems/wii.ixx`:
- Around line 32-49: In printInfo, avoid casting header_data.data() to Header*
(violates alignment/aliasing); instead create a local Header variable and use
std::memcpy to copy sizeof(Header) bytes from header_data.data() into that local
Header, then reference the local Header (e.g., header.disc_version,
header.wii_magic, header->disc_id etc. -> header.disc_id) for all subsequent
checks and prints; ensure you still validate header.wii_magic before using other
fields.
♻️ Duplicate comments (2)
dvd/dvd_scrambler.ixx (1)
52-53: Guard EDC access for short buffers.
frame->edcis read without ensuringsize >= offsetof(DataFrame, edc) + 4, so partial buffers can trigger out‑of‑bounds reads.🔧 Proposed fix
- if(endian_swap(frame->edc) == DVD_EDC().update(sector, offsetof(DataFrame, edc)).final()) - unscrambled = true; + const uint32_t edc_offset = offsetof(DataFrame, edc); + if(size < edc_offset + sizeof(frame->edc)) + return true; + + if(endian_swap(frame->edc) == DVD_EDC().update(sector, edc_offset).final()) + unscrambled = true;dvd/dvd_split.ixx (1)
166-190: Don’t write ISO data when descramble fails.
descramble()can return false and re‑scramble the sector, but the code still writes the payload. This can silently corrupt.iso.🔧 Proposed fix
- if(!success) - LOG("warning: descramble failed (LBA: {})", psn + DVD_LBA_START); - iso_fs.write((char *)(sector.data() + main_data_offset), FORM1_DATA_SIZE); + if(!success) + throw_line("descramble failed (LBA: {})", psn + DVD_LBA_START); + iso_fs.write((char *)(sector.data() + main_data_offset), FORM1_DATA_SIZE);
🧹 Nitpick comments (1)
dvd/dvd_scrambler.ixx (1)
45-48: Make the Nintendo offset integer‑explicit (avoid FP truncation).
+ 7.5promotes to double and truncates on assignment. If the half‑frame shift is intended, encode it explicitly in integer math (and verify intent).♻️ Proposed fix
- if(key) - offset = ((*key ^ (psn >> 4 & 0xF)) + 7.5) * FORM1_DATA_SIZE; + if(key) + { + constexpr uint32_t half_frame = FORM1_DATA_SIZE / 2; + offset = ((*key ^ (psn >> 4 & 0xF)) + 7) * FORM1_DATA_SIZE + half_frame; + }
superg
left a comment
superg
left a comment
There was a problem hiding this comment.
Partial review, I'm not done, need to go over dvd_dump.ixx and dvd_split.ixx.
dvd/dvd.ixx
Outdated
|
|
||
|
|
||
| export constexpr int32_t DVD_LBA_START = -0x30000; | ||
| export constexpr uint32_t DATA_FRAME_SIZE = 2064; |
There was a problem hiding this comment.
Is this export needed? It's sizeof(DataFrame)
dvd/dvd.ixx
Outdated
|
|
||
| export constexpr int32_t DVD_LBA_START = -0x30000; | ||
| export constexpr uint32_t DATA_FRAME_SIZE = 2064; | ||
| export constexpr uint32_t RECORDING_FRAME_SIZE = 2366; |
There was a problem hiding this comment.
Same here, it's sizeof(RecordingFrame)
| auto image_prefix = (std::filesystem::path(options.image_path) / options.image_name).generic_string(); | ||
|
|
||
| if(std::filesystem::exists(image_prefix + ".iso")) | ||
| if(std::filesystem::exists(image_prefix + ".iso") || std::filesystem::exists(image_prefix + ".raw")) |
There was a problem hiding this comment.
.raw means unstructured data, there are countless of .raw files with all kind of structured data inside.
Use .sdram here and in all other places.
There was a problem hiding this comment.
Yeah it was just a placeholder, .raw is also used for raw photo files. I'll use sdram
| }; | ||
|
|
||
|
|
||
| export struct RecordingFrame |
There was a problem hiding this comment.
We need the following functions:
- DataFrame RecordingFrame_to_DataFrame(const RecordingFrame &recording_frame);
- RecordingFrame DataFrame_to_RecordingFrame(const DataFrame &data_frame); // doesn't have to calculate parity now but definitely will need that in the future
- bool RecordingFrameValid(const RecordingFrame &recording_frame); // returns true if parity matches and false otherwise
.sdram file is a concatenation of RecordingFrames, starting from whatever is the earliest theoretical PSN addressing, similar to .scram. If drive returns DataFrame, it should be converted to RecordingFrame using these function before store, and vice versa.
I can write those conversion functions, that doesn't look complicated to me.
There was a problem hiding this comment.
I suggest that this PR gets merged into a branch on your repo, so you can work on these changes. Then, I will test it all before merging into main redumper.
dvd/dvd.ixx
Outdated
| struct Row | ||
| { | ||
| uint8_t main_data[172]; | ||
| uint8_t parity_inner[10]; | ||
| }; | ||
|
|
||
| Row row[12]; |
There was a problem hiding this comment.
It can just be:
struct
{
// ...
} row[12];
If Row type never used outside.
drive.ixx
Outdated
| drive_query.vendor_specific = normalize_string(std::string((char *)inquiry_data.vendor_specific, sizeof(inquiry_data.vendor_specific))); | ||
|
|
||
| std::string reserved5 = normalize_string(std::string((char *)inquiry_data.reserved5, sizeof(inquiry_data.reserved5))); | ||
| if(reserved5.compare(0, 9, "OmniDrive") == 0) |
There was a problem hiding this comment.
this logic should go to a function:
export bool is_omnidrive_firmware(const DriveConfig &drive_config) in drive.ixx
No decisions should be made based on omnidrive version (I do not want any branching in redumper code "if version is x.x.x than this is supported and this is not") although printing the version is fine.
(implementing this requires an answer to the previous comment)
There was a problem hiding this comment.
OmniDrive firmware will have a good base feature set of GC/Wii/Xbox/Xbox360/XboxOne/WiiU (and raw reading of CD/DVD/BD). If there are future versions, they are most likely for performance or bug fixes (which redumper does not need to check for). However, a future version may add support for other discs such as Dreamcast and SACD, which would need to be version-gated if requiring custom commands. If you prefer not storing the version, in the future it can be is_omnidrive2_firmware() or similar.
drive.ixx
Outdated
| ReadMethod read_method; | ||
| SectorOrder sector_order; | ||
| Type type; | ||
| std::optional<uint32_t> omnidrive; |
There was a problem hiding this comment.
I don't like having this here, need to think how to properly propagate reserved5 all the way to this structure without modifying drive table.
There was a problem hiding this comment.
reserved5 will need to be non-normalized string as the version field contains 0x01, 0x00 for version numbers.
I can store an optional reserved5 string there if it has any non-zero bytes.
options.ixx
Outdated
| bool mediatek_skip_leadout; | ||
| int mediatek_leadout_retries; | ||
| bool kreon_partial_ss; | ||
| bool raw_dvd; |
There was a problem hiding this comment.
Please change it to dvd_raw and --dvd-raw respectively.
redumper.ixx
Outdated
|
|
||
| ctx.disc_type = options.disc_type ? string_to_enum(*options.disc_type, DISC_TYPE_STRING) : profile_to_disc_type(current_profile); | ||
|
|
||
| if(ctx.drive_config.omnidrive) |
There was a problem hiding this comment.
Let's have omnidrive_version() function.
if(is_omnidrive_firmware(ctx.drive_config))
LOG("custom firmware: OmniDrive v{}", omnidrive_version(ctx.drive_config));
| // TODO: gate behind OmniDrive check | ||
| return cmd_read_omnidrive(*ctx.sptd, buffer, block_size, address, transfer_length, OmniDrive_DiscType::DVD, raw_addressing, force_unit_access, false, OmniDrive_Subchannels::NONE, false); | ||
|
|
||
| // TODO: read raw DVD from cache (cmd_mediatek_read_cache) |
There was a problem hiding this comment.
If I understand correctly, this is another way of reading, this should come in another MR.
There was a problem hiding this comment.
Yes, I've left it there to explain why this DVD raw read command needed its own file. I tried to keep it generic to allow for easy extending to other drives/commands later.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drive.ixx (1)
313-318: Missingreserved5propagation toDriveConfig.The
reserved5field is parsed intoDriveQuery(line 274) but never copied toDriveConfigindrive_get_config. This meansdrive_config.reserved5will always bestd::nullopt, causingis_omnidrive_firmware()to always returnfalse.🐛 Proposed fix
drive_config.vendor_id = drive_query.vendor_id; drive_config.product_id = drive_query.product_id; drive_config.product_revision_level = drive_query.product_revision_level; drive_config.vendor_specific = drive_query.vendor_specific; + if(!drive_query.reserved5.empty()) + drive_config.reserved5 = drive_query.reserved5; return drive_config;
🤖 Fix all issues with AI agents
In `@dvd/dvd_dump.ixx`:
- Around line 840-844: The validation has a copy-paste bug: in the DVD dump lba
checks (around the block that throws via throw_line and assigns lba_end) the
second condition incorrectly tests *options.lba_start against DVD_LBA_START;
update that condition to check *options.lba_end instead so the non-raw and
raw-boundary validations validate lba_end correctly, keeping the existing
throw_line message and assignment to lba_end.
♻️ Duplicate comments (2)
dvd/dvd_raw.ixx (1)
28-34: Address the OmniDrive capability check TODO before merging.As noted in past reviews,
read_dvd_rawunconditionally callscmd_read_omnidrivewithout verifying the drive supports OmniDrive commands. Non-OmniDrive drives will fail with SCSI errors. The TODO on line 30 should be resolved to either:
- Add a guard that checks
is_omnidrive_firmware(ctx.drive_config)before calling the command- Ensure the caller (
dvd_dump.ixx) gates access appropriatelydvd/dvd_scrambler.ixx (1)
52-57: Guard the EDC check to prevent out-of-bounds reads.When
size < offsetof(DataFrame, edc) + sizeof(frame->edc), accessingframe->edcat line 52 reads past the buffer. The initial validation at line 33 only ensuressize >= 6bytes. Add a size guard before the EDC verification.🔧 Proposed fix
+ constexpr uint32_t edc_end = offsetof(DataFrame, edc) + sizeof(frame->edc); + if(size < edc_end) + return unscrambled; + if(endian_swap(frame->edc) == DVD_EDC().update(sector, offsetof(DataFrame, edc)).final()) unscrambled = true;
🧹 Nitpick comments (3)
dvd/dvd_raw.ixx (1)
21-25: Unused structMediatekCacheFrame.This struct is defined but never referenced in the file. If it's intended for future use (the TODO on line 33), consider removing it until needed to avoid dead code.
dvd/dvd_split.ixx (2)
148-148: Variablesuccessis declared but inconsistently used.The
successvariable is declared at line 148 but in the Nintendo block (line 166) and main loop (lines 184, 186), the result is assigned tosuccessbut never checked to halt processing on failure. The warnings are logged, but the loop continues writing potentially scrambled data.Consider adding an early exit or
--force-splitcheck as the TODO on line 187 suggests.
175-191: Infinite loop exits only on partial read.The
while(true)loop at line 175 only exits whenbytesRead != sector.size()(line 179-180). This relies on reaching EOF, which should work correctly. However, consider adding a sector count limit or explicit EOF check for robustness.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@drive.ixx`:
- Around line 427-438: The omnidrive_version function assumes reserved5 has at
least 12 bytes and formats bytes as chars; add a size guard and format numeric
byte values. In omnidrive_version(const DriveConfig &drive_config) keep the
is_omnidrive_firmware check, then verify drive_config.reserved5->size() >= 12
before accessing indices 9–11 and, when building the version string, cast each
byte to unsigned char (e.g., static_cast<unsigned
char>((*drive_config.reserved5)[9])) so std::format prints numeric values rather
than characters and avoid out-of-range access.
In `@dvd/dvd_dump.ixx`:
- Around line 849-852: The call to progress_output currently passes lba (which
can be negative) into a uint32_t parameter causing unsigned wrap; update the
implementation so progress_output accepts a signed integer (e.g., int32_t) or
change the call to pass a non-negative offset and compute percent using the
signed delta; specifically, modify progress_output signature/usage to accept
int32_t (or compute and pass lba - lba_start) and compute progress as (lba -
lba_start) / (double)(lba_end - lba_start) while still logging the actual LBA
value (errors.scsi can remain as-is) so negative LBAs are represented correctly
without wrap.
♻️ Duplicate comments (1)
dvd/dvd_dump.ixx (1)
808-812: Align raw state pre-read with the actual LBA range.
When raw mode allows negativelba_start, the fixed index-DVD_LBA_STARTwithsectors_countwon’t cover the negative LBA region, so refine/verify can miss prior state and miscount errors. Consider moving this pre-read afterlba_start/lba_endare finalized and uselba_index_start = raw ? (lba_start - DVD_LBA_START) : 0withcount = raw ? (lba_end - lba_start) : sectors_count.
| export bool is_omnidrive_firmware(const DriveConfig &drive_config) | ||
| { | ||
| return drive_config.reserved5 && drive_config.reserved5->compare(0, 9, "OmniDrive") == 0; | ||
| } | ||
|
|
||
| export std::string omnidrive_version(const DriveConfig &drive_config) | ||
| { | ||
| if(!is_omnidrive_firmware(drive_config)) | ||
| return ""; | ||
|
|
||
| return std::format("{}.{}.{}", (*drive_config.reserved5)[9], (*drive_config.reserved5)[10], (*drive_config.reserved5)[11]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
C++ std::format char formatting behavior control characters signedness
💡 Result:
Short summary:
-
By default std::format treats plain char as a character (prints the character code unit, so control characters are copied to output as-is), and treats unsigned char / signed char as integers unless you request character presentation. [1][7]
-
You control behavior with format types:
- {:c} — force character presentation (prints the code unit as a character).
- {:d}, {:x}, etc. — integer presentation (promotes the char value to an integer; because plain char’s signedness is implementation-defined, negative values can appear if char is signed). To print an unsigned numeric view, cast to unsigned (e.g. static_cast(c) or to unsigned int). [2][7]
-
Escaped/quoted (debug) formatting (C++23): use the debug/escaped form (formatter’s set_debug_format or the ? presentation) to get quoted output with control characters escaped (e.g. "\n" instead of a raw newline). [2]
-
Historical note / spec fix: formatting char as integers had ambiguous behavior due to implementation-defined signedness; P2909R4 addresses / clarifies code-unit-as-integer formatting in the standard. If you rely on numeric values, cast to an explicitly signed/unsigned type. [3]
Sources:
[1] cppreference — std::format overview and format-specification.
[2] cppreference — standard format specification and character/string escaping (C++23).
[3] P2909R4 "Fix formatting of code units as integers" (WG21 proposal).
🏁 Script executed:
cd /tmp && find . -name "drive.ixx" -type f 2>/dev/null | head -1Repository: superg/redumper
Length of output: 41
🏁 Script executed:
find . -name "*.ixx" -o -name "*.hpp" -o -name "*.h" | grep -i drive | head -10Repository: superg/redumper
Length of output: 201
🏁 Script executed:
cat -n drive.ixx | sed -n '410,450p'Repository: superg/redumper
Length of output: 1062
🏁 Script executed:
rg "reserved5" drive.ixx -B 2 -A 2 | head -50Repository: superg/redumper
Length of output: 1357
🏁 Script executed:
rg "std::format" drive.ixx | head -20Repository: superg/redumper
Length of output: 504
Guard OmniDrive version parsing and format numeric bytes.
std::format with char uses character presentation by default, so control characters and unprintable bytes output as-is instead of numeric values. Additionally, char signedness is implementation-defined, and there is no size validation before accessing indices [9], [10], [11].
Add a size guard and cast to unsigned char to ensure numeric output:
Proposed fix
export std::string omnidrive_version(const DriveConfig &drive_config)
{
- if(!is_omnidrive_firmware(drive_config))
+ if(!is_omnidrive_firmware(drive_config) || drive_config.reserved5->size() < 12)
return "";
- return std::format("{}.{}.{}", (*drive_config.reserved5)[9], (*drive_config.reserved5)[10], (*drive_config.reserved5)[11]);
+ auto major = static_cast<unsigned>(static_cast<unsigned char>((*drive_config.reserved5)[9]));
+ auto minor = static_cast<unsigned>(static_cast<unsigned char>((*drive_config.reserved5)[10]));
+ auto patch = static_cast<unsigned>(static_cast<unsigned char>((*drive_config.reserved5)[11]));
+ return std::format("{}.{}.{}", major, minor, patch);
}📝 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.
| export bool is_omnidrive_firmware(const DriveConfig &drive_config) | |
| { | |
| return drive_config.reserved5 && drive_config.reserved5->compare(0, 9, "OmniDrive") == 0; | |
| } | |
| export std::string omnidrive_version(const DriveConfig &drive_config) | |
| { | |
| if(!is_omnidrive_firmware(drive_config)) | |
| return ""; | |
| return std::format("{}.{}.{}", (*drive_config.reserved5)[9], (*drive_config.reserved5)[10], (*drive_config.reserved5)[11]); | |
| } | |
| export std::string omnidrive_version(const DriveConfig &drive_config) | |
| { | |
| if(!is_omnidrive_firmware(drive_config) || drive_config.reserved5->size() < 12) | |
| return ""; | |
| auto major = static_cast<unsigned>(static_cast<unsigned char>((*drive_config.reserved5)[9])); | |
| auto minor = static_cast<unsigned>(static_cast<unsigned char>((*drive_config.reserved5)[10])); | |
| auto patch = static_cast<unsigned>(static_cast<unsigned char>((*drive_config.reserved5)[11])); | |
| return std::format("{}.{}.{}", major, minor, patch); | |
| } |
🤖 Prompt for AI Agents
In `@drive.ixx` around lines 427 - 438, The omnidrive_version function assumes
reserved5 has at least 12 bytes and formats bytes as chars; add a size guard and
format numeric byte values. In omnidrive_version(const DriveConfig
&drive_config) keep the is_omnidrive_firmware check, then verify
drive_config.reserved5->size() >= 12 before accessing indices 9–11 and, when
building the version string, cast each byte to unsigned char (e.g.,
static_cast<unsigned char>((*drive_config.reserved5)[9])) so std::format prints
numeric values rather than characters and avoid out-of-range access.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dvd/dvd_dump.ixx`:
- Around line 491-496: The CI failure is due to clang-format violations in the
function progress_output; run clang-format-18 on dvd/dvd_dump.ixx (or apply the
formatter) to reflow the line containing the LOGC_RF call so it meets style
rules; specifically reformat the LOGC_RF invocation inside progress_output to
wrap/align parameters per clang-format-18 output and commit the updated file so
the formatter warning is resolved.
🧹 Nitpick comments (1)
dvd/dvd_dump.ixx (1)
858-860: Minor: signed/unsigned comparison.Line 860 uses
int32_t icompared againstuint32_t sectors_to_read. While this works correctly in practice (sectors_to_read is always small and positive), consider usinguint32_t ifor consistency with the type ofsectors_to_read.♻️ Suggested fix
uint32_t sectors_to_read = std::min(sectors_at_once, (uint32_t)(lba_end - lba)); auto base_range = find_range(protection, lba); - for(int32_t i = 0; i < sectors_to_read; ++i) + for(uint32_t i = 0; i < sectors_to_read; ++i)
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.