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

fix esp32c3 uart initialization #1156

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Conversation

ju6ge
Copy link
Contributor

@ju6ge ju6ge commented Feb 11, 2024

Implement uart initialization according to the esp32c3 technical reference manual section 26.5.2.1

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

esp-hal/src/uart.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 12, 2024

Thanks a lot!

I think I'm not a huge fan of having a completely different init for one chip. I understand you probably can only test on C3 and don't want to break things for other chips

Probably it would be good to have this for other chips as well (if they have same functionality, e.g. mem_clk_en, restting fifos (some chips have dedicated rests for rx/tx fifo)

I like we have the update implemented for C3 here - no idea why it was forgotten initially (or maybe lost during a refactoring)

@ju6ge
Copy link
Contributor Author

ju6ge commented Feb 12, 2024

Possibly update was overlooked, because the relevant bit is in a different register.

The reason i put it in a different function called init is to stick to the same pattern like for sync_regs. This makes easily extendable to other chips that have a more complex init routine like the esp32c3. I did not have time to check what other chips have the same uart implementation, I would not be surprised if there are more and also I can only test for the c3.

@ju6ge ju6ge force-pushed the fix/esp32c3-uart-init branch from e2465c1 to 25f1204 Compare February 18, 2024 11:55
@ju6ge
Copy link
Contributor Author

ju6ge commented Feb 18, 2024

Hey guys,

I just rebased this to the newest commits in main. Not sure if you want me to change anything else about this fix. If there is still something that needs to be changed let me know.

Kind regards
ju68e

@ju6ge ju6ge force-pushed the fix/esp32c3-uart-init branch 2 times, most recently from 6d719cf to cff0a85 Compare February 21, 2024 10:16
@ju6ge
Copy link
Contributor Author

ju6ge commented Feb 21, 2024

@bjoernQ I update the PR. It now uses enable_peripheral(). I also added a TODO comment about moving to reset_peripheral() as soon at exists.

esp-hal/src/uart.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 21, 2024

Besides a typo in a comment and the annoying CHANGELOG.md conflict looks good.

Just out of curiosity: I tried e.g. the advanced_serial.rs example on main and your branch and both just work. It's definitely good to do the initialization the way the TRM suggests and I'm fine with having it just for C3 for now - but still I wonder what was causing your problems. What config did you use? (e.g. the example uses 115200 8N1) What dev-board do you use? (or some custom PCB?) Would be very good to be able to reproduce it to create some meaningful automated tests in future (to avoid regressions)

@ju6ge ju6ge force-pushed the fix/esp32c3-uart-init branch from cff0a85 to 68264ea Compare February 21, 2024 14:01
@ju6ge ju6ge force-pushed the fix/esp32c3-uart-init branch from 68264ea to 8442f41 Compare February 21, 2024 14:04
@ju6ge
Copy link
Contributor Author

ju6ge commented Feb 21, 2024

@bjoernQ rebased onto master again and fixed typos.

I am using a dev-board for the esp32c3 from ai-thinker. I am building a thingy to receive data from multiple smart-meters via SML. So my baudrate is at 9600 and other settings are at default. Since I want to read multiple smart meters I need multiple receive pins and have written a UartMultiChannel class that will let me easily reconfigure the active pins via IOMUX.

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

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 21, 2024

@bjoernQ rebased onto master again and fixed typos.

I am using a dev-board for the esp32c3 from ai-thinker. I am building a thingy to receive data from multiple smart-meters via SML. So my baudrate is at 9600 and other settings are at default. Since I want to read multiple smart meters I need multiple receive pins and have written a UartMultiChannel class that will let me easily reconfigure the active pins via IOMUX.

Sounds interesting and probably the reconfiguring is the difference. Thanks

Probably the same initialization should be done for other targets. I can look into that after this one got in.

Any thoughts @MabezDev @jessebraham ? Will merge this otherwise - don't see a reason not to

@jessebraham
Copy link
Member

jessebraham commented Feb 21, 2024

I think it's going to be quite difficult to track which state each driver is in for each chip, and that we should be adding functionality for all supported chips. We already have a bunch of partially implemented drivers in the HAL which nobody has found the time to get back to.

But I also don't really have the energy to deal with this right now, so feel free to just do whatever.

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 21, 2024

Going to merge - as said I will take care of the next steps then

@bjoernQ bjoernQ added this pull request to the merge queue Feb 21, 2024
Merged via the queue into esp-rs:main with commit 4bc1aaa Feb 21, 2024
17 checks passed
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