emb:esp32c3:implement _write by jtag output#1440
Conversation
Summary of ChangesHello @luoliwoshang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the debugging and output capabilities for bare-metal ESP32-C3 applications by integrating _write system call support through the USB Serial JTAG. It also refines the underlying RISC-V target configurations, adopting a more structured inheritance model and ensuring critical initialization sections are correctly handled during the build process. These changes are validated by a new regression test, providing a more robust and functional environment for ESP32-C3 development. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the _write syscall for the ESP32-C3 target to allow output via JTAG, which is a great addition for debugging. The changes include a new C file with the _write implementation, updates to linker scripts to ensure correct section mapping, and a new test script to validate these changes. Additionally, there's a nice refactoring of the RISC-V target configuration files, improving maintainability.
My review focuses on the new C implementation. I've suggested a couple of improvements:
- A type-safety improvement in a low-level write function.
- A correctness fix in
_writeto handle file descriptors as described in its documentation.
Overall, this is a solid contribution. The changes are well-structured and the inclusion of a test script is excellent.
| * @return Number of bytes written, or -1 on error | ||
| */ | ||
| ssize_t _write(int fd, const void *buf, size_t count) { | ||
| const char *data = (const char *)buf; |
There was a problem hiding this comment.
The function's documentation comment states that it only supports stdout (1) and stderr (2) and should return an error for other file descriptors. However, the current implementation doesn't check the fd value, causing it to write to the JTAG serial for any file descriptor. This contradicts the documentation and could lead to unexpected behavior. Please add a check to handle unsupported file descriptors.
if (fd != 1 && fd != 2) {
return -1;
}
const char *data = (const char *)buf;| static inline int usb_serial_jtag_ll_write_txfifo(const uint8_t *buf, uint32_t wr_len) { | ||
| int i; | ||
| for (i = 0; i < (int)wr_len; i++) { | ||
| if (!USB_SERIAL_JTAG.ep1_conf.serial_in_ep_data_free) | ||
| break; | ||
| USB_SERIAL_JTAG.ep1.rdwr_byte = buf[i]; | ||
| } | ||
| return i; | ||
| } |
There was a problem hiding this comment.
The loop counter i is an int while the length wr_len is a uint32_t. Casting wr_len to int could lead to issues if wr_len exceeds INT_MAX. It's safer to use uint32_t for the loop counter and the return type to maintain type consistency and avoid potential overflow problems, even if large writes are unlikely with the current FIFO size.
| static inline int usb_serial_jtag_ll_write_txfifo(const uint8_t *buf, uint32_t wr_len) { | |
| int i; | |
| for (i = 0; i < (int)wr_len; i++) { | |
| if (!USB_SERIAL_JTAG.ep1_conf.serial_in_ep_data_free) | |
| break; | |
| USB_SERIAL_JTAG.ep1.rdwr_byte = buf[i]; | |
| } | |
| return i; | |
| } | |
| static inline uint32_t usb_serial_jtag_ll_write_txfifo(const uint8_t *buf, uint32_t wr_len) { | |
| uint32_t i; | |
| for (i = 0; i < wr_len; i++) { | |
| if (!USB_SERIAL_JTAG.ep1_conf.serial_in_ep_data_free) { | |
| break; | |
| } | |
| USB_SERIAL_JTAG.ep1.rdwr_byte = buf[i]; | |
| } | |
| return i; | |
| } |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| * @param count Number of bytes to write | ||
| * @return Number of bytes written, or -1 on error | ||
| */ | ||
| ssize_t _write(int fd, const void *buf, size_t count) { |
There was a problem hiding this comment.
[Security/Correctness] Missing file descriptor validation
The documentation states "only stdout (1) and stderr (2) are supported" but the code doesn't validate the fd parameter. Any file descriptor (including 0, -1, or invalid values) will write to JTAG output.
Impact:
- Violates documented API contract
- Could cause information disclosure if code attempts to write to other file descriptors expecting failure
- Breaks POSIX compliance
Recommendation:
ssize_t _write(int fd, const void *buf, size_t count) {
// Only stdout (1) and stderr (2) are supported
if (fd != 1 && fd != 2) {
return -1; // EBADF: Bad file descriptor
}
const char *data = (const char *)buf;
// ... rest of implementation
| * @return Number of bytes written, or -1 on error | ||
| */ | ||
| ssize_t _write(int fd, const void *buf, size_t count) { | ||
| const char *data = (const char *)buf; |
There was a problem hiding this comment.
[Security] Missing NULL pointer check
No validation of buf before dereferencing. In bare-metal systems without MMU, _write(1, NULL, 10) could cause undefined behavior or crash.
Recommendation:
if (buf == NULL && count > 0) {
return -1; // EFAULT: Bad address
}
| * @param fd File descriptor (only 1 and 2 supported) | ||
| * @param buf Data buffer to write | ||
| * @param count Number of bytes to write | ||
| * @return Number of bytes written, or -1 on error |
There was a problem hiding this comment.
[Documentation] Inaccurate return value documentation
The documentation says "or -1 on error" but the implementation always returns count and never returns -1. This misleads developers about error handling.
Also, the function always reports success even when characters are dropped due to FIFO full (non-blocking behavior).
Recommendation: Update documentation to:
* @return Always returns count (number of bytes requested).
* NOTE: Due to non-blocking FIFO behavior, some characters may be dropped
* if the FIFO is full. The return value does NOT guarantee transmission.
targets/device/esp/esp32c3.c
Outdated
| */ | ||
| static void usb_serial_jtag_write_char(char c) { | ||
| // Check if TX FIFO has space | ||
| if (usb_serial_jtag_ll_txfifo_writable()) { |
There was a problem hiding this comment.
[Performance] Redundant FIFO check
The outer if check at line 350 is redundant since usb_serial_jtag_ll_write_txfifo() already checks FIFO availability internally (line 329). This causes 2 volatile register reads per character.
Impact: ~5-10% performance overhead on write operations.
Recommendation: Remove the outer check or inline the register write:
static void usb_serial_jtag_write_char(char c) {
if (usb_serial_jtag_ll_txfifo_writable()) {
USB_SERIAL_JTAG.ep1.rdwr_byte = (uint8_t)c; // Direct write
if (c == '\n') {
usb_serial_jtag_ll_txfifo_flush();
}
}
}
| * | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
| #ifndef _SOC_USB_SERIAL_JTAG_STRUCT_H_ |
There was a problem hiding this comment.
[Code Quality] Unnecessary header guard in .c file
This entire C file is wrapped in #ifndef _SOC_USB_SERIAL_JTAG_STRUCT_H_ header guards (lines 6-394). Header guards are only needed for .h files that might be included multiple times, not for .c files.
Recommendation: Remove the header guards or extract the struct definitions to a proper .h file if they need to be shared.
Code Review SummaryThis PR implements USB Serial JTAG output for ESP32-C3 with comprehensive testing. The implementation is functional and demonstrates solid embedded systems knowledge. However, several issues need addressing: Critical Issues (Must Fix):
Performance & Quality:
Strengths:
Please address the critical issues before merging. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1440 +/- ##
=======================================
Coverage 90.58% 90.58%
=======================================
Files 43 43
Lines 11429 11429
=======================================
Hits 10353 10353
Misses 914 914
Partials 162 162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luoliwoshang
force-pushed
the
fix/esp32c3jtagout
branch
from
b797b28 to
c668a10
Compare
This commit fixes the illegal instruction crash issue for ESP32-C3 and improves the architecture to support all ESP RISC-V chips through a proper llvm-target based approach. ## Problem 1. ESP32-C3 only supports RV32IMC (no A/D/F extensions) 2. Previous fix used hardcoded `targetName == "esp32c3"` check, which doesn't work for inherited targets (esp32c3-supermini, m5stamp-c3, etc.) 3. compiler-rt's atomic.c was included for all riscv32 targets, causing compilation failure on ESP chips (requires A extension) ## Solution ### 1. Create ESP RISC-V base configuration - Rename: `riscv32-nostart.json` → `riscv32-esp.json` - Change `llvm-target` from `riscv32-unknown-none` to `riscv32-esp-elf` - Change `cflags` from `-march=rv32imac` to `-march=rv32imc` ### 2. Update ESP32-C3 inheritance - Update `targets/esp32c3.json` to inherit from `riscv32-esp` - Automatically applies to all boards inheriting esp32c3: - esp32c3-supermini - m5stamp-c3 - esp32-c3-devkit-rust-1 - esp-c3-32s-kit - esp32c3-12f ### 3. Fix compiler-rt atomic.c inclusion - Exclude atomic.c for ESP targets (riscv32-esp-elf) - Keep atomic.c for other riscv32 targets that support A extension ### 4. Use llvm-target instead of targetName - Check `config.LLVMTarget == "riscv32-esp-elf"` instead of hardcoded name - Properly supports inheritance relationships - ESP series: `-march=rv32imc` - Other riscv32: `-march=rv32imac` ## Benefits ✅ Fixes illegal instruction crash (C.FLD) for all ESP32-C3 boards ✅ Fixes compiler-rt atomic.c compilation error ✅ Supports inheritance - all ESP32-C3 variants automatically fixed ✅ Follows ESP-IDF conventions (riscv32-esp-elf is official target triple) ✅ Scalable - future ESP RISC-V chips just inherit riscv32-esp.json ✅ Backward compatible - other riscv32 targets unchanged ## Files Changed - targets/riscv32-nostart.json → targets/riscv32-esp.json (rename + modify) - targets/esp32c3.json (update inherits) - internal/crosscompile/compile/rtlib/compiler_rt.go (exclude atomic.c for ESP) - internal/crosscompile/crosscompile.go (use LLVMTarget instead of targetName) Fixes: Issue goplus#1427 Related: PR goplus#1440 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
luoliwoshang
force-pushed
the
fix/esp32c3jtagout
branch
2 times, most recently
from
76201f4 to
7865382
Compare
luoliwoshang
force-pushed
the
fix/esp32c3jtagout
branch
from
7865382 to
88e511f
Compare
|
If the _write implementation in the current |
targets/device/esp/esp32c3.c
Outdated
| */ | ||
| static void usb_serial_jtag_write_char(char c) { | ||
| // Check if TX FIFO has space | ||
| if (usb_serial_jtag_ll_txfifo_writable()) { |
There was a problem hiding this comment.
It seems it's not a good implement, when the JTAG buffer is full, we should wait until available, that's we called blocked write
There was a problem hiding this comment.
However, in the future, we may support non-blocking way to implement it. See #1445
There was a problem hiding this comment.
_write is non-blocking and drops on full. The USB Serial JTAG TX FIFO is only 64B and the UART TX FIFO is ~128B on ESP32-C3, so long/bursty prints will be lost when those buffers fill.
In the ESP-IDF implementation, it will wait 50 milliseconds before choosing to discard the data. Perhaps here, to ensure data is not lost, we could keep blocking and waiting for usb_serial_jtag_ll_txfifo_writable()?
static void usb_serial_jtag_tx_char_no_driver(int fd, int c)
{
uint8_t cc = (uint8_t)c;
// Try to write to the buffer as long as we still expect the buffer to have
// a chance of being emptied by an active host. Just drop the data if there's
// no chance anymore.
// When we first try to send a character and the buffer is not accessible yet,
// we wait until the time has been more than TX_FLUSH_TIMEOUT_US since we successfully
// sent the last byte. If it takes longer than TX_FLUSH_TIMEOUT_US, we drop every
// byte until the buffer can be accessible again.
do {
if (usb_serial_jtag_ll_txfifo_writable()) {
usb_serial_jtag_ll_write_txfifo(&cc, 1);
if (c == '\n') {
//Make sure line doesn't linger in fifo
usb_serial_jtag_ll_txfifo_flush();
}
//update time of last successful tx to now.
s_ctx.last_tx_ts = esp_timer_get_time();
break;
}
} while ((esp_timer_get_time() - s_ctx.last_tx_ts) < TX_FLUSH_TIMEOUT_US);
}
- Update newlib version to esp-4.3.0_20250211-patch4 - Includes weak symbol support for _write() syscall - Enables custom _write() implementation override Depends on: goplus/newlib#9
luoliwoshang
force-pushed
the
fix/esp32c3jtagout
branch
from
88e511f to
832457e
Compare
Change _write() implementation from non-blocking to blocking mode. Now waits until TX FIFO has space before writing instead of dropping characters when FIFO is full.
luoliwoshang
force-pushed
the
fix/esp32c3jtagout
branch
from
b45b56c to
fb79304
Compare
4 participants
part of #1427
ESP32-C3 USB Serial JTAG Output Support
Overview
Implements USB Serial JTAG output support for ESP32-C3, enabling
printf()and other standard output functions to work in bare-metal applications without external UART connections.Why USB Serial/JTAG Instead of UART?
Core reason: All ESP32-C3 chips have built-in USB Serial/JTAG, but not all development boards have USB-UART converter chips!
According to ESP-IDF USB Serial/JTAG Controller Documentation:
Many low-cost development boards (e.g., XIAO ESP32C3, SuperMini) do not include external USB-UART chips (CH340/CP2102) to save costs. Their USB ports connect directly to the chip's built-in USB Serial/JTAG (GPIO18/19). If UART output were used, these boards would have no serial output at all.
TinyGo encountered the same issue in Issue #3631 and resolved it via PR #4011 by changing the default serial from UART to USB Serial/JTAG:
Source: TinyGo commit d7c77b67
Key Features
_write()system call\nautomatically converted to\r\nImplementation Sources
This implementation is based on official code from ESP-IDF v6.0, streamlined and adapted:
1. USB Serial JTAG Register Structure
Source:
components/soc/esp32c3/register/soc/usb_serial_jtag_struct.hDefines the register layout for USB Serial JTAG peripheral, including:
ep1- TX/RX FIFO data registerep1_conf- FIFO status and control registerMapped to hardware address
0x60043000.2. HAL Layer Low-Level Functions
Source:
hal/esp32c3/include/hal/usb_serial_jtag_ll.hImplements three key inline functions:
usb_serial_jtag_ll_txfifo_flush()- Flush TX FIFOusb_serial_jtag_ll_txfifo_writable()- Check if TX FIFO has spaceusb_serial_jtag_ll_write_txfifo()- Write data to TX FIFO3. System Call Implementation
Source: Referenced from
esp_driver_usb_serial_jtag/src/usb_serial_jtag_vfs.cImplements
_write()system call to redirect standard output to USB Serial JTAG. Main features:Code Changes
1. Memory Layout (
targets/esp32c3.memory.ld)Added USB Serial JTAG peripheral base address:
2. Device Implementation (
targets/device/esp/esp32c3.c)Complete USB Serial JTAG driver implementation (~400 lines), including:
_write()system call (referenced from ESP-IDF VFS layer)Dependencies
Required: goplus/newlib#9
Newlib PR #9 declares
_write()as a weak symbol, allowing us to provide a custom implementation:During linking:
Usage Example
Build and run (automatic flash + monitor):
llgo run -a -target esp32c3 .Expected output (displayed directly via USB cable):
Implementation Notes
Why Not Use ESP-IDF Directly?
Our Approach
✅ Extract Core: Only extract register definitions and key functions
✅ Simplified Implementation: Remove VFS layer, interrupts, DMA, and other complex features
✅ Maintain Compatibility: Register operations fully compatible with ESP-IDF
✅ Document Sources: Code clearly annotated with sources for easy maintenance and updates
Related Links
ESP-IDF Official Documentation
ESP-IDF Source Code References
Dependency PR
_write()Commit History
This PR includes the following commits:
Fix Type: Feature
Priority: Medium (improves ESP32-C3 development experience)
Backward Compatible: Yes (does not affect existing code)
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com