Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to latest PACs and resolve the (numerous) resulting errors #984

Merged
merged 15 commits into from
Nov 30, 2023

Conversation

jessebraham
Copy link
Member

@jessebraham jessebraham commented Nov 30, 2023

Well, this ended up being a monumental task 😅 Hope I never need to do anything like this again.

Apologies for the massive PR but unfortunately it's not really possible to do this incrementally. I'm not sure how well this can reasonably be reviewed, but things that should be focused on (IMO) include:

  • Crypto peripherals
    • I had to rewrite the alignment helper to use pointers; based on my testing the crypto examples still seem to work, but I have not tested extensively across devices.
  • I2S and UART
    • Other PRs were merged while I was working on this, and I had to rebase on main. I believe I have not broken anything but some additional confirmation will be needed

I will try to do another review of my changes (though I'm a little tired of looking at all this 😂) and do some more testing on hardware.

No user-facing changes here, so I have not updated CHANGELOG.md.

@jessebraham jessebraham added the skip-changelog No changelog modification needed label Nov 30, 2023
@jessebraham
Copy link
Member Author

jessebraham commented Nov 30, 2023

The AES and RSA drivers specifically probably needs some eyes on it. Example seems to work but not super confident in my changes.

@AnthonyGrondin if you're able to find some time to review the changes to these two drivers and the alignment helper that would be much appreciated, I think you probably understand these parts much better than I do 😅. No pressure, of course.

@jessebraham
Copy link
Member Author

I have self-reviewed this, and other than the aforementioned drivers I think everything should be okay.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Looks good from my point of view

@jessebraham
Copy link
Member Author

jessebraham commented Nov 30, 2023

RSA on ESP32-C6 seems to be broken (just the multiplication_example function), guess I tested every other crypto peripheral 😅 Looking into it.

Will test on other chips as well.

EDIT: ESP32 and ESP32-S3 both fail in the same way, other crypto examples seem fine. Really strange issue.

@jessebraham
Copy link
Member Author

Fixed the issue, based on my limited testing all crypto peripherals seem to be working now.

@AnthonyGrondin
Copy link
Contributor

RSA on ESP32-C6 seems to be broken (just the multiplication_example function), guess I tested every other crypto peripheral 😅 Looking into it.

Will test on other chips as well.

EDIT: ESP32 and ESP32-S3 both fail in the same way, other crypto examples seem fine. Really strange issue.

Oh yeah, I had forgot. Since the alignment changed, we have to update the bit shifting.
df8a53e fixes it.

I'll test those new changes with my implementation of bignum in esp-mbedtls

@bugadani
Copy link
Contributor

Considering that this PR is basically blocking all others, what would you think about merging as is and hammering out the remaining issues later?

@AnthonyGrondin
Copy link
Contributor

I've done a few testing with my current bignum impl. Self-tests and sync_server works, but both will soft-lock if I try to run them in release mode. I cannot tell if it's my implementation or a bug in the driver, as it's doing this when also using the current origin before the changes in the PACS.

@jessebraham
Copy link
Member Author

Okay, thanks for the updates. If the behaviour is the same prior to these changes then I will just go ahead and merge this to get other PRs unblocked. At least we're not in any worse shape than we were before, we can address these issues in a subsequent PR.

@jessebraham jessebraham added this pull request to the merge queue Nov 30, 2023
Merged via the queue into esp-rs:main with commit c9e0ad2 Nov 30, 2023
17 checks passed
@jessebraham jessebraham deleted the fixes/pacs branch November 30, 2023 19:02
@SergioGasquez SergioGasquez mentioned this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants