Skip to content

refactor: comprehensive code review fixes across core and programmer crates#15

Merged
ArthurHeymans merged 3 commits intomasterfrom
FullReview
Feb 23, 2026
Merged

refactor: comprehensive code review fixes across core and programmer crates#15
ArthurHeymans merged 3 commits intomasterfrom
FullReview

Conversation

@ArthurHeymans
Copy link
Owner

Fix critical bugs, reduce code duplication, and improve Rust idioms
based on a full code review of the codebase.

Critical fixes:

  • operations::write() now respects max_write_len() to prevent data corruption
  • Guard integer underflow in smart_write_region when data is empty
  • Log data loss risk when erase+restore fails during region writes
  • Handle erase blocks extending both before AND after a write region

High priority:

  • Add multi-I/O read support (dual/quad) to SpiFlashDevice::read()
  • Add Box blanket impl for trait object usage
  • Extract default_execute/default_execute_with_vec helpers to deduplicate
    SpiMaster::execute() across 7 programmer crates

Medium priority:

  • Add addr context to ReadError, WriteError, VerifyError variants
  • Replace process::exit(1) with proper error returns in CLI commands
  • Add descriptive messages to assert_eq! in operations.rs
  • Log warnings instead of silently discarding exit_4byte_mode errors
  • Deduplicate FlashContext methods across cfg blocks
  • Standardize LayoutError::IoError to use Display formatting consistently

Low priority:

  • Name magic timing constants in spi25.rs (WRSR_POLL_US, etc.)
  • Add WpError: std::error::Error and Display for LayoutSource
  • Use VecDeque instead of Vec for WASM StatusLog (O(1) vs O(n) removal)
  • Use strip_prefix in parse_number
  • Extract shared format_size utility for human-readable byte formatting
  • Add aliases field to ProgrammerInfo
  • LayoutError::IoError now carries inner error string

…crates

Fix critical bugs, reduce code duplication, and improve Rust idioms
based on a full code review of the codebase.

Critical fixes:
- operations::write() now respects max_write_len() to prevent data corruption
- Guard integer underflow in smart_write_region when data is empty
- Log data loss risk when erase+restore fails during region writes
- Handle erase blocks extending both before AND after a write region

High priority:
- Add multi-I/O read support (dual/quad) to SpiFlashDevice::read()
- Add Box<dyn FlashDevice> blanket impl for trait object usage
- Extract default_execute/default_execute_with_vec helpers to deduplicate
  SpiMaster::execute() across 7 programmer crates

Medium priority:
- Add addr context to ReadError, WriteError, VerifyError variants
- Replace process::exit(1) with proper error returns in CLI commands
- Add descriptive messages to assert_eq! in operations.rs
- Log warnings instead of silently discarding exit_4byte_mode errors
- Deduplicate FlashContext methods across cfg blocks
- Standardize LayoutError::IoError to use Display formatting consistently

Low priority:
- Name magic timing constants in spi25.rs (WRSR_POLL_US, etc.)
- Add WpError: std::error::Error and Display for LayoutSource
- Use VecDeque instead of Vec for WASM StatusLog (O(1) vs O(n) removal)
- Use strip_prefix in parse_number
- Extract shared format_size utility for human-readable byte formatting
- Add aliases field to ProgrammerInfo
- LayoutError::IoError now carries inner error string
Bug fixes that were accidentally reverted by the previous review commit:
- Restore WpRange::from_start_end inclusive end fix (len = end - start + 1)
  to avoid off-by-one when end is inclusive
- Restore is_valid_range u64 arithmetic to prevent u32 truncation when
  len > u32::MAX
- Restore plan_optimal_erase buffer_offset u64 comparison to handle
  region_end == u32::MAX correctly

Additional fixes from PR review:
- Add log::warn when QPI read mode falls back to single I/O (both in
  operations.rs and spi_device.rs) so users know about the performance
  degradation
- Use ProgrammerError instead of misleading ReadError{addr:0} in
  default_execute_with_vec short-read path
- Fix SFDP test mock to use maybe_async annotations matching the async
  SpiMaster trait definition
- Run cargo fmt to fix formatting in operations.rs, spi_device.rs,
  unified.rs, and flash.rs (long lines wrapped by rustfmt)
- Gate ToString and vec imports behind static-chips feature in
  database.rs to fix unused-import clippy warning when the feature
  is not enabled
@ArthurHeymans ArthurHeymans merged commit 048baf5 into master Feb 23, 2026
5 checks passed
@ArthurHeymans ArthurHeymans deleted the FullReview branch February 23, 2026 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant