Complete CLI-to-Crate Unification (Phase 1-3): Eliminate Remaining Duplication

[coderabbitai]

Context

This issue continues the work from issue #9 and PR #10.

✅ Completed in PR #10 (~140 lines eliminated):

• GPS conversion functions migrated to crate
• Flag formatting functions migrated to crate
• Error handling unified with anyhow::Result
• H-frame parsing and 14-bit encoding aligned

🔄 Remaining Work (~310 lines to eliminate):
This issue covers the final three phases to complete full unification.

---

🎯 Critical Principle: Dependency Direction

CLI (src/main.rs, src/bbl_format.rs)
  ↓ DEPENDS ON ↓
Crate (src/lib.rs, src/parser/*)

✅ CORRECT: CLI imports and uses crate functions
❌ WRONG: Making CLI modules public for crate to use (creates reverse dependency)

---

Phase 1: Unify Helper Functions (~50 lines)

Objective

Move sign_extend_* helper functions FROM src/bbl_format.rs TO crate library.

Current State

• CLI: src/bbl_format.rs contains 7 sign_extend functions (lines vary)
• Crate: No centralized helper functions module

AI Agent Instructions

Step 1.1: Create Helper Module in Crate

Create new file: src/parser/helpers.rs

//! Helper functions for BBL parsing

/// Sign-extend a 2-bit value to i32
pub fn sign_extend_2bit(value: u8) -> i32 {
    if value & 0x02 != 0 {
        (value as i32) | !0x03
    } else {
        value as i32
    }
}

/// Sign-extend a 4-bit value to i32
pub fn sign_extend_4bit(value: u8) -> i32 {
    if value & 0x08 != 0 {
        (value as i32) | !0x0F
    } else {
        value as i32
    }
}

/// Sign-extend a 6-bit value to i32
pub fn sign_extend_6bit(value: u8) -> i32 {
    if value & 0x20 != 0 {
        (value as i32) | !0x3F
    } else {
        value as i32
    }
}

/// Sign-extend an 8-bit value to i32
pub fn sign_extend_8bit(value: u8) -> i32 {
    if value & 0x80 != 0 {
        (value as i32) | !0xFF
    } else {
        value as i32
    }
}

/// Sign-extend a 14-bit value to i32
pub fn sign_extend_14bit(value: u16) -> i32 {
    if value & 0x2000 != 0 {
        (value as i32) | !0x3FFF
    } else {
        value as i32
    }
}

/// Sign-extend a 16-bit value to i32
pub fn sign_extend_16bit(value: u16) -> i32 {
    if value & 0x8000 != 0 {
        (value as i32) | !0xFFFF
    } else {
        value as i32
    }
}

/// Sign-extend a 24-bit value to i32
pub fn sign_extend_24bit(value: u32) -> i32 {
    if value & 0x800000 != 0 {
        (value as i32) | !0xFFFFFF
    } else {
        value as i32
    }
}

Verification: Ensure implementations exactly match src/bbl_format.rs

Step 1.2: Export from Parser Module

Update: src/parser/mod.rs

// ... existing module declarations ...
mod helpers;

// ... existing pub use statements ...
pub use helpers::*;

Step 1.3: Export from Crate Root

Update: src/lib.rs

Ensure parser module is already public and re-exported (should be from PR #10):

pub mod parser;
pub use parser::*;

Step 1.4: Update CLI to Import Helpers

Update: src/bbl_format.rs

Add at top of file:

use bbl_parser::parser::{
    sign_extend_2bit, 
    sign_extend_4bit, 
    sign_extend_6bit,
    sign_extend_8bit, 
    sign_extend_14bit, 
    sign_extend_16bit, 
    sign_extend_24bit,
};

DELETE the 7 sign_extend function definitions from src/bbl_format.rs

Estimated lines removed: ~35-50

Step 1.5: Validation

# Build and test
cargo build --release
cargo test

# A/B test CLI against reference
./target/release/bbl_parser test_logs/*.BBL --csv --gpx --event
# Compare outputs against known-good reference

Checkpoint: Verify no behavior changes, all tests pass.

---

Phase 2: Unify parse_frame_data (~160 lines)

Objective

Replace CLI's parse_frame_data with crate's version, handling the debug parameter difference.

Current State

• CLI: src/bbl_format.rs::parse_frame_data has debug: bool parameter
• Crate: src/parser/frame.rs::parse_frame_data does NOT have debug parameter

AI Agent Instructions

Step 2.1: Add Debug Parameter to Crate Function

Update: src/parser/frame.rs

Modify function signature:

pub fn parse_frame_data(
    stream: &mut BBLDataStream,
    frame_def: &FrameDefinition,
    current_frame: &mut [i32],
    previous_frame: Option<&[i32]>,
    previous2_frame: Option<&[i32]>,
    skipped_frames: u32,
    raw: bool,
    data_version: u8,
    sysconfig: &HashMap<String, i32>,
    debug: bool,  // ADD THIS PARAMETER
) -> Result<()>

Add debug logging (mirror CLI behavior):

• When debug == true, log field decoding steps
• Example locations: after decode_field_value, after apply_predictor

Reference CLI implementation in src/bbl_format.rs for exact debug output format.

Step 2.2: Update Crate Call Sites

Search for parse_frame_data calls in src/parser/:

rg "parse_frame_data\(" src/parser/ -A 2

Add debug: false to any internal crate calls.

Step 2.3: Update CLI to Use Crate Function

Update: src/main.rs

Add import:

use bbl_parser::parser::parse_frame_data;

Replace call sites:
Find: bbl_format::parse_frame_data(
Replace with: parse_frame_data(

Step 2.4: Remove CLI Implementation

Update: src/bbl_format.rs

DELETE the entire parse_frame_data function (approximately 160 lines).

Step 2.5: Validation

# Full build and test suite
cargo clean
cargo build --release
cargo test --all

# A/B test with debug enabled
./target/release/bbl_parser test_logs/flight1.BBL --debug --csv
# Compare against reference implementation output

# Test without debug
./target/release/bbl_parser test_logs/flight1.BBL --csv
diff output.csv reference.csv

Checkpoint: Verify identical output with and without debug flag.

---

Phase 3: Unify BBLDataStream & Delete bbl_format.rs (~100 lines)

Objective

Make CLI use crate's BBLDataStream, eliminate src/bbl_format.rs entirely.

Current State

• CLI: src/bbl_format.rs::BBLDataStream (~100 lines)
• Crate: src/parser/stream.rs::BBLDataStream (~200 lines)

AI Agent Instructions

Step 3.1: Verify Implementation Compatibility

# Compare the two implementations
diff -u <(sed -n '/pub struct BBLDataStream/,/^}/p' src/bbl_format.rs) \
        <(sed -n '/pub struct BBLDataStream/,/^}/p' src/parser/stream.rs)

Action: If implementations differ, align crate version with CLI behavior first (separate commit).

Step 3.2: Update CLI Imports

Update: src/main.rs

Remove:

mod bbl_format;

Add:

use bbl_parser::BBLDataStream;

If any constants remain in bbl_format:

use bbl_parser::parser::{ENCODING_*, PREDICTOR_*}; // if needed

Step 3.3: Global Type Replacement in main.rs

Find all occurrences:

rg "bbl_format::" src/main.rs

Replace:

• bbl_format::BBLDataStream → BBLDataStream
• bbl_format::ENCODING_* → ENCODING_* (if constants moved)
• Any other bbl_format:: references

Step 3.4: Verify No Remaining bbl_format Dependencies

# Check for any remaining references
rg "bbl_format" src/main.rs
# Should return zero results

Step 3.5: Delete CLI bbl_format Module

# Remove the file
git rm src/bbl_format.rs

# Verify it's not referenced in lib.rs
rg "bbl_format" src/lib.rs

Update src/lib.rs if any bbl_format references remain (should already be clean from PR #10).

Step 3.6: Comprehensive Validation

# Full rebuild
cargo clean
cargo build --release
cargo test --all

# Test all frame types
for file in test_logs/*.BBL; do
    echo "Testing: $file"
    ./target/release/bbl_parser "$file" --csv --gpx --event
done

# Compare against reference implementation
diff -r output_new/ output_reference/

Final Checkpoint:

• ✅ All tests pass
• ✅ CLI output identical to reference
• ✅ src/bbl_format.rs deleted
• ✅ ~310 total lines eliminated

---

❌ Anti-Patterns: What NOT to Do

DO NOT: Make CLI Modules Public

Wrong Example (from commit 2904762):

// ❌ WRONG in src/lib.rs
pub mod bbl_format;  // Making CLI module public
pub use bbl_format::*;

Why Wrong: Creates reverse dependency (crate depends on CLI module)

Correct Approach: Move functionality TO crate, CLI imports FROM crate

DO NOT: Add crate → CLI Dependencies

Wrong Pattern:

// ❌ WRONG in src/parser/gps.rs
use crate::bbl_format::some_helper;

Why Wrong: Crate should never import from CLI modules

Correct Pattern:

// ✅ CORRECT in src/main.rs
use bbl_parser::parser::some_helper;

DO NOT: Skip Validation Steps

Wrong Approach:

• Making all changes at once without testing
• Skipping A/B testing against reference
• Not verifying behavior parity

Correct Approach:

• Complete one phase at a time
• Validate after each step
• Run full test suite + A/B comparison
• Commit and test before moving to next phase

DO NOT: Change Working Logic

Wrong Example:

// ❌ WRONG: Changing the algorithm while migrating
pub fn sign_extend_14bit(value: u16) -> i32 {
    // Don't optimize or refactor the logic here!
    // Just move it exactly as-is
}

Correct Approach: Copy implementation byte-for-byte, refactor in separate PR if needed

---

Execution Checklist

Phase 1: Helper Functions

• Create src/parser/helpers.rs with 7 sign_extend functions
• Export from src/parser/mod.rs
• Verify exports in src/lib.rs
• Import in src/bbl_format.rs
• Delete sign_extend definitions from src/bbl_format.rs
• Run cargo build --release
• Run cargo test
• A/B test CLI output
• Commit: "refactor: move sign_extend helpers to crate library"

Phase 2: parse_frame_data

• Add debug: bool param to crate's parse_frame_data
• Add debug logging in crate function
• Update internal crate call sites
• Import crate's parse_frame_data in src/main.rs
• Replace CLI call sites
• Delete parse_frame_data from src/bbl_format.rs
• Run cargo clean && cargo build --release
• Run cargo test --all
• Test with --debug flag
• A/B test with/without debug
• Commit: "refactor: unify parse_frame_data implementation"

Phase 3: BBLDataStream

• Verify BBLDataStream implementations match
• Remove mod bbl_format; from src/main.rs
• Import BBLDataStream from crate
• Replace all bbl_format::* references in src/main.rs
• Verify no remaining bbl_format references
• Delete src/bbl_format.rs
• Run cargo clean && cargo build --release
• Run cargo test --all
• Full A/B test suite on all test logs
• Commit: "refactor: unify BBLDataStream, eliminate CLI bbl_format module"

---

Expected Outcomes

Code Metrics

• Lines Eliminated: ~310 (Phase 1: ~50, Phase 2: ~160, Phase 3: ~100)
• Total with PR Refactor CLI to use crate library functions (eliminate code duplication) (#9) #10: ~450 lines of duplication eliminated
• Modules Deleted: src/bbl_format.rs (entire file)

Architecture

• ✅ CLI fully depends on crate library
• ✅ Single source of truth for all parsing logic
• ✅ Identical behavior guaranteed between CLI and library
• ✅ Simplified maintenance (fix bugs once)

Testing

• ✅ All unit tests pass
• ✅ Integration tests pass
• ✅ CLI output identical to reference implementation
• ✅ Debug mode works correctly
• ✅ All frame types parse correctly (H, G, E, etc.)

---

References

• Issue Refactor CLI to use crate library functions (eliminate code duplication) #9: Refactor CLI to use crate library functions (eliminate code duplication) #9
• PR Refactor CLI to use crate library functions (eliminate code duplication) (#9) #10: Refactor CLI to use crate library functions (eliminate code duplication) (#9) #10
• Commit 850fc27: Last good commit with ~140 lines eliminated

---

Notes for AI Agents

1. One Phase at a Time: Complete Phase 1 fully before starting Phase 2
2. Validate Between Phases: Run full test suite after each phase
3. Preserve Working Code: Don't refactor logic while migrating
4. Direction Matters: Always CLI → crate dependencies, never reverse
5. A/B Testing is Mandatory: Compare against reference implementation
6. Commit Granularity: One commit per phase with clear message
7. When in Doubt: Ask the human maintainer for clarification

Success Criteria: All tests pass + CLI output identical to reference + src/bbl_format.rs deleted