Skip to content

Conversation

@Georges760
Copy link
Contributor

@Georges760 Georges760 commented Aug 24, 2023

  • Keeps existing API for uart, but wraps it in Tx and Rx sub-types
  • Adds split() method similar to embassy-rs nRF/stm32/rp for getting indepdendent TX and RX parts
  • Implements e-h traits for TX and RX types
  • Add esp32s3 example

Thank you!

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@Georges760
Copy link
Contributor Author

Do anyone know why the Instance type is not propagated to UartFuture in the new() constructor ?

It is well propagated from Uart to UartRx and UartTx in their new_inner() contructor.

Is it because UartFuture lives in a sub mod Asynch ?

@MabezDev
Copy link
Member

MabezDev commented Sep 4, 2023

Sorry, missed this completely!

To fix the current compilation errors, you just need to specify that the T used by the UartFuture is the same T from the UartRx/Tx. I.e UartFuture::new( => UartFuture::<T>::new(. This seems to be a quirk of async Rust, as normal functions can infer this usually.

@Georges760 Georges760 marked this pull request as ready for review September 5, 2023 14:31
@Georges760
Copy link
Contributor Author

Georges760 commented Sep 5, 2023

why do my cargo fmt is not the same as the one from CI ?

$ cargo fmt --version
rustfmt 1.6.0-stable (5680fa1 2023-08-23)

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

This is looking good functionality wise, but could we improve the example? What about sending the read bytes over a channel to the write to be written back out? As it stands the example isn't very interactive. We can also drop the AT command stuff for this example.

@MabezDev
Copy link
Member

Another alternative would be to instead of adding a new example, change the current embassy_serial one to use split by default.

@Georges760
Copy link
Contributor Author

Another alternative would be to instead of adding a new example, change the current embassy_serial one to use split by default.

OK I will do that. Sorry I have very few time for this now...

@Georges760 Georges760 closed this Sep 19, 2023
* Keeps existing API for uart, but wraps it in Tx and Rx sub-types
* Adds split() method similar to embassy-rs nRF/stm32/rp for getting indepdendent TX and RX
parts
* Implements e-h traits for TX and RX types
* Add esp32s3 example
@Georges760
Copy link
Contributor Author

Georges760 commented Sep 19, 2023

i did mess up my repo while rebasing, and this close the PR. sorry

@Georges760 Georges760 reopened this Sep 19, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks @Georges760! LGTM!

@MabezDev MabezDev merged commit 107732b into esp-rs:main Sep 20, 2023
SergioGasquez pushed a commit to SergioGasquez/esp-hal that referenced this pull request Sep 22, 2023
* Add support for splitting esp32 uart into TX and RX

* Keeps existing API for uart, but wraps it in Tx and Rx sub-types
* Adds split() method similar to embassy-rs nRF/stm32/rp for getting indepdendent TX and RX
parts
* Implements e-h traits for TX and RX types
* Add esp32s3 example

* finish implementing

* rebase examples

* declare new examples

* fix fmt

* rebase on 0.12.0

* fix fmt

* fix fmt

* merge embassy_serial_split example with embassy_serial

* fix changelog

* fix changelog
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.

2 participants