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

Implement UART type erasure #2381

Merged
merged 14 commits into from
Oct 25, 2024
Merged

Implement UART type erasure #2381

merged 14 commits into from
Oct 25, 2024

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 22, 2024 •
edited
Loading

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:

@bugadani bugadani mentioned this pull request Oct 22, 2024
Closed
@bugadani bugadani force-pushed the erase-uart branch 3 times, most recently from 40da306 to 5c4c0f3 Compare October 24, 2024 06:29
@bugadani bugadani marked this pull request as ready for review October 24, 2024 06:30
@bugadani bugadani force-pushed the erase-uart branch 5 times, most recently from 1ce8234 to e3aad0c Compare October 24, 2024 10:13
@bugadani
Copy link
Contributor Author

bugadani commented Oct 24, 2024 •
edited
Loading

Since 1.81 fat LTO dislikes something and miscompiles this... for C6 only for some weird reason. Nevermind a different test is broken with thin LTO...

@bugadani bugadani force-pushed the erase-uart branch 4 times, most recently from f67b298 to 0657965 Compare October 24, 2024 11:48
bjoernQ
bjoernQ reviewed Oct 25, 2024
View reviewed changes
esp-hal/src/uart.rs
fn interrupt() -> Interrupt;
fn interrupt(&self) -> Interrupt;

#[doc(hidden)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really related but ....

I wonder if we should doc-hide all those Instance/RegisterAccess traits 🤔

We do for some peripherals (e.g. I2c) but for most we don't - however it's something that would be just confusing to users in best case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the long run, I want to split out the RegisterBlock/signal part of the trait, and make it public. The rest (i.e. the actual driver implementations) will I think remain private. I'm hiding these things mostly as an "under construction" sign.

bjoernQ
bjoernQ approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - feel free to think about doc-hiding the Instance trait or merge as is

@bugadani
Copy link
Contributor Author

Are we fine with me disabling LTO in the test suite?

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 25, 2024

Are we fine with me disabling LTO in the test suite?

What would be the alternative?

@bugadani
Copy link
Contributor Author

Figuring out what's wrong, fixing the compiler and holding off the PR until the fix is released 😅

@bugadani bugadani added this pull request to the merge queue Oct 25, 2024
Merged via the queue into esp-rs:main with commit 681f4ef Oct 25, 2024
28 checks passed
@bugadani bugadani deleted the erase-uart branch October 25, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dominaezzz Dominaezzz Dominaezzz approved these changes

@bjoernQ bjoernQ bjoernQ 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.
3 participants
@bugadani @bjoernQ @Dominaezzz