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

Call One time tu_fifo_write_n on cdcd_xfer_cb #713

Merged
merged 4 commits into from
Mar 12, 2021

Conversation

HiFiPhile
Copy link
Collaborator

Describe the PR
I thought it was included in old PRs but seems not...

In my CDC test with SAME70, throughput increased from 800KB/s to 27MB/s.

Signed-off-by: HiFiPhile <admin@hifiphile.com>
Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

thanks for the PR, yeah, this is one of the improvement I planed to do but totally forgot. However, the implementation is not actually quite right. The driver must invoke the wanted_cb() right at the time it push the wanted_char into the fifo. which allow the application callback that call read() and see the wanted_chr as the LAST character in the fifo.

For example, to catch the \n then call read() to get the whole line. Else application must skip dozen of characters. which can potentially has another '\n' ( received multi-line within a packet) and cause some confusion when the callback is invoked with more data than it could expect.

Solution: we should still keep the loop however instead of writing one byte one, we keep an count and write_n(count) when we see the wanted (or write all if found none).

Update: please skip this review request and follow the below review. I have an change of mind, thanks :)

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

After giving it more thought, I think it is not worth to have wanted as the last char in the fifo at the time of the callback. It doesn't occur often and also doesn't have much impact on existing usage (application may need a minor of adjustment). Let's do it this way, though we still need to check if there is still data in the fifo before invoking the wanted callback, else it could confuse the app.

src/class/cdc/cdc_device.c Show resolved Hide resolved
Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Perfert !! though I just realized the fifo_empty() did a quicker check than computing the actual count. and changes to both callback check to !fifo_empty().

@hathach hathach changed the base branch from master to edpt_ISO_xfer March 12, 2021 06:06
@hathach hathach changed the base branch from edpt_ISO_xfer to master March 12, 2021 06:06
@hathach hathach merged commit d9f0475 into hathach:master Mar 12, 2021
@HiFiPhile HiFiPhile deleted the cdc_read branch July 22, 2021 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants