Repository navigation
Conversation
davecheney
left a comment
There was a problem hiding this comment.
Thank you for working on this. I have a piece of general feedback which I would ask you consider; I think this PR does to many things. The description says "add uart2 support", but the first file extends the UART structure to include parity support.
Would you consider breaking this into two parts, or at least breaking off the UART2 support so we can land that separately, then additional PRs to add parity, stop bits, etc, can be added later.
I know as a contribution you've waited quite a while for a review so my suggestion to break this apart into multiple PRs sounds counter productive, but I hope you'll trust me when I say to you that a smaller PR is easier to review and easier to land because its impact is well classified, and sets the stage for further followsups which are of similar size and hopefully land promptly.
Thank you for your consideration.
|
created a new pr #6075 that contains only uart feature |
UART support for esp32s3-generic target
I tested it on ESP32 S3 DevKitC-1 N16R8 + DFPlayer mini
with my devices it worked well, loopback test also worked
mostly code is copied from c3