Skip to content

riscv-rt: include device in link.x #274

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

riscv-rt: include device in link.x #274

merged 1 commit into from
Mar 11, 2025

Conversation

romancardenas
Copy link
Contributor

This follows the approach of cortex-m-rt.

Related to #238

@romancardenas romancardenas requested a review from a team as a code owner March 8, 2025 10:01
@romancardenas romancardenas changed the title include device in link.x riscv-rt: include device in link.x Mar 8, 2025
Copy link
Contributor

@rmsyn rmsyn left a comment

Choose a reason for hiding this comment

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

I think as long as this is opt-in (as it is now), end users still have the flexibility of defining their own device.x.

Do we have any build tests that enable this feature?

LGTM

@romancardenas
Copy link
Contributor Author

romancardenas commented Mar 10, 2025

Will try to work on it. It is a bit painful, as we need to make files visible to the linker. Maybe a tests-riscv crate? We cannot use the tests crate as trybuild and cross-compiled projects don't get along very well.

@rmsyn
Copy link
Contributor

rmsyn commented Mar 11, 2025

Will try to work on it. It is a bit painful, as we need to make files visible to the linker. Maybe a tests-riscv crate? We cannot use the tests crate as trybuild and cross-compiled projects don't get along very well.

No worries, I wasn't aware it was such a pain. I think the automated tests would definitely be very useful, but their lack is not necessarily a blocker for this set of changes.

Whatever you want to do.

@romancardenas
Copy link
Contributor Author

Ok! I will merge this, open a similar PR, and then work on a better test scenario so we can experiment with the new features.

@romancardenas romancardenas added this pull request to the merge queue Mar 11, 2025
Merged via the queue into master with commit 3626b66 Mar 11, 2025
119 checks passed
@romancardenas romancardenas mentioned this pull request Mar 11, 2025
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