Enable consumer of ec-slimloader-imxrt to construct FlashJournal#29
Enable consumer of ec-slimloader-imxrt to construct FlashJournal#29ayodejiige wants to merge 1 commit intoOpenDevicePartnership:mainfrom
Conversation
There was a problem hiding this comment.
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::partitionsmethod is now async and returns a constructedFlashJournalinstead of a rawPartition - The
Partitionsstruct now contains aFlashJournalinstead of a raw state partition - Journal initialization logic and error handling have been moved from the
Boardimplementation 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>>, |
There was a problem hiding this comment.
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.
| pub journal: FlashJournal<Partition<'static, ExternalStorage, RW>>, | |
| pub journal: FlashJournal<Partition<'static, ExternalStorage, RW, NoopRawMutex>>, |
| pub journal: FlashJournal<Partition<'static, ExternalStorage, RW>>, | ||
| pub slots: Vec<Partition<'static, ExternalStorage, RO, NoopRawMutex>, MAX_SLOT_COUNT>, | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| // 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. |
|
|
||
| /// The memory range an image is allowed to be copied to. | ||
| const LOAD_RANGE: Range<*mut u32>; | ||
|
|
There was a problem hiding this comment.
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.
| /// 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. |
No description provided.