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

min on cdc r/w #2219

Merged
merged 2 commits into from
Aug 14, 2023
Merged

min on cdc r/w #2219

merged 2 commits into from
Aug 14, 2023

Conversation

tswan-quasi
Copy link
Contributor

@tswan-quasi tswan-quasi commented Aug 14, 2023

Describe the PR
Adds a MIN() check for the bufsize on cdc reads and writes. This is to avoid a loop where the bufsize passed to tud_cdc_write() or tud_cdc_read() is 65536 (or a multiple) causing nothing to be written/read.

Additional context
Casting values to uint16_t greater than UINT16_MAX just truncates the upper bits, which means if bufsize ends up being 65536 (or a multiple) it results in the uint16_t cast giving a value of 0, causing no read/write to occur. This then ultimately returns 0 from the top level function, and if the top level application is relying on the return value of something such as tud_cdc_write(), then the program will get stuck sending nothing.

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.

look good, would you mind changing TU_MIN() to tu_min16() i.e

  uint32_t num_read = tu_fifo_read_n(&p_cdc->rx_ff, buffer, tu_min16(bufsize, UINT16_MAX));

@tswan-quasi
Copy link
Contributor Author

look good, would you mind changing TU_MIN() to tu_min16() i.e

  uint32_t num_read = tu_fifo_read_n(&p_cdc->rx_ff, buffer, tu_min16(bufsize, UINT16_MAX));

I think that would result in the same issue right? Since arg is type uint16_t would just cast bufsize and do the same thing

@hathach
Copy link
Owner

hathach commented Aug 14, 2023

I think that would result in the same issue right? Since arg is type uint16_t would just cast bufsize and do the same thing

Ah you are right, my bad :)

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 you, we can use tu_min32, but i guess it is not really matter in this case.

@hathach hathach merged commit ba40d66 into hathach:master Aug 14, 2023
40 checks passed
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.

2 participants