Skip to content

Improved readString() for File #5445

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

Merged
merged 21 commits into from
Feb 4, 2019

Conversation

apicquot
Copy link
Contributor

@apicquot apicquot commented Dec 7, 2018

readString() method for the class File is inherited from the class Stream that requires a timeout of 1 second at the end of the file, hence a very poor performance. Stream implementation also uses multiple memory resizing that could be anticipated based on the size of the file.

The proposed implementation solves these two issues.

@apicquot apicquot changed the title Improved read string file Improved readString() for File Dec 7, 2018
@devyte devyte added this to the 2.5.0 milestone Jan 25, 2019
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

After discussion with @earlephilhower , this implementation is also suboptimal. Implementation should be in terms of the File implementation read(buf, size) method. To avoid large mem use, it should be chunked, e.g.: 128 or 256 bytes at a time, along the lines of how this write method is implemented:

@apicquot
Copy link
Contributor Author

may want to use a define rather than sizeof for the size of the block to read ?

@devyte
Copy link
Collaborator

devyte commented Jan 26, 2019

The function sizeof is compile-time code, so it gets optimized to a single value. Your use of sizeof is correct. The number 256 should be in a macro, though.

@devyte
Copy link
Collaborator

devyte commented Jan 30, 2019

ping!

@apicquot
Copy link
Contributor Author

I don't understand, the code works for me for files<256 or files>= 256: I read and push blocks while countRead>0.

The test countRead<sizeof(temp) is just to place a string terminator for the last block.

@devyte
Copy link
Collaborator

devyte commented Jan 30, 2019

Apologies, my previous review comment was messed up. It's corrected now.

@apicquot
Copy link
Contributor Author

You are absolutely right @devyte : String::concat(char*) needs a string terminator to work. I fixed it.

string::concat(char*, int length) may have been more elegant but it is a protected method

@devyte devyte merged commit e43a586 into esp8266:master Feb 4, 2019
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.

3 participants