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

Optimize Print::write(const char *str) #486

Closed
vshymanskyy opened this issue Apr 9, 2018 · 10 comments
Closed

Optimize Print::write(const char *str) #486

vshymanskyy opened this issue Apr 9, 2018 · 10 comments

Comments

@vshymanskyy
Copy link

size_t Print::write(const char *str) {

This should really call strlen() and then call Print::write(const void *buffer, uint32 size) implementation.
Why so? Because the write(buffer, size) is commonly overridden and implemented in an optimized manner (sending data to the wifi/gsm module in chunks, not byte by byte...).

@rogerclarkmelbourne
Copy link
Owner

rogerclarkmelbourne commented Apr 9, 2018

Virtually nothing is bounds tested.

Adding bounds testing to such a low level function will considerably slow down its operation and break people's code, who rely on fast transfers

Please do your bounds checking before calling write

@vshymanskyy
Copy link
Author

vshymanskyy commented Apr 10, 2018

Well, virtually every library out there expects Arduino Core to behave this way ;)
I am not asking for bounds checks, I'm asking to call a different overloaded/overridden function.
Check out this:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/Print.h#L52

@stevstrong
Copy link
Collaborator

I think that this would make sense, even if at the end some of the overloaded functions end up with the same single writes.
I would go further and move these functions in a header file as Arduino core does.

@victorpv
Copy link
Contributor

Not sure if the call to strlen would increase the size, but I agree with OP and Steve that is better to use Print::write(const void *buffer, uint32 size), should remove a lot of overhead when sending a few characters at once.
@Steve, do you have a link to the relevant files in the Arduino code to see how it's done there?

@rogerclarkmelbourne
Copy link
Owner

Ah,

I thought you were proposing bounds checking.. My mistake...

@stevstrong
Copy link
Collaborator

Victor, take a look at the very first link of OP.

rogerclarkmelbourne added a commit that referenced this issue Apr 12, 2018
@rogerclarkmelbourne
Copy link
Owner

Fix is in commit

f56c37e

I will close this.,

Re-open if there is a problem

@vshymanskyy
Copy link
Author

Thanks! BTW is it only related to F1 or tp other cores as well? Didn't check them

@rogerclarkmelbourne
Copy link
Owner

Good point, I should update the F4 and perhaps the F3

Note. The F3 is really not supported, there arent any F3 boards in the boards.txt, its just there as an archive, but I should probably remove it, because git will keep a record of it in the older commits

stevstrong added a commit to stevstrong/Arduino_STM32 that referenced this issue Apr 17, 2018
@rogerclarkmelbourne
Copy link
Owner

Fixed

minimum-necessary-change pushed a commit to minimum-necessary-change/Arduino_STM32 that referenced this issue Sep 16, 2019
minimum-necessary-change pushed a commit to minimum-necessary-change/Arduino_STM32 that referenced this issue Sep 16, 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

No branches or pull requests

4 participants