Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Add a pre_init function to be called at the start of the reset handler #71

Closed
wants to merge 1 commit into from

Conversation

jcsoo
Copy link
Contributor

@jcsoo jcsoo commented May 11, 2018

Implements #17

I kept the implementation as close to DEFAULT_HANDLER as possible. Unsure whether the pre_init function should be unsafe or not.

@japaric
Copy link
Member

japaric commented May 13, 2018

Thanks for the PR @jcsoo.

Unfortunately it can't be accepted as it is because this crate must compile on beta / stable by default. However, this could land behind an opt-in feature.

#51 is another take on this problem that uses the .pre_init_array support in the r0 crate. One of the biggest questions about this feature is how to deal with peripheral singletons: if you take the peripherals in pre-init then you can't use them in main; and if you take the peripherals in pre-init then you have to use a static to pass them to main but that makes them globally visible, which is bad, and imposes synchronazation overhead (and maybe Option.unwraping).

Look at my latest comment in #51 for my current thoughts on this issue.

And yes pre_init functions need to be marked unsafe because accessing a static variable from them is undefined behavior (they are not initialized at that point).

@jcsoo
Copy link
Contributor Author

jcsoo commented May 14, 2018

Thanks for the response. I did this initial pull request against 0.4.0 because I haven't updated my code to 0.5.0 yet, but would it make sense for me to rebase it against the current version, gated against an opt-in feature?

I see now how the uninitialized statics require the pre_init function to be unsafe.

Regarding things like peripheral singletons, wouldn't that be the concern of the consuming crate? I can see how a crate using strict RAII at the peripheral level wouldn't simply be able to release the singleton, but crates using a different peripheral access philosophy (or simply using volatile access as is used for FPU initialization) wouldn't necessarily have that issue.

Would a better approach be to make __RESET_VECTOR a weak symbol and override at that level?

@japaric
Copy link
Member

japaric commented May 15, 2018

would it make sense for me to rebase it against the current version, gated against an opt-in feature?

I'd prefer not to add Cargo features that require nightly to enable. I think it's feasible to implement this feature without unstable features though.

Regarding things like peripheral singletons, wouldn't that be the concern of the consuming crate?

Yes, that's what I said in the other thread. That issue can solved in a downstream crate.

Would a better approach be to make __RESET_VECTOR a weak symbol and override at that level?

I think that's too dangerous. The user may omit the initialization of static variables causing UB in every function that uses static variables.

@yodaldevoid
Copy link
Contributor

@jcsoo Is there a possibility you can update this in the near future so that it might be pulled in?

@korken89
Copy link
Contributor

korken89 commented Jul 30, 2018

Hi @japaric / @jcsoo, what is the status of this PR / similar feature? I am writing a bootloader in Rust, and I would need to do checks before anything else runs to jump to user code.
If there is something I can help with, give me a ping and I can take over this PR to move it forward.

The support for running some "pre init" code is crucial to many embedded programs.

@japaric
Copy link
Member

japaric commented Aug 3, 2018

Thanks for the original PR, @jcsoo. I'm going to close this in favor of #81 which has an implementation that works on stable.

@japaric japaric closed this Aug 3, 2018
bors bot added a commit that referenced this pull request Aug 13, 2018
81: Add a __pre_init function to be called at the start of the reset handler r=korken89 a=yodaldevoid

I needed this for a project so I went ahead and rebased and tweaked @jcsoo 's changes from #71. I will admit, I got a little impatient waiting and that also played into why I did this. If either @jcsoo or @japaric have an issue with this PR, please let me know and I will remove it. I apologize for toes that I have stepped on.

Now onto the PR. This is possibly the simplest implementation that is possible. No nightly features were used in this implementation. Also, there is no hand-holding for the end user; if they want to mess with uninitialized statics, that is on them.

If you would like me to add more documentation, please let me know what you would like to see.

Fixes #17 

Co-authored-by: Jonathan Soo <[email protected]>
Co-authored-by: Gabriel Smith <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants