Iridium: PHY cross-validation against gr-iridium's test burst#30
Conversation
The reference implementation's own generated test data (prbs15-2M-20dB.sigmf-data: a synthetic frame with PRBS15 payload at 20 dB channel SNR, i.e. ~3 dB full-band) demodulates bit-perfectly through our chain: burst detected spectrally, downlink access code recognized, and the payload satisfies the PRBS15 recurrence x[i] = x[i-15] xor x[i-14] with zero violations across all 343 testable bits. That validates burst gating, tone CFO, the coherent UW fit, DQPSK decode, dqpsk_map, and bit ordering against gr-iridium's modulator — their TX, our RX. The DDC'd burst (250 kHz channel rate, 32 KB i16) is vendored as a CI fixture with attribution; tests/crossval.rs asserts the access code and the bit-perfect PRBS15 property. With the iridium-toolkit oracle covering layer 2 (#29), both layers of the new core are now validated against their reference implementations. New dumpbits example is the capture harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97e7110770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `test-data/prbs15-2M-20dB.sigmf-data` (https://github.com/muccc/gr-iridium, | ||
| GPL-3; this is *test data* generated by their reference modulator — a |
There was a problem hiding this comment.
The new fixture is DDC'd from gr-iridium's test-data/prbs15-2M-20dB.sigmf-data and this README labels that source as GPL-3, while this crate still inherits the workspace MIT OR Apache-2.0 package license and the repo sourcing policy only allows GPL projects as fact references. Shipping the copied/derived binary in tests/data therefore makes source/package distributions include GPL material under incompatible crate licensing; please replace it with a permissively licensed/generated fixture or explicitly handle it as separately licensed/excluded data.
Useful? React with 👍 / 👎.
| assert_eq!(bursts.len(), 1, "exactly one burst expected"); | ||
| let bits = &bursts[0]; | ||
| assert_eq!(&bits[..24], &frame::ACCESS_DL[..], "downlink access code"); | ||
| let payload = &bits[24..]; |
There was a problem hiding this comment.
For this reference burst the commit claims 343 testable PRBS bits, which implies a 358-bit payload, but this check accepts any payload length of 300 or more. If the demodulator starts truncating the tail of this fixture, CI can still pass while the cross-validation no longer proves the burst is demodulated bit-for-bit; assert the exact expected length or exact expected bitstream before checking the recurrence.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThis PR introduces cross-validation infrastructure and example code for the Iridium PHY demodulator. A PRBS15 test fixture and integration test validate bit-perfect demodulation against gr-iridium, while a new ChangesCross-validation and examples for Iridium demodulator
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
🧹 Nitpick comments (3)
crates/xng-mode-iridium/examples/dumpbits.rs (3)
30-30: ⚡ Quick winReplace magic number with CHANNEL_PASSBAND_HZ constant.
The passband value
25_000.0is hardcoded, butCHANNEL_PASSBAND_HZis a public constant in the same module (per context snippet 1) and is used by the upstreamIridiumChannelDecoder. Using the constant improves maintainability and consistency.♻️ Proposed fix
-use xng_mode_iridium::{demod::IridiumDemod, CHANNEL_RATE}; +use xng_mode_iridium::{demod::IridiumDemod, CHANNEL_PASSBAND_HZ, CHANNEL_RATE};- let mut ddc = Ddc::new(rate, CHANNEL_RATE, offset, 25_000.0).unwrap(); + let mut ddc = Ddc::new(rate, CHANNEL_RATE, offset, CHANNEL_PASSBAND_HZ).unwrap();🤖 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 `@crates/xng-mode-iridium/examples/dumpbits.rs` at line 30, Replace the hardcoded passband literal with the module constant: locate the Ddc::new(...) call (the instantiation in dumpbits.rs) and replace the 25_000.0 argument with CHANNEL_PASSBAND_HZ so the Ddc::new(rate, CHANNEL_RATE, offset, CHANNEL_PASSBAND_HZ). This keeps behavior aligned with IridiumChannelDecoder and avoids a magic number.
15-23: ⚡ Quick winConsider detecting partial samples at end of file.
chunks_exact(8)silently drops any trailing bytes if the file size is not a multiple of 8. For a validation tool, detecting and warning about truncated samples might be valuable.🛡️ Proposed fix
let raw = std::fs::read(&path).expect("read"); + if raw.len() % 8 != 0 { + eprintln!("Warning: file size {} not a multiple of 8; {} trailing bytes ignored", raw.len(), raw.len() % 8); + } let samples: Vec<Complex<f32>> = raw🤖 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 `@crates/xng-mode-iridium/examples/dumpbits.rs` around lines 15 - 23, The current use of raw.chunks_exact(8) in the samples construction silently drops trailing bytes; update the logic around raw and samples to detect if raw.len() % 8 != 0 and emit a warning (or return an error) about truncated partial sample data before or instead of collecting samples. Specifically, inspect raw.len() or use raw.chunks_exact(8) with raw.chunks_exact(8).remainder() to detect leftover bytes, and wire the warning/error into the validation flow where samples are created so truncated samples are reported rather than silently ignored.
11-12: ⚡ Quick winImprove error messages for missing or invalid arguments.
When
rateoroffsetarguments are missing or fail to parse, the.unwrap()calls will panic with a generic message instead of showing usage. Consider using.expect("usage: dumpbits <cf32> <rate> <offset>")consistently for better UX.🔧 Proposed fix
- let rate: f64 = args.next().unwrap().parse().unwrap(); - let offset: f64 = args.next().unwrap().parse().unwrap(); + let rate: f64 = args + .next() + .expect("usage: dumpbits <cf32> <rate> <offset>") + .parse() + .expect("rate must be a valid float"); + let offset: f64 = args + .next() + .expect("usage: dumpbits <cf32> <rate> <offset>") + .parse() + .expect("offset must be a valid float");🤖 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 `@crates/xng-mode-iridium/examples/dumpbits.rs` around lines 11 - 12, Replace the generic unwrap() calls when retrieving and parsing the CLI args for rate and offset with explicit expect() messages so missing or invalid inputs print a consistent usage hint; specifically update the code that reads from the args iterator (the expressions creating rate and offset) to call args.next().expect("usage: dumpbits <cf32> <rate> <offset>") and then .parse().expect("usage: dumpbits <cf32> <rate> <offset>") so both absence and parse failures produce the same helpful usage message.
🤖 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 `@crates/xng-mode-iridium/examples/dumpbits.rs`:
- Line 27: The example's DDC bypass condition uses a stricter offset threshold
(offset.abs() < 1e-9) than the upstream IridiumChannelDecoder::new which uses
1e-6 for both rate and offset; update the example's condition in dumpbits.rs
(the if that builds channel: Vec<Complex<f32>>) to use offset.abs() < 1e-6 so
the example's DDC-bypass behavior matches IridiumChannelDecoder::new's contract.
---
Nitpick comments:
In `@crates/xng-mode-iridium/examples/dumpbits.rs`:
- Line 30: Replace the hardcoded passband literal with the module constant:
locate the Ddc::new(...) call (the instantiation in dumpbits.rs) and replace the
25_000.0 argument with CHANNEL_PASSBAND_HZ so the Ddc::new(rate, CHANNEL_RATE,
offset, CHANNEL_PASSBAND_HZ). This keeps behavior aligned with
IridiumChannelDecoder and avoids a magic number.
- Around line 15-23: The current use of raw.chunks_exact(8) in the samples
construction silently drops trailing bytes; update the logic around raw and
samples to detect if raw.len() % 8 != 0 and emit a warning (or return an error)
about truncated partial sample data before or instead of collecting samples.
Specifically, inspect raw.len() or use raw.chunks_exact(8) with
raw.chunks_exact(8).remainder() to detect leftover bytes, and wire the
warning/error into the validation flow where samples are created so truncated
samples are reported rather than silently ignored.
- Around line 11-12: Replace the generic unwrap() calls when retrieving and
parsing the CLI args for rate and offset with explicit expect() messages so
missing or invalid inputs print a consistent usage hint; specifically update the
code that reads from the args iterator (the expressions creating rate and
offset) to call args.next().expect("usage: dumpbits <cf32> <rate> <offset>") and
then .parse().expect("usage: dumpbits <cf32> <rate> <offset>") so both absence
and parse failures produce the same helpful usage message.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c61fb5c-1685-423a-810f-96b2f8edf274
📒 Files selected for processing (5)
crates/xng-mode-iridium/PROVENANCE.mdcrates/xng-mode-iridium/examples/dumpbits.rscrates/xng-mode-iridium/tests/crossval.rscrates/xng-mode-iridium/tests/data/README.mdcrates/xng-mode-iridium/tests/data/grtest_prbs15_250k.i16
| eprintln!("{} samples ({:.3}s)", samples.len(), samples.len() as f64 / rate); | ||
|
|
||
| let mut chan: Vec<Complex<f32>> = Vec::new(); | ||
| let channel: Vec<Complex<f32>> = if (rate - CHANNEL_RATE).abs() < 1e-6 && offset.abs() < 1e-9 { |
There was a problem hiding this comment.
Offset threshold inconsistency with production code.
The DDC bypass condition uses offset.abs() < 1e-9, but the upstream IridiumChannelDecoder::new (context snippet 1) uses < 1e-6 for both rate and offset. This tighter threshold means the example will instantiate DDC more often than production code would for the same inputs, creating behavioral inconsistency.
🔧 Proposed fix to align with upstream contract
- let channel: Vec<Complex<f32>> = if (rate - CHANNEL_RATE).abs() < 1e-6 && offset.abs() < 1e-9 {
+ let channel: Vec<Complex<f32>> = if (rate - CHANNEL_RATE).abs() < 1e-6 && offset.abs() < 1e-6 {📝 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.
| let channel: Vec<Complex<f32>> = if (rate - CHANNEL_RATE).abs() < 1e-6 && offset.abs() < 1e-9 { | |
| let channel: Vec<Complex<f32>> = if (rate - CHANNEL_RATE).abs() < 1e-6 && offset.abs() < 1e-6 { |
🤖 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 `@crates/xng-mode-iridium/examples/dumpbits.rs` at line 27, The example's DDC
bypass condition uses a stricter offset threshold (offset.abs() < 1e-9) than the
upstream IridiumChannelDecoder::new which uses 1e-6 for both rate and offset;
update the example's condition in dumpbits.rs (the if that builds channel:
Vec<Complex<f32>>) to use offset.abs() < 1e-6 so the example's DDC-bypass
behavior matches IridiumChannelDecoder::new's contract.
1 participant
Completes #29's validation story at the PHY layer.
What
gr-iridium ships a test capture generated by its own reference modulator — a synthetic Iridium frame with a PRBS15 payload at 20 dB channel SNR (≈3 dB full-band, invisible to naive power detection). Our chain demodulates it bit-perfectly:
x[i] = x[i−15] ⊕ x[i−14]with zero violations across all 343 testable bits.Their TX, our RX — validating burst gating, tone CFO, the coherent UW fit, DQPSK decode,
dqpsk_map, and bit ordering against the reference implementation.CI guard
The burst, DDC'd to the 250 kHz channel rate, is a 32 KB vendored fixture (attributed in tests/data/README.md — it's their test data used as a validation vector, not ported code);
tests/crossval.rsasserts the access code and the bit-perfect PRBS15 property. Combined with #29's iridium-toolkit oracle at layer 2, both layers of the Iridium core are now validated against their reference implementations — the strongest validation available without an off-air L-band capture, which remains the noted follow-up.🤖 Generated with Claude Code