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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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.

@MabezDev MabezDev force-pushed the fix-core1-startup branch from 19c3401 to 3973b17 Compare March 13, 2024 22:19
@yanshay
Copy link
Contributor

yanshay commented Mar 13, 2024

I can confirm its working for me.

MabezDev added 2 commits March 13, 2024 22:31
@MabezDev
Fix core1 startup
860373a 
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.
@MabezDev
Improve multicore example
8b72c26
@MabezDev MabezDev force-pushed the fix-core1-startup branch from 5e51b80 to 84a6280 Compare March 13, 2024 22:31
@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
@MabezDev MabezDev force-pushed the fix-core1-startup branch from 84a6280 to 7d93d2e Compare March 13, 2024 22:35
@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 force-pushed the fix-core1-startup branch from e0e18e1 to 365dcc3 Compare March 14, 2024 12:45
@MabezDev MabezDev marked this pull request as ready for review March 14, 2024 12:47
bjoernQ
bjoernQ approved these changes Mar 14, 2024
View reviewed changes
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

jessebraham
jessebraham approved these changes Mar 14, 2024
View reviewed changes
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
@MabezDev @yanshay
Fix core1 startup (esp-rs#1286)
14017a4 
* 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
@MabezDev MabezDev mentioned this pull request Mar 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessebraham jessebraham jessebraham approved these changes

@bjoernQ bjoernQ bjoernQ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MabezDev @yanshay @jessebraham @bugadani @bjoernQ