Skip to content

Print not Aborting on Write() failure #3651

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

Closed
wants to merge 2 commits into from
Closed

Print not Aborting on Write() failure #3651

wants to merge 2 commits into from

Conversation

stickbreaker
Copy link
Contributor

The current 1.6.5 Print library report the total count of characters send during operations. The assumption is that this count is of contiguous characters. If you printed a buffer of 45 characters and the print command returned 44 as the number of characters sent, the assumption is that the last character was not sent. But, this is not a valid assumption. The two low level functions through which all of the helper print funtions work (size_t Print::write(const uint8_t *buffer, size_t size) and size_t Print::print(const __FlashStringHelper *ifsh)) do not exit when a write failure is detected. They just attempt to write the next character from the input buffer. This renders any positional transmit information useless.
I propose to change the behavior of these two routines. When a write error is encountered these function return with the count of sent characters. If the calling procedure wants to retry sending then ~buffer[sentCount] is the next character needing to be send.

see arduino/Arduino issue #3614

Chuck Todd

Print::write(const uint8_t *buffer, size_t size) and Print::print(const
__FlashStringHelper *ifsh) would continue calling write(char) after a
failed write(char) this behavior would render returned count unuseable
see arduino/Arduino issue #3614
@facchinm
Copy link
Member

Hi @stickbreaker , your proposal is interesting but the code won't work with cdc serials (like Leonardo/Micro) as write could return more than 1.

@stickbreaker
Copy link
Contributor Author

@facchinm , this fix is for the Print:: object, all of the higher level calls to Print::print(types) or Print::println(types) funnel thru the size_t Print::write(const uint8_t *buffer, size_t size)

What are cdc serials? reference please :) I'll check it out.

chuck

@matthijskooijman
Copy link
Collaborator

@stickbreaker, the CDC serial (virtual serial over USB) for AVR is here: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/CDC.cpp#L164-L191 (there is also one for SAM).

@facchinm, I'm not sure what you mean with write() returning more than one? AFAICS, write(char) should always return 0 or 1, and write(uint8_t*, size_t) should return 0 up to size. Both of the calls in this PR use the write(char) version. Note that it is not technically impossible for write(char) to return more than 1, but I'd consider this a bug (Ethernet currently does this, see #1699).

Note that this PR modifies the virtual write(uint8_t*, size_t) in Print, which is just the default implementation for subclasses that did not implement this virtual method themselves. This means that subclasses can still override this method and provide an implementation for which the assumptions about the return value do not hold. This would probably need to be checked against the known subclasses, and these assumptions should be written down somewhere (reference page for Print on arduino.cc seems to make sense).

You might want to modify your commit message to say "Closes #3614" in the last line, so the issue will be automatically closed when merging the PR.

@facchinm
Copy link
Member

Sorry @stickbreaker , your solution is ok, I was blinded by the difference between n += and n++ (trusting write() to return 1) and while searching for a counter-example I totally forgot the method signature 😄
Anyway, you missed a parentheses on if(write(*buffer++) n++;, so I'll merge after the fix

bad transcription
@stickbreaker
Copy link
Contributor Author

@facchinm , You mean every little punctuation must be Correct ! 😊
sorry about that, I thought I could copy 20 character without error, but I was wrong!

Chuck.
closes #3614

@facchinm
Copy link
Member

Pushed as single commit as 25d81c9

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.

5 participants