Skip to content

Add pre-default-start-trap feature #337

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

Merged
merged 5 commits into from
Aug 9, 2025

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Aug 7, 2025

In esp-hal we offer the "flip-link" feature.

When SP moves below RAM an exception is raised but we need a way to check that situation and move SP into RAM again before further processing.

One way would be to define our own _default_start_trap - another way would be to have a way to execute custom code before _default_start_trap which this PR implements.

Not sure if this is the best way to do it but it's simple and should work good enough.

This should be the last missing step to use riscv-rt in esp-hal. (see esp-rs/esp-hal#2390)

Let me know if you want me to change anything

@bjoernQ bjoernQ requested a review from a team as a code owner August 7, 2025 11:40
Copy link
Contributor

@romancardenas romancardenas left a comment

Choose a reason for hiding this comment

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

LGTM! Could you include a sample example in the docs to illustrate how to implement the new routine?

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Aug 8, 2025

I tried to come up with a more or less interesting and realistic example - but it might not be brief enough, let me know if I should reduce it to a minimum

@romancardenas
Copy link
Contributor

Usually, we just include an empty implementation with a comment like // your code goes here, just to avoid too long/complex examples.

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Aug 8, 2025

Usually, we just include an empty implementation with a comment like // your code goes here, just to avoid too long/complex examples.

Makes sense - will change it 👍

romancardenas
romancardenas previously approved these changes Aug 8, 2025
Copy link
Contributor

@romancardenas romancardenas left a comment

Choose a reason for hiding this comment

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

LGTM! A few comments:

  • should the docs explicitly say that the new symbol must be in the .trap.start section?

  • currently, the new symbol is only used in direct mode and in exceptions with vectored mode. Do you need this to be included in the _continue_trap symbol for vectored interrupts?

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Aug 8, 2025

Thanks for the quick reply!

should the docs explicitly say that the new symbol must be in the .trap.start section?

It's probably worth to add a recommendation to avoid people running into linker errors later. I will add that 👍

currently, the new symbol is only used in direct mode and in exceptions with vectored mode. Do you need this to be included in the _continue_trap symbol for vectored interrupts?

For me at least it's only needed for exceptions (in vectored mode). I guess having it for everything in direct mode is fine but I can't think of a case where I need it for vectored interrupts. Should I note this in the docs? (i.e. that it won't get called for vectored interrupts)

@romancardenas
Copy link
Contributor

Sounds good to me. Let's document everything and leave it as it is

Copy link
Contributor

@romancardenas romancardenas 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

@romancardenas romancardenas added this pull request to the merge queue Aug 9, 2025
Merged via the queue into rust-embedded:master with commit 6c0c255 Aug 9, 2025
139 checks passed
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.

2 participants