-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
usbd: device_next: cdc: wait IN EP ready before enqueue data to EP #79104
Open
zeonchew
wants to merge
1
commit into
zephyrproject-rtos:main
Choose a base branch
from
AmbiqMicro:usbd_acm_out_ep_busy_fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+26
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why new variable instead of just using
data->tx_fifo.altered
like it was earlier?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.
Hi @tmon-nordic,
I was just about to add my report on what I have done here.
Since that #75439 has been merged into zephyr main, I have rebased my works and retested with the changes of #75439 applied. However, I am still able to reproduce this issue.
I have also run more stress test on my fixed ported over to the latest main, and found out that raising
data->tx_fifo.altered
wouldn't help in retrigger the TX transaction, astx_fifo.altered
is cleared incdc_acm_irq_cb_handler()
.zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c
Lines 836 to 837 in a7aac8d
The previous testing wasn't rigorous enough, that the subsequent transaction was actually triggered by new data coming in from RX instead (and hence the stale data was flushed out together).
This being said, I have added another handling here to make sure that remaining data in
tx_fifo
, which the BULK IN endpoint is unable to send out in current transaction, gets sent out immediately after current TX is completed as well.zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c
Lines 571 to 580 in a7aac8d
Please help to review whether this make sense.
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 don't understand
cdc_acm_irq_cb_handler()
. Isn't thedata->tx_fifo.altered = false;
going to make theif (data->tx_fifo.altered)
effectively a dead code? Shouldn't this be fixed instead of adding new boolean?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 took me a while to understand this as well, from my understanding, in this handler, it will first clear all the altered flag, and then callback to the interrupt handler at application side:
data->cb(...)
.zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c
Lines 841 to 844 in a7aac8d
If there is more data to be transmitted by the application, the
tx_fifo.altered
flag will be raised by application before the callback returns. Then here we go, continue to send data out again.I see that this part is checked in by @jfischer-no. @jfischer-no, can you please help to confirm whether this understanding is correct?
Thanks.
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.
Isn't there a problem when there is more data in the tx ringbuffer? The sequence I think about is:
cdc_acm_irq_cb_handler()
setsdata->tx_fifo.altered = false;
data->cb(usbd_class_get_private(c_data), data->cb_data);
is called, application writes to ring buffer more bytes than a CDC ACM TX buffer can fitdata->tx_fifo.altered
was set to true alongside ring buffer writecdc_acm_tx_fifo_handler()
callslen = ring_buf_get(data->tx_fifo.rb, buf->data, buf->size);
and submits the transfer. The ring buffer still has pending TX data.usbd_cdc_acm_request()
submitscdc_acm_work_submit(&data->irq_cb_work);
cdc_acm_irq_cb_handler()
setsdata->tx_fifo.altered = false;
data->cb(usbd_class_get_private(c_data), data->cb_data);
is called, application doesn't have more data to writeIs there anything preventing the above scenario?
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.
Hi @tmon-nordic,
I added this for your case you mentioned in the latest commit just now☺️
zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c
Lines 571 to 580 in a7aac8d
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.
Why to have two booleans for roughly the same? Wouldn't it be better to change the
data->tx_fifo.altered
logic completely? If we go with separate variable, I thinkbool tx_deferred;
should be moved insidetx_fifo
structure.But is it really necessary to have
tx_deferred
? I think that theCDC_ACM_TX_EP_BUSY
in combination withring_buf_is_empty(data->tx_fifo.rb))
should be enough to determine whether the TX work has to be raised or not.My idea would be to replace
if (data->tx_fifo.altered)
withif (!ring_buf_is_empty(data->tx_fifo.rb)) && !atomic_test_and_set_bit(&data->state, CDC_ACM_TX_EP_BUSY))
. Then theCDC_ACM_TX_EP_BUSY
would be cleared inusbd_cdc_acm_request()
(and inside the TX handler if it fails to enqueue the buffer) when the IN request completes. This would essentially eliminate the need fordata->tx_fifo.altered
altogether and potentially make the code simpler to follow and more robust. Am I missing something?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.
Open Source development works on technical merit. I would say that there is no need to consult the original author when proposing a change that solves issues or otherwise enhances the code base.
While there may be times where you want to ask what was intended, there is also the possibility that the original author simply is no longer around.
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.
sorry, was going to replace my comment with another one just now but somehow I lost all of them.
(The comment on saying I am new to Open-Source and ZephyrOS, asking on how this community works, and whether it is okay to make such a change without consulting original author is correct way to do things.)
I am kind of agree that in this case, checking for
ring_buf_is_empty()
atcdc_acm_irq_cb_handler()
plus checking forCDC_ACM_TX_EP_BUSY
flag atcdc_acm_tx_fifo_handler()
would be optimal as checking the ring buffer status directly gives us the most accurate status for actions to be taken.It seems that the original intend of
altered
flag is to provide a fast access to the status whether application layer has accessed the ring buffer. However, it seems to me that the bit might not be good enough for the scenario in tx_fifo here anymore.@jfischer-no , what do you think? Is @tmon-nordic suggestion here to check whether to trigger
tx_fifo_work
based on whetherring_buf_is_empty()
sounds good to you as well?