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

Use ROM memcpy and friends over compiler-builtins function #1255

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Mar 8, 2024

nm examples/target/xtensa-esp32s3-none-elf/release/gpio_interrupt | grep memcpy shows 0x400011f4 on esp32s3, so it looks like this is correctly overriding the linkage.

@bjoernQ maybe you were using PROVIDE which I think only "fills" in the symbol if its missing.

I think the last time memcpy from ROM was enabled, bad things with esp-wifi happened, @bjoernQ do you recall this, and would you mind testing?

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 8, 2024

@bjoernQ maybe you were using PROVIDE which I think only "fills" in the symbol if its missing.

I thought I was overriding the symbol, not providing but I see the changed symbol here. Whatever 🤷‍♂️

I think the last time memcpy from ROM was enabled, bad things with esp-wifi happened, @bjoernQ do you recall this, and would you mind testing?

I think there was something the other way around. For some chips esp-wifi already uses the ROM functions and some crypto (?) driver example in esp-hal broke 🤔 But can't find the issue - not in esp-wifi and not in esp-hal issues

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 8, 2024

Found the issue: https://github.com/esp-rs/esp-wifi/issues/245

Hopefully just the driver makes assumptions which we can fix - or maybe it's just working now

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 8, 2024

So ... everything works except ESP32-S2 BUT it also doesn't work with esp-hal 0.16.0

It worked with esp-hal 6a663f8 however 😲

Oh I see: 0240e2b breaks it

@jessebraham
Copy link
Member

cc @playfulFence

@MabezDev MabezDev added the skip-changelog No changelog modification needed label Mar 8, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 11, 2024

Just double checked: I can make esp-wifi work with this by locally commenting out crate::soc::trng::ensure_randomness();

So, it's definitely not a problem of this PR

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 11, 2024

Just double checked: I can make esp-wifi work with this by locally commenting out crate::soc::trng::ensure_randomness();

So it's definitely not a problem of this PR

@MabezDev MabezDev marked this pull request as ready for review March 11, 2024 09:48
@playfulFence
Copy link
Contributor

playfulFence commented Mar 11, 2024

I will investigate this problem

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.

LGTM

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM, hopefully the TRNG issues will be resolved tomorrow morning!

@jessebraham jessebraham added this pull request to the merge queue Mar 11, 2024
Merged via the queue into esp-rs:main with commit 301462a Mar 11, 2024
18 of 19 checks passed
@MabezDev MabezDev mentioned this pull request Jul 11, 2024
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