Skip to content

Add pre-default-start-trap feature - #337

Merged
romancardenas merged 5 commits into
rust-embedded:masterfrom
bjoernQ:pre_default_start_trap
Aug 9, 2025
Merged

romancardenas merged 5 commits into
rust-embedded:masterfrom
bjoernQ:pre_default_start_trap

Conversation

@bjoernQ

@bjoernQ bjoernQ commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

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

@romancardenas romancardenas left a comment

Copy link
Copy Markdown
Contributor

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

bjoernQ commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@bjoernQ

bjoernQ commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

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

@romancardenas romancardenas left a comment

Copy link
Copy Markdown
Contributor

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

bjoernQ commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

@romancardenas romancardenas left a comment

Copy link
Copy Markdown
Contributor

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