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.

Sign up for GitHub

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

Jump to bottom

Auto generate memory.x #216

Merged
merged 3 commits into from Apr 29, 2021
Merged

Auto generate memory.x #216

merged 3 commits into from Apr 29, 2021

Conversation

Piroro-hs
Copy link
Contributor

This PR adds ld feature, which enables the memory.x generation and prevent issues like #215.

@Piroro-hs Piroro-hs force-pushed the memory_x branch 2 times, most recently from 575c747 to 6ceef20 Compare April 21, 2021 16:55
Sh3Rm4n
Sh3Rm4n requested changes Apr 21, 2021
View reviewed changes
Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

I like, that the current memory.x file is removed now. It was only added to be able to build examples, but was not intended to be used by the user. Having the ld feature for an opt in correct memory.x files is the correct way to go, IMO 👍

I guess this feature is worth a mention in the CHANGELOG.md? :)

build.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Sh3Rm4n
Sh3Rm4n reviewed Apr 26, 2021
View reviewed changes
build.rs Show resolved Hide resolved
Sh3Rm4n
Sh3Rm4n reviewed Apr 26, 2021
View reviewed changes
build.rs Outdated Show resolved Hide resolved
@Piroro-hs Piroro-hs force-pushed the memory_x branch 2 times, most recently from da41e56 to 908e64c Compare April 26, 2021 16:46
Sh3Rm4n
Sh3Rm4n reviewed Apr 26, 2021
View reviewed changes
Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

Very much appreciate the documentation updates and moving the device-selected check towards the build.rs file is a good solution as well! :)

Nice work. Resolving the commented out devices in CI and after that, this looks good to be merged.

build.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Apr 27, 2021 •
edited

I was thinking about maybe adding the ld feature to the default features, as well as the rt feature. This should be much more user-friendly initially and experienced users can turn of default features anyways.

But I think, this is a discussion for later. Let's first release a version with memory.x auto-generation and wait for feedback. Just a note for the future.

Sh3Rm4n
Sh3Rm4n approved these changes Apr 28, 2021
View reviewed changes
Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

LGTM. If you don't say otherwise, I would merge it :)

@Sh3Rm4n Sh3Rm4n merged commit 31ca8a3 into stm32-rs:master Apr 29, 2021
@Piroro-hs Piroro-hs deleted the memory_x branch May 27, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rahix Rahix Rahix left review comments

@Sh3Rm4n Sh3Rm4n Sh3Rm4n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Piroro-hs @Sh3Rm4n @Rahix