Adding correction to the iec-60908b code.#2028
Conversation
📝 WalkthroughWalkthroughThis pull request adds a Mode 2 CD sector corrector that validates and repairs Yellow Book EDC/ECC using Galois-field arithmetic. It introduces helpers for GF operations and symbol addressing, implements Reed-Solomon line correction, adds EDC verification via CRC32, and orchestrates repair by iteratively correcting P and Q lines until the sector passes EDC validation. ChangesCD Sector Error Correction
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@third_party/iec-60908b/correct.c`:
- Around line 151-158: The code currently reads the form bit from subheader[2]
into form and returns 0 for form==2 immediately after a failed
check_edc(sector), which incorrectly skips ECC repair if that form bit is
corrupted; instead, after check_edc(sector) fails, do not short-circuit on the
subheader bit—attempt the ECC repair path used for Form 1 (i.e., invoke the
module’s ECC correction routine on sector) even when (subheader[2] & 0x20) is
set, and only treat the sector as unrecoverable if ECC correction also fails;
keep the original form value for post-repair validation but do not use it to
prevent attempting ECC correction (reference: subheader, form, check_edc and the
project’s ECC correction function).
In `@third_party/iec-60908b/correct.h`:
- Line 5: Update the license header year from 2026 to 2025 in the third-party
IEC-60908b sources: replace the line that currently reads "Copyright (c) 2026
Nicolas \"Pixel\" Noble" with the same text using 2025 in both the header and
the corresponding source file (the identical copyright block in correct.h and
correct.c).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a4b8b23a-58bf-4035-86a6-01f518a251a7
📒 Files selected for processing (2)
third_party/iec-60908b/correct.cthird_party/iec-60908b/correct.h
| const uint8_t* subheader = body + 4; | ||
| unsigned form = (subheader[2] & 0x20) ? 2 : 1; | ||
|
|
||
| // A clean sector needs no work, and Form 2 has no ECC at all, so for Form 2 | ||
| // the EDC verdict is final either way. | ||
| if (check_edc(sector)) return 1; | ||
| if (form == 2) return 0; | ||
|
|
There was a problem hiding this comment.
Don’t gate ECC repair on a single potentially corrupted form bit.
form is derived only from subheader[2], then if (form == 2) return 0; skips correction entirely. If a Form 1 sector has that bit flipped, this returns failure without attempting recoverable ECC repair.
Suggested hardening
- const uint8_t* subheader = body + 4;
- unsigned form = (subheader[2] & 0x20) ? 2 : 1;
+ const uint8_t* subheader = body + 4;
+ unsigned form_a = (subheader[2] & 0x20) ? 2 : 1; // first subheader copy
+ unsigned form_b = (subheader[6] & 0x20) ? 2 : 1; // duplicated copy
// A clean sector needs no work, and Form 2 has no ECC at all, so for Form 2
// the EDC verdict is final either way.
if (check_edc(sector)) return 1;
- if (form == 2) return 0;
+ // Only treat as Form 2 when both copies agree; otherwise bias to Form 1 path
+ // so ECC still gets a chance to recover a damaged Form 1 subheader.
+ if (form_a == 2 && form_b == 2) return 0;
+ unsigned form = 1;📝 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.
| const uint8_t* subheader = body + 4; | |
| unsigned form = (subheader[2] & 0x20) ? 2 : 1; | |
| // A clean sector needs no work, and Form 2 has no ECC at all, so for Form 2 | |
| // the EDC verdict is final either way. | |
| if (check_edc(sector)) return 1; | |
| if (form == 2) return 0; | |
| const uint8_t* subheader = body + 4; | |
| unsigned form_a = (subheader[2] & 0x20) ? 2 : 1; // first subheader copy | |
| unsigned form_b = (subheader[6] & 0x20) ? 2 : 1; // duplicated copy | |
| // A clean sector needs no work, and Form 2 has no ECC at all, so for Form 2 | |
| // the EDC verdict is final either way. | |
| if (check_edc(sector)) return 1; | |
| // Only treat as Form 2 when both copies agree; otherwise bias to Form 1 path | |
| // so ECC still gets a chance to recover a damaged Form 1 subheader. | |
| if (form_a == 2 && form_b == 2) return 0; | |
| unsigned form = 1; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/iec-60908b/correct.c` around lines 151 - 158, The code currently
reads the form bit from subheader[2] into form and returns 0 for form==2
immediately after a failed check_edc(sector), which incorrectly skips ECC repair
if that form bit is corrupted; instead, after check_edc(sector) fails, do not
short-circuit on the subheader bit—attempt the ECC repair path used for Form 1
(i.e., invoke the module’s ECC correction routine on sector) even when
(subheader[2] & 0x20) is set, and only treat the sector as unrecoverable if ECC
correction also fails; keep the original form value for post-repair validation
but do not use it to prevent attempting ECC correction (reference: subheader,
form, check_edc and the project’s ECC correction function).
|
|
||
| MIT License | ||
|
|
||
| Copyright (c) 2026 Nicolas "Pixel" Noble |
There was a problem hiding this comment.
Use the repository’s expected copyright year in the license header.
This header uses 2026, but project convention here is 2025; please align this line (and the same license block in third_party/iec-60908b/correct.c).
Suggested patch
-Copyright (c) 2026 Nicolas "Pixel" Noble
+Copyright (c) 2025 Nicolas "Pixel" Noble📝 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.
| Copyright (c) 2026 Nicolas "Pixel" Noble | |
| Copyright (c) 2025 Nicolas "Pixel" Noble |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@third_party/iec-60908b/correct.h` at line 5, Update the license header year
from 2026 to 2025 in the third-party IEC-60908b sources: replace the line that
currently reads "Copyright (c) 2026 Nicolas \"Pixel\" Noble" with the same text
using 2025 in both the header and the corresponding source file (the identical
copyright block in correct.h and correct.c).
1 participant
No description provided.