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

Rmt: Don't use the paste macro #2976

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

wisp3rwind
Copy link
Contributor

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

This replaces constructs of the form

  • old: rmt.int_raw().read().[< ch $ch_num _tx_thr_event >]().bit()
  • new: rmt.int_raw().read().ch_tx_thr_event($ch_num).bit()

which removes the need for the paste! macro and its somewhat unwieldy [< ... >] syntax, since the PACs have indexed accessors for all registers and fields.

Github doesn't format the diff very well, it's much easier to review with git diff -b.

Note that this PR is relative to #2960, only the last commit is new.

Testing

  • Checked that the riscv rmt examples compile.
  • Ran a heavily modified RMT driver, including this change, on an ESP Rust Board in tx mode.
  • I didn't test xtensa, since I don't have a toolchain set up.

@bugadani bugadani added the skip-changelog No changelog modification needed label Jan 16, 2025
There's no need for it, the PACs have accessor methods that take channel
indices (which were already used in a few places).
@wisp3rwind wisp3rwind force-pushed the rmt-remove-paste-macro branch from 0be9683 to a286e37 Compare January 16, 2025 22:19
@wisp3rwind
Copy link
Contributor Author

Just rebased on main, with #2960 merged.

@bugadani
Copy link
Contributor

Github doesn't format the diff very well, it's much easier to review with git diff -b.

Github's "Hide whitespace" option also gives a decent diff :) It's a bit hidden, but very useful in my experience.

image

Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

Thank you so much for taking care of this!

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.

Thanks! This might even pave the way to move things out of the macro but it's a nice improvement on it's own

@bjoernQ bjoernQ added this pull request to the merge queue Jan 17, 2025
@wisp3rwind
Copy link
Contributor Author

Thanks! This might even pave the way to move things out of the macro but it's a nice improvement on it's own

Actually, I've done something like that locally, but I'm not sure whether it's really in a good form to upstream, yet. I'll probably push some other changes first.

Merged via the queue into esp-rs:main with commit e150a53 Jan 17, 2025
28 checks passed
@wisp3rwind wisp3rwind deleted the rmt-remove-paste-macro branch January 17, 2025 09:36
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.

3 participants