Skip to content

Print error summary #399

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Deterous wants to merge 5 commits into from
Closed

Print error summary #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56c83c4
Print error summary
Deterous Jun 25, 2026
6d1df69
Use .. instead of - for range separator
Deterous Jun 25, 2026
45daaad
Error summary is verbose
Deterous Jun 25, 2026
965a421
Review
Deterous Jun 29, 2026
16d8e62
clang format
Deterous Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cd/cd_dump.ixx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import cd.subcode;
import cd.toc;
import common;
import drive;
import interval_set;
import options;
import range;
import scsi.cmd;
Expand Down Expand Up @@ -338,6 +339,9 @@ export bool redumper_dump_cd(Context &ctx, const Options &options, bool dump)
int32_t subcode_shift = 0;
uint32_t subcode_byte_desync_counter = 0;

IntervalSet<int32_t> scsi_error_intervals;
IntervalSet<int32_t> c2_error_intervals;

SignalINT signal;

int32_t lba_overread = lba_end;
Expand Down Expand Up @@ -432,7 +436,10 @@ export bool redumper_dump_cd(Context &ctx, const Options &options, bool dump)
if(session_gap_range == nullptr && lba < lba_end)
{
if(dump)
{
errors.scsi += CD_DATA_SIZE_SAMPLES;
scsi_error_intervals.add(lba);
}

if(options.verbose)
LOGC_R("[LBA: {:6}] SCSI error ({})", lba, SPTD::StatusMessage(status));
Expand Down Expand Up @@ -478,7 +485,10 @@ export bool redumper_dump_cd(Context &ctx, const Options &options, bool dump)
if(c2_samples)
{
if(dump)
{
errors.c2 += c2_samples;
c2_error_intervals.add(lba);
}

if(options.verbose)
{
Expand Down Expand Up @@ -575,6 +585,16 @@ export bool redumper_dump_cd(Context &ctx, const Options &options, bool dump)
LOG(" SCSI: {} samples", errors.scsi);
LOG(" C2: {} samples", errors.c2);
LOG(" Q: {}", errors.q);

if(options.verbose && (!scsi_error_intervals.empty() || !c2_error_intervals.empty()))

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not help at all, if disc is damaged, usually you get thousands of C2 errors and it's not every sector so this output will be a mess of tiny ranges.

I don't want this in log.

@Deterous Deterous Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verbose printing of errors to log is requested by multiple mods and this error summary is the compromise I could come up with

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you enumerate cases where do we want to see errors and how does accept / reject submission depend on this?

Maybe there is a better way.

@Deterous Deterous Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This printing is under "verbose" and should not be printed for most users. This printing is for people who want to see all the error locations, and is not relevant for redump.info

{
LOG("");
LOG("LBA error ranges: ");
if(!scsi_error_intervals.empty())
LOG(" SCSI: {}", scsi_error_intervals.to_string());
if(!c2_error_intervals.empty())
LOG(" C2: {}", c2_error_intervals.to_string());
}
}
else
{
Expand Down
69 changes: 69 additions & 0 deletions debug.ixx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ module;

export module debug;

import bd;
import cd.cd;
import cd.common;
import cd.subcode;
import cd.toc;
import common;
import drive;
import drive.mediatek;
import dvd;
import dvd.dump;
import dvd.xbox;
import options;
Expand Down Expand Up @@ -414,6 +416,73 @@ export int redumper_debug(Context &ctx, Options &options)
return exit_code;
}

export int redumper_state(Context &ctx, Options &options)

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this whole thing vibecoded?

Looks pretty low effort and too many things to correct.

@Deterous Deterous Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want a debug command I can remove it, it's just for mods who want to parse an existing state file

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want great code but it's just dirty implementation.

{
int exit_code = 0;

std::string image_prefix = (std::filesystem::path(options.image_path) / options.image_name).string();

std::filesystem::path state_path(image_prefix + ".state");
if(!std::filesystem::exists(state_path))
throw_line("state file not found ({})", state_path.filename().string());

bool cd = std::filesystem::exists(image_prefix + ".scram");
if(!cd && options.disc_type && *options.disc_type == "CD")
cd = true;
Comment on lines +430 to +431

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this check at all?
.scram exists, that's cd, .sdram exists it's dvd, .sbram exists it's bluray.

@Deterous Deterous Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that you can print the state file from someone else's logs, you can manually say it's CD state file

uint64_t entries_count = std::filesystem::file_size(state_path) / sizeof(State);

std::fstream state_fs(state_path, std::fstream::in | std::fstream::binary);
if(!state_fs.is_open())
throw_line("unable to open file ({})", state_path.filename().string());

int32_t offset = 0;

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any useful if it's not aligned to sync for data disc?

I think most users will be confused by this.

@Deterous Deterous Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the need for the function is just to help visualize what is in the state file. You say that there is no need to print to log but they still want the error location information including C2 error sizes. It is already in state file so this debug function is meant to provide that function.

if(cd)
offset = LBA_START * CD_DATA_SIZE_SAMPLES;
else if(std::filesystem::exists(image_prefix + ".sdram"))
offset = dvd::LBA_START;
else if(std::filesystem::exists(image_prefix + ".sbram"))
offset = bd::LBA_START;

static const char *STATE_NAME[] = { "Unread", "Read Error", "Success (No confirmed C2 data)", "Success (No SCSI state)", "Success" };

LOG("state file (unit: {}): ", cd ? "samples" : "sectors");

std::vector<State> buffer(CHUNK_1KB + 1);
State current = (State)0xFF;
int64_t range_start = offset;

for(uint64_t i = 0; i < entries_count; i += CHUNK_1KB)
{
uint32_t count = std::min((uint64_t)CHUNK_1KB, entries_count - i);
state_fs.read((char *)buffer.data(), count * sizeof(State));

// create state change at end of file to trigger final print
if(i + count >= entries_count)
buffer[count++] = (State) ~(uint8_t)current;

for(uint32_t j = 0; j < count; ++j)
{
if(buffer[j] == current)
continue;

int64_t range_end = offset + (int64_t)(i + j) - 1;
if(range_start <= range_end)
{
const char *state_name = (uint8_t)current < std::size(STATE_NAME) ? STATE_NAME[(uint8_t)current] : "Unknown";
if(cd)
LOG(" {}..{} (LBA {}..{}): {}", range_start, range_end, sample_to_lba(range_start, 0), sample_to_lba(range_end, 0), state_name);
else
LOG(" {}..{}: {}", range_start, range_end, state_name);
}

current = buffer[j];
range_start = offset + (int64_t)(i + j);
}
}

return exit_code;
}

export int redumper_flip(Context &ctx, Options &options)
{
int exit_code = 0;
Expand Down
6 changes: 6 additions & 0 deletions dvd/dvd_dump.ixx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,6 +1378,12 @@ export bool redumper_dump_dvd(Context &ctx, const Options &options, bool dump)
LOG("media errors: ");
LOG(" SCSI: {}", errors.scsi);
LOG(" EDC: {}", errors.edc);

if(options.verbose && !error_intervals.empty())

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as bad as CD but I think it's useless.

{
LOG("");
LOG("LBA error ranges: {}", error_intervals.to_string());
}
}
else
{
Expand Down
15 changes: 15 additions & 0 deletions interval_set.ixx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
module;
#include <algorithm>
#include <cstdint>
#include <format>
#include <optional>
#include <string>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -213,6 +215,19 @@ public:
return std::nullopt;
}


std::string to_string() const

@superg superg Jun 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't belong here, it's fairly custom formatter that includes last element into range and quite ineffective implementation. Please move it to the place of use.

{
std::string str;
for(auto &r : _ranges)
{
if(!str.empty())
str += ", ";
str += (r.second - r.first == 1) ? std::format("{}", r.first) : std::format("{}-{}", r.first, r.second - 1);
}
return str;
}

private:
std::vector<Interval> _ranges;
};
Expand Down
1 change: 1 addition & 0 deletions redumper.ixx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ const std::map<std::string, Command> COMMANDS{
{ "debug", { false, false, false, false, false, redumper_debug } },
{ "fixmsf", { false, false, false, true, false, redumper_fix_msf } },
{ "debug::flip", { false, false, false, true, false, redumper_flip } },
{ "debug::state", { false, false, false, true, false, redumper_state } },
{ "drive::test", { true, true, true, false, false, redumper_drive_test } },
};

Expand Down
Loading