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

UART wrapper (chrysn's cleanup) #143

Merged
merged 41 commits into from
Jan 13, 2025
Merged

UART wrapper (chrysn's cleanup) #143

merged 41 commits into from
Jan 13, 2025

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Jan 6, 2025

This PR just contains fixes relative to #39; let's keep discussion there, but it's a PR so that we can see what CI says.

src/uart.rs Outdated Show resolved Hide resolved
@chrysn
Copy link
Member Author

chrysn commented Jan 7, 2025

I've made a bit of a mess with which branch to push to, and GitHub's view of when things were pushed is inconsistent -- it shows "Use From for error conversion" before your review, but actually I don't think you've seen them. I don't think they are major, but maybe you'd like to revisit (best viewed individually)

and the recent 9e052af (but that's just sequence changes)

I'll do some squashing, trying to strike a balance between documenting the genesis and keeping small fixes out of the way; I'll force-push the tree-identical result.

@chrysn chrysn force-pushed the chrysn-impl_uart_wrapper branch from 9e052af to dcfeff9 Compare January 7, 2025 22:04
@chrysn
Copy link
Member Author

chrysn commented Jan 7, 2025

Squashed now into what I think is a sensible sequence of ocmmits, leaning towards keeping history.

  • GitHub is said to be a bit annoying when @-mentioning users in commit messages; removed the @.
  • Applied cargo fmt to all commits using git filter-branch -f --tree-filter 'cargo fmt; git add **/*.rs' fa66f7f0eec46679bf6c031f1d2588068c323972.. (This has no impact )
  • Squashed fixes where they are simple oversights
  • Squashed groups of commits where there was no significant difference. Note that for applying reviewers' comments, there is a batch feature in some (but not all) GitHub views of a PR
  • Reordered commits to reduce the chances of build failures (even though I don't expect to ever have a bisect hit in here, given that as a whole the PR just introduces a feature)

@chrysn chrysn marked this pull request as ready for review January 7, 2025 22:09
This funcitonality is not consistently exposed as functions in RIOT,
even when `periph_uart_reconfigure` is present. (For example,
samr21-xpro is missing it).
@chrysn
Copy link
Member Author

chrysn commented Jan 13, 2025

As a last-minute removal I had to ditch the get_[rt]x_pin methods -- those are just not available consistently in RIOT across boards; it's a niche feature probably only tested wherever anyone had a concrete use case and implemented it on the boards, and this would be the first time it is being checked for across the boards.

Enabling auto-merge already; that will fail anyway if there is any follow-up trouble (which I don't expect).

@chrysn chrysn enabled auto-merge January 13, 2025 11:48
@chrysn chrysn merged commit 8a41b4a into main Jan 13, 2025
89 checks passed
@chrysn chrysn deleted the chrysn-impl_uart_wrapper branch January 13, 2025 12:10
@kbarning
Copy link
Contributor

Sorry for not answering, I was very busy during the last weeks. Is there anything left to do @chrysn?

@chrysn
Copy link
Member Author

chrysn commented Jan 13, 2025

Sure, that was a time period where one is bound to get mixed availabilities.

I think all is done here :-)

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.

3 participants