-
Notifications
You must be signed in to change notification settings - Fork 5
Feat: Add uart support #30
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
Conversation
👋 Hello Dzarda7, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
|
@antmak @erhankur WDYT? I implemented only the stuff we need, feel free to suggest changes or request other features. Few things that came to my mind during this:
Thanks! |
Thanks, yes, that is exactly what I meant. We might want to adapt the runtime approach for some functions too I believe, for example large vs normal flashing, but there might be simpler solution for that, we will see.
Yes, the issue is that I have no role in this repo. Thanks for letting me know. |
|
In view of #32 some points become clearer
I'm not aware many details, but I'd take newer chips into account, as their implementation is usually more mature. So looks good to implement things in
If it is not a stub-lib interface, then it makes sense |
a87b258 to
b5c73b6
Compare
|
This PR is now in state that it works for most of the targets. Sorry it took so long, I had other things to do and a long leave. Tested with:
It was implemented in a way that it should be possible to use it without previous initialization that ROM code does after boot. Before making final version, it needs to be decided how to handle support of things like UART interrupt receive and other stuff that only we need for esptool but you do not need it for openocd stub (and vice-versa). We talked about this in Shanghai with @antmak, @sobuch and @gerekon. Our stub can depend on previous initialization from ROM, so for example UART is already preinitialized. But your stub can be used in two modes, one of them requires zero initialization if I understand correctly. With this in mind we need to decide on how to implement the functions, which should be common, which not and how to differentiate them. We can either implement everything in a way that is expects no previous initialization. This will take much more time and testing with no real benefit for any of us except that all functions will work in all cases (with making our stub a bit bigger). Or we can somehow differentiate the functions that need ROM preinitialization. Either comment, or some suffix. WDYT @erhankur and @dobairoland? I have to say I tried first approach for the UART and it was quite exhaustive to check all the targets. I also thought about some init function that only you will use in openocd and others will be common, but I believe there will be more differences so either comment or suffix or different file for that is necessary. |
b5c73b6 to
58ee7c9
Compare
|
@Dzarda7 I might be missing some communication done without me but implementing just what is not in ROM sounds better for me. This would also help us to have MVP ASAP. And I don't see why couldn't we implement more parts later (if there will be a good reason for it). |
|
@dobairoland sorry, I might have not explained good enough. Point is that for openocd, guys implemented for example UART init function because they do not have benefit of initialization from ROM. We do not need, because ROM initializes this for us. The issue is that this spreads and for us to enable for example interrupt for UART, it may be done as simple as just attaching callback function for us, because ROM did the rest. Now if they do not need it, we can only do what is necessary for us. But lets say they decide to use it in openocd stub or already need it, but we implemented it with ROM preinit in mind. I believe in that case it will help both sides to somehow differentiate which functions depend on ROM preinit and which does not. Otherwise I believe it will get messy quite soon as they will be fixing the functions to initialize all and our stub will be getting bigger. |
|
@Dzarda7 UART isn’t critical for the OpenOCD stub since it's only used for TX logging. We also have a better alternative, logging to memory. So we can consider removing the UART log option entirely. Feel free to choose your path. For reference, we had added minimal UART init code before, and it worked fine.
The main challenge will be implementing flash and cache. The OpenOCD stub must avoid breaking existing settings when adding or removing breakpoints during application runtime. (Hot attach, runtime mode) |
58ee7c9 to
5fe8ee4
Compare
|
We agreed to add suffix and mention in the documentation when some function expects ROM preinitialization. Now this is ready for final review, thanks for the help and suggestions. |
There was a problem hiding this 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 adds comprehensive UART support for ESP chips and makes soc_utils.h part of the public API. The changes include new UART initialization, baudrate configuration, interrupt handling, and FIFO operations. The soc_utils.h header is moved from private/ to target/ to make it available in external projects like esp-flasher-stub.
Key Changes:
- Implemented UART support with initialization, baudrate setting, and interrupt handling functions
- Moved
soc_utils.hfrom private to public API (target/soc_utils.h) - Added UART register definitions and SOC capability headers for all supported ESP chips
- Refactored CMakeLists.txt files to use INTERFACE libraries for chips without custom implementations
Reviewed Changes
Copilot reviewed 66 out of 67 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/target/esp8266/src/uart.c | New UART implementation for ESP8266 with wait, init, baudrate, and flush functions |
| src/target/esp32/src/uart.c | Enhanced ESP32 UART with baudrate configuration and idle wait |
| src/target/esp32s2/src/uart.c | Added baudrate setting function for ESP32-S2 |
| src/target/esp32s3/src/uart.c | Added baudrate setting and uart_tx_switch for ESP32-S3 |
| src/target/esp32c2/src/uart.c | Added baudrate setting with XTAL frequency for ESP32-C2 |
| src/target/esp32c3/src/uart.c | Added baudrate setting function for ESP32-C3 |
| src/target/common/src/uart.c | Common UART implementation with interrupt handling and FIFO operations |
| src/target/base/include/target/uart.h | New public UART API with comprehensive documentation |
| src/target/base/include/target/soc_utils.h | Moved from private, contains register access macros |
| src/target/*/include/soc_caps.h | New SOC capability headers defining UART port counts |
| src/target/*/include/soc/uart_reg.h | UART register definitions for each chip |
| src/target/*/CMakeLists.txt | Build system changes for INTERFACE libraries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Yes, that might sense. UART logging was mainly used during stub bootstrap for new chips. Theoretically it could be useful if smth gets broken with logging to RAM. Logging to RAM is a bit more complex feature, so more error prone. But as you said in the most cases we will not use it. So maybe we could keep some reference implementation for initialization from scratch for 1 chip (esp32c3 ?) somewhere. So in future if someone who works on stub will face problem and logging to RAM won't be available yet, he will be able to enable special macro in OpenOCD stub, add URAT init for that chip and have the ability to debug it using UART logs. |
1cf4dec to
fae1c8d
Compare
fc74c50 to
35e1d42
Compare
.check_copyright_config.yaml
Outdated
| include: | ||
| - src/target/*/ld/*.ld | ||
| - example/ld/*.ld | ||
| - src/target/*/include/soc/uart_reg.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't uart_reg.h has proper license info? If not, it is ok to modify the license part. We don't need to copy paste directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the change, this item should not go into the ignore list but a separate section defined for it accepting Apache 2.0 only. This comment applies only if we stick with Apache 2.0 option only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copy pasted from ESP-IDF and did small modification. If I understand correctly, I cannot change license to Apache 2.0 OR MIT as the file is distributed under Apache 2.0 only. In that case I added section into .check_copyright_config.yaml as @dobairoland suggested. I might be wrong here definitely, but this seems to be the correct approach as we discussed with @dobairoland. Feel free to correct me please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erhankur please explain your point of view about this to @dobairoland and why we kept it this way, thanks.
10f7e66 to
8eb314a
Compare
|
@erhankur @dobairoland I believe I addressed all the issues, PTAL again. Left some threads open so you have context, feel free to close them if you consider it solved, thanks. |
dobairoland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
f0ab8fe to
367dd6f
Compare
|
I just fixed copyright I missed for esp8266, that is why pre-commit failed. Thanks for the quick review @dobairoland. |
erhankur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dzarda7 I did a quick TX test (log_uart) with S3, but it failed. Added note about it. Should I test for all targets?
367dd6f to
616a2a6
Compare
This commit adds support for UART with interrupt possibility and helper functions especially for RX path
616a2a6 to
ec7beca
Compare
|
@erhankur I believe I addressed all the issues, thanks for the review. |
erhankur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dzarda7 LGTM. Thanks for taking my comments into account. Feel free to merge when you think it’s ready.
Description
This PR adds support for UART and makes
soc_utils.hpart of public API as we need it in esp-flasher-stub.Testing
Tested with esp-flasher-stub. Receive in interrupt and response works.
Checklist
Before submitting a Pull Request, please ensure the following: