-
Notifications
You must be signed in to change notification settings - Fork 8
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
Make further improvements to build.rs
#187
Conversation
The `memory.x` used here is a conservative one, so not all of the target's available memory might be used. It would be much better to include a more accurate configuration, depending on Cargo features set, but this is better than nothing.
Rebased, ready for review. |
With these features, `build.rs` and the HAL code can make sure that the correct configuration for the target hardware is selected.
I've come to the conclusion that having a default target selection is more confusing that not having one. Not selecting a target results in a clear error message, while selecting a target while forgetting to unselect the default one could be confusing to new users. "You have to select a target" is also easier to explain in documentation than the default target, as I'm currently finding out. Given this, I believe the default target is not worth it, especially since most users will select a target in their `Cargo.toml` anyway.
(continuing the discussion from #14) @david-sawatzke I went ahead and implemented full target selection support. I see that you commented in #14 and approved this pull request in the meantime. I still think this was worthwhile work. Please let me know what you think about the new commits. |
I just saw that the order of commits, as displayed here, is wrong. The first two commits should be switched. The reason for that is, that I regularly rebase while I develop locally, to make the resulting commits easier to read. Unfortunately, GitHub shows commits in pull requests sorted by time, not by actual order. Please rest assured, that the actual order (as shown by |
We also need to featuregate CAPT & DAC1 now (probably also previously, but now more obviously) |
Merging now, since the previous commit was approved, and the last one only removes some outdated comments.
I've opened #191. |
Based on #186. I recommend reviewing/merging that one first.