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

cdc: Fix autoflush for FIFO < MPS #1487

Merged
merged 1 commit into from Dec 6, 2022

Conversation

tore-espressif
Copy link
Contributor

@tore-espressif tore-espressif commented Jun 2, 2022

Describe the PR
If CDC TX buffer size is configured to be less than bulk packet size, the autoflushing condition is never reached.

Changes:

  1. TX buff is automatically flushed when TX FIFO is full

Additional context
Found out during espressif/esp-idf#9040

@tore-espressif
Copy link
Contributor Author

tore-espressif commented Jun 2, 2022

This is in great extent reverting #547

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

@perigoso perigoso left a comment

Choose a reason for hiding this comment

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

LGTM

@tore-espressif
Copy link
Contributor Author

tore-espressif commented Oct 18, 2022

@PanRe @perigoso @hathach any update about this?

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.

sorry for late response, I was too busy with other works. The reason for change request: although it has more code, the additional condition CFG_TUD_CDC_TX_BUFSIZE < BULK_PACKET_SIZE is constant expression and will help the compiler to optimize it out.

Therefore we don't have to run tu_fifo_full() when bufsize is larger than the packet size (and const expression also not executed at all in other case)

src/class/cdc/cdc_device.c Outdated 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.

thank for the update

@hathach hathach merged commit d9817eb into hathach:master Dec 6, 2022
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

3 participants