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

Fix core1 startup #1286

Merged
merged 4 commits into from
Mar 14, 2024
Merged

Fix core1 startup #1286

merged 4 commits into from
Mar 14, 2024

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Mar 13, 2024

The original code tried to set the stack pointer and call the closure in the same stack frame. This isn't possible, so the actual stack being used was the 8K stack setup by ROM code. If this stack overflowed, it could corrupt the ROM function .bss and .data which is why I was seeing intermittent panic messages. I still don't understand all the failure cases, but I know that now we are using the correct stack.

@MabezDev
Copy link
Member Author

cc: @yanshay Could you try this PR?
cc: @bjoernQ, this might solve the esp32 issue too - if it doesn't I have more ideas on what's going on and it probably depends on where the esp32 ROM stacks are found.

@yanshay
Copy link
Contributor

yanshay commented Mar 13, 2024

I can confirm its working for me.

The original code tried to set the stack pointer and call the closure
_in the same stack frame_. This isn't possible, so the actual stack
being used was the 8K stack setup by ROM code. If this stack overflowed,
it could corrupt the ROM function .bss and .data which is why I was
seeing intermittent panic messages. I still don't understand all the
failure cases, but I know that now we are using the correct stack
properly.
@jessebraham
Copy link
Member

Thank you for sticking it out with this! I can take a peek in the morning.

@MabezDev MabezDev marked this pull request as ready for review March 13, 2024 22:34
@bugadani
Copy link
Contributor

Shouldn't the same change be done for ESP32, too?

@MabezDev
Copy link
Member Author

Shouldn't the same change be done for ESP32, too?

Oops, I forgot they weren't sharing the code. I will do the same for the ESP32 tomorrow, thanks!

@MabezDev MabezDev marked this pull request as draft March 13, 2024 23:45
@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 14, 2024

Not really related to the PR but I really wonder what the initial SP is when core-1 gets out of reset. I guess it's hardwired in hardware (assuming the boot address is really the first instruction the core will execute) 🤔

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 14, 2024

Not really related to the PR but I really wonder what the initial SP is when core-1 gets out of reset. I guess it's hardwired in hardware (assuming the boot address is really the first instruction the core will execute) 🤔

Nevermind - found the information

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 14, 2024

It seems like the curtain of mystery is being lifted on the ESP32 issue: #1085 (comment)

@MabezDev MabezDev marked this pull request as ready for review March 14, 2024 12:47
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.

Code looks good to me and examples work fine. However, I never had code to reproduce the original issue. Seems @yanshay was already able to confirm this fixes the issues I guess we have a solution for the original problem

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, thanks again!

@jessebraham jessebraham added this pull request to the merge queue Mar 14, 2024
Merged via the queue into esp-rs:main with commit de19bb0 Mar 14, 2024
18 checks passed
@MabezDev MabezDev deleted the fix-core1-startup branch March 14, 2024 13:47
yanshay pushed a commit to yanshay/esp-hal that referenced this pull request Mar 16, 2024
* Fix core1 startup

The original code tried to set the stack pointer and call the closure
_in the same stack frame_. This isn't possible, so the actual stack
being used was the 8K stack setup by ROM code. If this stack overflowed,
it could corrupt the ROM function .bss and .data which is why I was
seeing intermittent panic messages. I still don't understand all the
failure cases, but I know that now we are using the correct stack
properly.

* Improve multicore example

* Change log

* apply fix to esp32
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.

5 participants