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

[wiring] Make sure that `Serial` and `SerialX` methods are in sync with the documentation and don't return unexpected values #1782

Merged
merged 2 commits into from May 16, 2019

Conversation

@avtolstoy
Copy link
Member

commented May 16, 2019

Problem

#1737

Solution

Make sure that available(), availableForWrite(), peek() and read() don't forward the errors from HAL and only return 0 or -1 in case of errors appropriately.

Steps to Test

N/A

Example App

N/A

References


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

  • [bugfix][wiring] Make sure that Serial and SerialX methods are in sync with the documentation and don't return unexpected values #1782

@avtolstoy avtolstoy added this to the 1.2.0-rc.1 milestone May 16, 2019

@avtolstoy avtolstoy requested review from technobly and m-mcgowan May 16, 2019

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

I'm not sure about this - rather than breaking the . behaviour wouldn't;'t it be better to instead update the documentation? That way we make no breaking changes to existing apps.

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

We are not really breaking the behavior, but restoring it here at the very least for Gen 3? 😄 Currently Serial1.read() may return -260 on Gen 3 (SYSTEM_ERROR_NO_MEMORY, which is actually somewhat wrong), and I assume many apps simply check for != -1 instead of < 0 for error conditions.

We've discussed this previously and came to a conclusion that as a first step we'd ensure that no error codes are propagating up to the wiring caller and we are in conformance with the documentation and then later would change the behavior, documentation as well as notify about the breaking change in the release notes. I agree though that with 1.2.0 we may go straight to doing that. So, what do we do?

@@ -99,7 +98,7 @@ void USBSerial::blockOnOverrun(bool block)

int USBSerial::peek()
{
return HAL_USB_USART_Receive_Data(_serial, true);
return std::max(0, (int)HAL_USB_USART_Receive_Data(_serial, true));

This comment has been minimized.

Copy link
@sergeuz

sergeuz May 16, 2019

Member

This should be std::max(-1, ...)

avtolstoy and others added 2 commits May 16, 2019
[wiring] serialx: make sure that available(), availableForWrite(), pe…
…ek() and read() don't forward the errors from HAL and only return 0 or -1 in case of errors appropriately

@sergeuz sergeuz force-pushed the ch32372/wiring-serialx-return-vals branch from 8e6ec19 to 1ea10fb May 16, 2019

@sergeuz sergeuz merged commit 3b0f849 into develop May 16, 2019

1 check passed

continuous-integration/travis-ci/push The Travis CI build passed
Details

@sergeuz sergeuz deleted the ch32372/wiring-serialx-return-vals branch May 16, 2019

@technobly technobly removed request for technobly and m-mcgowan May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.