Skip to content

Conversation

@yconst
Copy link
Collaborator

@yconst yconst commented Jan 19, 2026

Summary

Fixes two code review issues in NVM legacy migration logic with minimal changes:

  1. Blocker: Migration now preserves user config by restoring runtime state before saving
  2. Medium: Legacy detection now uses full metadata validation instead of single field check

Changes

Fix 1: Migration Preserves Legacy Config

File: firmware/src/nvm/nvm.c (lines 403-426)

Added restore function calls in nvm_wl_migrate_legacy_config() before calling nvm_save_config():

  • frames_restore_config()
  • ADC_restore_config()
  • motor_restore_config()
  • sensors_restore_config()
  • observers_restore_config()
  • controller_restore_config()
  • CAN_restore_config()
  • traj_planner_restore_config()

This ensures that when nvm_save_config() reads from getters, it gets the legacy config data instead of current runtime state, preserving user settings during migration.

Fix 2: Robust Legacy Detection

File: firmware/src/nvm/nvm.c (lines 373-399)

Replaced single magic_marker check with nvm_wl_validate_metadata() in nvm_wl_detect_legacy_config(). This validates all metadata fields (magic_marker, metadata_version, data_size, and metadata_checksum) instead of just one field, preventing misclassification of corrupted configs.

Implementation Details

Both fixes reuse existing, well-tested functions:

  • Fix 1 uses the same restore functions already used in nvm_load_config()
  • Fix 2 uses nvm_wl_validate_metadata() already used in scan and load operations

Total lines changed: ~10 lines across 2 functions
New code added: 0 (only calls to existing functions)
Risk: Low (affects only legacy migration path)

Testing

  • ✅ Debug build: Success
  • ✅ Release build: Success
  • ✅ No compilation errors or warnings related to changes
  • ⏸️ Hardware tests require device setup (to be run by reviewer)

Edge Cases Considered

  1. Corrupted Legacy Config: Migration won't be attempted, device boots with defaults
  2. Corrupted New-Format Config: Full validation fails, attempts legacy detection
  3. Partial Restore Failure: Restore functions handle invalid data gracefully
  4. Migration After CAN ID Change: Legacy CAN ID is preserved correctly

@yconst yconst merged commit 4908c8d into master Jan 19, 2026
15 checks passed
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