Skip to content

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 latest Migration Guide.
  • My changes are in accordance to the esp-rs developer guidelines

Extra:

Pull Request Details 📖

Split out of #4604.

This changes tx and rx methods to require PulseCode instead of supporting impl Into<PulseCode> and impl From<PulseCode>, respectively.

Supporting these traits has probably had limited value, was only added (by me) very recently, and removing them prepares for supporting more general data types (#4604 or whatever alternative turns out to be a better design).

This was also suggested by @bugadani in #4126 (review).

Testing

HIL tests.

which was probably of little value, anyway, and also in preparation for adding
more sophisticated Encoder data types
@JurajSadel JurajSadel added the trusted-author Allow the author of this Pull Request to run HIL tests and the `binary-size` test. label Dec 4, 2025
@github-actions
Copy link

github-actions bot commented Dec 4, 2025

[HIL trust list]

Trusted users for this PR (click to expand)

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Author @wisp3rwind was trusted for this PR via the trusted-author label.
They can now use /hil quick or /hil full.

@wisp3rwind
Copy link
Contributor Author

/hil quick

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

Triggered quick HIL run for #4616.

Run: https://github.com/esp-rs/esp-hal/actions/runs/19942537608

Status update: ✅ HIL (quick) run succeeded.

@wisp3rwind wisp3rwind marked this pull request as ready for review December 4, 2025 20:37
Copilot AI review requested due to automatic review settings December 4, 2025 20:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR simplifies the RMT (Remote Control) peripheral API by removing generic type support from transmission and reception methods. The change requires buffers to contain PulseCode directly rather than supporting arbitrary types through Into<PulseCode> and From<PulseCode> trait bounds.

Key Changes:

  • Removed generic type parameters from TxTransaction and RxTransaction structs
  • Updated all TX methods to accept &[PulseCode] directly instead of generic impl Into<PulseCode>
  • Updated all RX methods to accept &mut [PulseCode] directly instead of generic impl From<PulseCode>

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
esp-hal/src/rmt/writer.rs Updated write method to accept &[PulseCode] directly and removed .into() conversion
esp-hal/src/rmt/reader.rs Updated read method to accept &mut [PulseCode] directly and removed .into() conversion
esp-hal/src/rmt.rs Removed generic type parameters from transaction structs and future types; updated all public API methods for both blocking and async channels
esp-hal/MIGRATING-1.0.0.md Added comprehensive migration guide with code examples showing how to update from u32 arrays to PulseCode arrays
esp-hal/CHANGELOG.md Added changelog entry documenting the breaking change; improved formatting consistency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@JurajSadel JurajSadel 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

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 :)

@bugadani bugadani added this pull request to the merge queue Dec 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 8, 2025
@bugadani bugadani added this pull request to the merge queue Dec 8, 2025
Merged via the queue into esp-rs:main with commit 2093260 Dec 8, 2025
46 of 47 checks passed
@wisp3rwind wisp3rwind mentioned this pull request Dec 9, 2025
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

trusted-author Allow the author of this Pull Request to run HIL tests and the `binary-size` test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants