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

Automatically add rom_functions.x to the linker scripts #2339

Closed
wants to merge 3 commits into from

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Oct 11, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Don't require users to manually add rom_functions.x when using esp-wifi

esp-hal now includes a (private) feature to do that. It's automatically activated by esp-wifi since we depend on esp-hal anyways

skip-changelog because of the added private feature in esp-hal

Testing

Examples still work. Apparently adding it manually in addition is not a problem (for lld .... but it is for gnu-ld)

esp-hal/Cargo.toml Outdated Show resolved Hide resolved
@bjoernQ bjoernQ added the skip-changelog No changelog modification needed label Oct 11, 2024
@bjoernQ bjoernQ marked this pull request as ready for review October 11, 2024 14:49
examples/build.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor Author

bjoernQ commented Oct 11, 2024

I guess the failing HIL test is unrelated to this PR

Comment on lines +11 to +14
#IF __include_rom_coexist_and_phy
INCLUDE "rom_coexist.x"
INCLUDE "rom_phy.x"
#ENDIF
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep these on incompatible devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given it's an internal feature my thinking was it's doesn't need additional safety belts - we could certainly check it in build.rs if we think we should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

T.b.h long term we should change esp-wifi-sys to include all rom functions via rom_functions.x but maybe when we have some more interesting reasons to justify a new version

@Sycrosity
Copy link
Contributor

This is a nice QOL change! I've been manually added build.rs scripts to avoid .cargo/config.toml files with the current system.

@MabezDev
Copy link
Member

What about if all the linker scripts lived in esp-hal? For instance, rom-functions.x definitely could and would save an internal feature here - not sure about the phy or coexist linker files though

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Oct 16, 2024

What about if all the linker scripts lived in esp-hal? For instance, rom-functions.x definitely could and would save an internal feature here - not sure about the phy or coexist linker files though

They all definitely could - one annoyance is when updating drivers we might need to change them since it often happens that ROM functions are replaced by the libraries (and apparently just defining them weak would cause problems)

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Oct 16, 2024

Thinking about it - yes let's do that. We just need to make sure we are in sync with what commit the drivers are from - shouldn't be a huge problem

@bjoernQ bjoernQ closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants