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

jtag serial drive write function loses characters when rx ring buffer… (IDFGH-10925) #12119

Closed
wants to merge 2 commits into from

Conversation

jjsch-dev
Copy link

upon examining the code, I noticed that the usbjtag_tx_char_via_driver function contains a comparison that checks whether an immediate send operation succeeds. However, the return value from usb_serial_jtag_write_bytes is always 1, which prevents the intended logic to try blocking for 50 mS, but also a return that always skyp the bloking try.

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2023

CLA assistant check
All committers have signed the CLA.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 22, 2023
@github-actions github-actions bot changed the title jtag serial drive write function loses characters when rx ring buffer… jtag serial drive write function loses characters when rx ring buffer… (IDFGH-10925) Aug 22, 2023
@ginkgm
Copy link
Collaborator

ginkgm commented Aug 23, 2023

Hi @jjsch-dev

usb_serial_jtag_write_bytes never returning 0 is a real issue. We will fix this.

The API will return 0 when write fails. Change this to -1 is a breaking change that we can't do.

From the VFS side, losing data when ringbuffer full is a expected behavior. We can't detect the connection to host, so we use the fifo overflow as indication.

@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development Status: Reviewing Issue is being reviewed and removed Status: Opened Issue is new Status: Selected for Development Issue is selected for development Status: Reviewing Issue is being reviewed labels Aug 23, 2023
@jjsch-dev
Copy link
Author

Hi @ginkgm, thanks for the explanation, if you want I'll close the PR.

@jjsch-dev
Copy link
Author

Hi @ginkgm, I modified the return of usb_serial_jtag_write_bytes to return 0 on failure and the size on success, and reverted the change to usbjtag_tx_char_via_driver.

@chipweinberger
Copy link
Contributor

@jjsch-dev thanks for creating the PR!

@jjsch-dev
Copy link
Author

@chipweinberger, thanks for the comment.

@mythbuster5
Copy link
Collaborator

@jjsch-dev Yes, this time changes is absolutely correct, I already have an internal PR to fix this, and that will be merged soon. Thanks for your contribution.

@jjsch-dev
Copy link
Author

@mythbuster5, thanks for the confirmation, I will wait the merge.

@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Selected for Development Issue is selected for development labels Aug 24, 2023
espressif-bot pushed a commit that referenced this pull request Sep 9, 2023
CommanderRedYT pushed a commit to CommanderRedYT/esp-idf that referenced this pull request Sep 24, 2023
espressif-bot pushed a commit that referenced this pull request Oct 10, 2023
espressif-bot pushed a commit that referenced this pull request Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants