Skip to content

Conversation

@ayodejiige
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings October 17, 2025 22:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the initialization of FlashJournal by moving its construction from the Board implementation into the partitions method of the ImxrtConfig trait. This change enables consumers of ec-slimloader-imxrt to construct the FlashJournal themselves with their own configuration.

Key Changes

  • The ImxrtConfig::partitions method is now async and returns a constructed FlashJournal instead of a raw Partition
  • The Partitions struct now contains a FlashJournal instead of a raw state partition
  • Journal initialization logic and error handling have been moved from the Board implementation to the trait implementer

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


pub struct Partitions {
pub state: Partition<'static, ExternalStorage, RW, NoopRawMutex>,
pub journal: FlashJournal<Partition<'static, ExternalStorage, RW>>,
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FlashJournal type is missing the mutex type parameter that was present in the original Partition (NoopRawMutex). This creates an inconsistency with the slots field on line 47 which still specifies NoopRawMutex. Verify that FlashJournal's type parameters are correct and consistent with the partition's mutex requirements.

Suggested change
pub journal: FlashJournal<Partition<'static, ExternalStorage, RW>>,
pub journal: FlashJournal<Partition<'static, ExternalStorage, RW, NoopRawMutex>>,

Copilot uses AI. Check for mistakes.
pub journal: FlashJournal<Partition<'static, ExternalStorage, RW>>,
pub slots: Vec<Partition<'static, ExternalStorage, RO, NoopRawMutex>, MAX_SLOT_COUNT>,
}

Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #[allow(async_fn_in_trait)] attribute is added without explanation. Consider adding a comment explaining why this lint suppression is necessary, especially since async functions in traits have specific stability and compatibility implications.

Suggested change
// Allow async functions in traits for ImxrtConfig, as async trait methods are required for this API.
// Note: async functions in traits are currently unstable in Rust. This lint suppression is intentional,
// and should be revisited when the feature is stabilized or if trait design changes.

Copilot uses AI. Check for mistakes.

/// The memory range an image is allowed to be copied to.
const LOAD_RANGE: Range<*mut u32>;

Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this trait method async is a breaking change that moves error handling responsibility to trait implementers. The original implementation had explicit panic handling with a descriptive message. Consider documenting the expected error handling behavior for FlashJournal::new failures, or provide guidance on how implementers should handle initialization errors.

Suggested change
/// Initializes the partitions required for bootloading.
///
/// # Error Handling
/// Implementers are responsible for handling any errors that may occur during
/// initialization (e.g., failures in `FlashJournal::new`). It is recommended to
/// log errors with a descriptive message and either panic or return a default value,
/// depending on the application's requirements. Consistent error handling should be
/// ensured across all implementations.

Copilot uses AI. Check for mistakes.
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