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

cpu/esp32/stdio_usb_serial_jtag: Fix to ESP32 stdio usb serial hanging if it receives data too quickly #20972

Conversation

IsikcanYilmaz
Copy link
Contributor

@IsikcanYilmaz IsikcanYilmaz commented Nov 11, 2024

Contribution description

When the stdio serial usb jtag module of the ESP32 receives data at (relatively) high speeds, there's a chance its tx fifo won't be flushed and/or the correct irq flag won't be cleared. This leads to nothing being printed through the usb stdio anymore, and (most instances) data being unable to be received by the device, rendering the shell unusable.

This PR clears the IRQ flags at the end of the ISR and flushes the fifo. With this change I have not been able to repro the bug in question.

Testing procedure

  • Use an ESP32 that has stdio_usb_serial_jtag support. (I've been using a seeedstudio xiao esp32s3)
  • Load up a basic example that uses the shell module, like examples/default
  • Wail on the shell using the following python code, or anything equivalent:
#!/usr/bin/env python3
import serial, time
ser = serial.Serial("/dev/tty.usbmodem111301")
count = 10
while(True):
    ser.write("help\n".encode())
    time.sleep(0.001)
    count -= 1
    if (count == 0):
        break
print("Writer done")
  • Observe that the shell is no longer responsive
  • This PR fixes this issue

Issues/PRs references

@github-actions github-actions bot added Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Nov 11, 2024
…flush the tx fifo at the end of usb serial isrs
@IsikcanYilmaz IsikcanYilmaz force-pushed the pr/esp32_usb_serial_jtag_isr_flag_reset branch from 9ac9cec to a21d302 Compare November 11, 2024 10:21
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 11, 2024
@benpicco benpicco requested a review from mguetschow November 11, 2024 13:46
@riot-ci
Copy link

riot-ci commented Nov 11, 2024

Murdock results

✔️ PASSED

a21d302 cpu/esp32/stdio_usb_serial_jtag: clear all usb serial jtag flags and flush the tx fifo at the end of usb serial isrs

Success Failures Total Runtime
10215 0 10215 19m:11s

Artifacts

@mguetschow mguetschow requested a review from maribu November 11, 2024 13:59
@mguetschow
Copy link
Contributor

Thanks @IsikcanYilmaz for your PR and welcome to the community! :)

Unfortunately, I don't have hardware at hand to test this, but I'm sure someone else will.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Thank you for the fix, I've stumbled upon this several times before!
Works like a charm now.

@benpicco benpicco enabled auto-merge November 11, 2024 22:46
@benpicco benpicco added this pull request to the merge queue Nov 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 12, 2024
@mguetschow mguetschow added this pull request to the merge queue Nov 12, 2024
Merged via the queue into RIOT-OS:master with commit ec9afd2 Nov 12, 2024
27 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants