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.

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

Merged
merged 11 commits into from
Jan 10, 2020
Merged

Make further improvements to build.rs #187

merged 11 commits into from
Jan 10, 2020

Conversation

hannobraun
Copy link
Member

Based on #186. I recommend reviewing/merging that one first.

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.
@hannobraun hannobraun marked this pull request as ready for review January 8, 2020 12:54
@hannobraun
Copy link
Member Author

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.
@hannobraun
Copy link
Member Author

(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.

@hannobraun
Copy link
Member Author

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 git log) is correct.

memory_32_8.x Outdated Show resolved Hide resolved
@david-sawatzke
Copy link
Member

We also need to featuregate CAPT & DAC1 now (probably also previously, but now more obviously)

@hannobraun
Copy link
Member Author

Merging now, since the previous commit was approved, and the last one only removes some outdated comments.

We also need to featuregate CAPT & DAC1 now (probably also previously, but now more obviously)

I've opened #191.

@hannobraun hannobraun merged commit a25af0d into lpc-rs:master Jan 10, 2020
@hannobraun hannobraun deleted the build.rs branch January 10, 2020 06:00
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