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

Improve CDC read code #1953

Closed
wants to merge 1 commit into from
Closed

Improve CDC read code #1953

wants to merge 1 commit into from

Conversation

pbrook
Copy link
Contributor

@pbrook pbrook commented Mar 22, 2014

Remove superfluous ring buffer from the CDC read code. The USB hardware already buffers 128 bytes of data, so just read from that on demand.

The result is smaller, faster, uses substantially less ram, and does not do large data copies in an interrupt handler.

Read CDC data from USB FIFO on demand instead of in ISR.
Remove superfluous ring buffer.

Signed-off-by: Paul Brook <[email protected]>
@embmicro
Copy link
Contributor

It seems like your modified peek function should be
if (peek_buffer < 0)
peek_buffer = USB_Recv(CDC_RX);
return peek_buffer;
So that you don't read more than one byte if you call peek repeatedly.

@pbrook
Copy link
Contributor Author

pbrook commented Mar 24, 2014

You are right. Patch fixed and branch updated.

ring_buffer *buffer = &cdc_rx_buffer;
return (unsigned int)(SERIAL_BUFFER_SIZE + buffer->head - buffer->tail) % SERIAL_BUFFER_SIZE;
if (peek_buffer >= 0) {
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

I guess that the
return 1;
should be:
return 1 + USB_Available(CDC_RX);

To avoid situations where queued chars may seems to "disappear":

av1 = Serial.available(); // av1 may be > 1
Serial.peek();
av2 = Serial.available(); // after the peek av2 is 1

assert(av2>=av1); // Assertion fails if av1>1

@cmaglie
Copy link
Member

cmaglie commented May 30, 2014

Manually merged here:
ddbb6b3

added also the patch for available() method:
6914af0

Thank you guys!

@ffissore
Copy link
Contributor

This PR introduced a regression. I've opened issue #2123

@cmaglie
Copy link
Member

cmaglie commented Jun 13, 2014

Good catch, thanks @ffissore and thanks to @embmicro for the quick patch.
C

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.

4 participants