Skip to content

Audit flexspi HAL for panics#535

Merged
kurtjd merged 1 commit intoOpenDevicePartnership:mainfrom
kurtjd:audit-flexspi
Dec 23, 2025
Merged

Audit flexspi HAL for panics#535
kurtjd merged 1 commit intoOpenDevicePartnership:mainfrom
kurtjd:audit-flexspi

Conversation

@kurtjd
Copy link
Contributor

@kurtjd kurtjd commented Dec 18, 2025

Removes unwraps and most indexing/slicing (two cases of slicing were left in but are very easy to see they can never panic) from the flexspi HAL.

I use this pattern several times for copying a remainder (which may be less than a 4 byte chunk):

for (to, from) in remainder.iter_mut().zip(data.to_le_bytes().iter()) {
    *to = *from;
}

since it avoids the need to use copy_from_slice (with a calculated min length). However, this may be slightly less efficient than the equivalent copy_from_slice especially since the compiler should be able to elide the panic path as remainder size can be determined statically (as_chunks::<N> guarantees statically that remainder length is less than N). Open to using the slice method with an allow lint if folks prefer.

Testing

The changes in peripheral.rs I was able to test by copying the unit tests to another project with my new implementations and verifying they pass (speaking of, I can't figure out how to get cargo test to work here).

My other changes I at least tested using the flexspi-embedded-storage and flexspi-nor-flash examples on the 685evk. Still having issues getting the flexspi-nor-flash example working properly as I'm having trouble receiving defmt logs to verify it's functioning correctly.

Resolves OpenDevicePartnership/embedded-services#643

@kurtjd kurtjd self-assigned this Dec 18, 2025
@kurtjd kurtjd requested a review from a team as a code owner December 18, 2025 21:05
@kurtjd kurtjd added the enhancement New feature or request label Dec 18, 2025
@kurtjd kurtjd moved this to In review in Embedded Controller Dec 18, 2025
@jerrysxie
Copy link
Contributor

jerrysxie commented Dec 19, 2025

@kurtjd flexspi-storage-service example is not working? I believe it requires loading the image into RAM. You can look at how to do it from this PR: #336.

@kurtjd
Copy link
Contributor Author

kurtjd commented Dec 19, 2025

@kurtjd flexspi-storage-service example is not working? I believe it requires loading the image into RAM. You look at how to do it from this PR: #336.

Ah thanks, I will give that a try.

felipebalbi
felipebalbi previously approved these changes Dec 19, 2025
@kurtjd kurtjd force-pushed the audit-flexspi branch 2 times, most recently from bafd8aa to 8e9fcc1 Compare December 19, 2025 18:19
@kurtjd kurtjd added the BREAKING CHANGE PR causes a breaking change label Dec 19, 2025
@kurtjd kurtjd force-pushed the audit-flexspi branch 2 times, most recently from 2b4b40f to 62b50b6 Compare December 19, 2025 19:55
@jerrysxie jerrysxie self-requested a review December 19, 2025 22:14
jerrysxie
jerrysxie previously approved these changes Dec 19, 2025
Copy link
Contributor

@jerrysxie jerrysxie left a comment

Choose a reason for hiding this comment

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

Can you add a testing section to the PR description to indicate which flash example was ran with this change?

@jerrysxie
Copy link
Contributor

Can you add a testing section to the PR description to indicate which flash example was ran with this change?

Oh, and please announce the breaking change on Zulip, once it is ready to be merged.

jerrysxie
jerrysxie previously approved these changes Dec 20, 2025
@kurtjd kurtjd merged commit ddbfae8 into OpenDevicePartnership:main Dec 23, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Embedded Controller Dec 23, 2025
@kurtjd
Copy link
Contributor Author

kurtjd commented Dec 23, 2025

Can you add a testing section to the PR description to indicate which flash example was ran with this change?

Oh, and please announce the breaking change on Zulip, once it is ready to be merged.

Done.

) -> Result<(), NorStorageBusError> {
if cmd.data_bytes.is_some() && cmd.data_bytes.unwrap() > MAX_TRANSFER_SIZE_PER_COMMAND as u32 {
// `program_lut` expects data_bytes to be available so we bail out early if that's not the case
let Some(data_bytes) = cmd.data_bytes else {

Choose a reason for hiding this comment

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

Maybe:

let data_bytes = cmd.data_bytes.ok_or(NorStorageBusError::StorageBusInternalError)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah that is cleaner, unfortunately already merged :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKING CHANGE PR causes a breaking change enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Audit flexspi HAL

6 participants