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

default implementation for buffered Stream.read only reads every second byte #8452

Closed
INemesisI opened this issue Jan 18, 2022 · 5 comments · Fixed by #8453
Closed

default implementation for buffered Stream.read only reads every second byte #8452

INemesisI opened this issue Jan 18, 2022 · 5 comments · Fixed by #8453
Assignees

Comments

@INemesisI
Copy link
Contributor

The current Stream::read (uint8_t* buffer, size_t maxLen) provides a standard implementation for writing a stream to a buffer.
It uses the virtual int method Stream::read() to read the stream byte by byte until no more bytes are available.

However, it calls read() twice in each loop. This results in only every 2nd byte actually being read into the destination buffer, as the read() method will always return the next byte in the stream.
At least that is my understanding of how the read() method of streams is supposed to work.

The lines of code involved:

int Stream::read (uint8_t* buffer, size_t maxLen)
{
IAMSLOW();
size_t nbread = 0;
while (nbread < maxLen && available())
{
int c = read();
if (c == -1)
break;
buffer[nbread++] = read();
}
return nbread;

@d-a-v d-a-v self-assigned this Jan 18, 2022
@d-a-v d-a-v added this to the 3.1 milestone Jan 18, 2022
@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2022

Nice catch !
We didn't see it because this function is often overridden in subclasses.
Would you like to submit a pull request for fixing this ?

@INemesisI
Copy link
Contributor Author

That was also my first guess, that it is probably overloaded everywhere, in favour of a faster version.
The PR is submitted!

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2022

Thanks !
You may edit your first post in #8453 and add a new line "fixes #8452"

@INemesisI
Copy link
Contributor Author

Like this?

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 18, 2022

Like this?

Yes. Doing this adds a reference to the issue (look to the right column on this page) and automatically closes the issue (this post) when the PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants