Add declaration to expose enable_* functions from SerialBase #14175

harmut01 · 2021-01-19T14:47:25Z

Summary of changes

UnBufferedSerial is missing a declaration to expose enable_input and enable_output, which are inherited from the private base class SerialBase. Add the using-declaration to the class definition to expose these functions.

Impact of changes

None

Migration actions required

None

Documentation

None

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@kjbracey-arm @ithinuel @hugueskamba

UnBufferedSerial is missing a declaration to expose enable_input and enable_output, which are inherited from the private base class Serial Base. Add the using-declaration to the class definition.

ciarmcom · 2021-01-19T15:00:30Z

@harmut01, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2021-01-20T08:09:28Z

This exposes the member functions for UnbufferedSerial, but does it actually override the virtual members from FileHandle as required?

If I was reading the Compiler Explorer output from @ithinuel's example (https://godbolt.org/z/aoE78x) correctly, it looked like the using wasn't sufficient.

ithinuel · 2021-01-20T08:40:39Z

@kjbracey-arm I did explore a bit more using armv7-a clang with -Os https://godbolt.org/z/T9EG83
It looks to me like it does what we want.

@pan- Is that a valid solution ?

kjbracey · 2021-01-20T08:59:12Z

I don't see how you conclude that from your example. You don't have an actual Child object, so you can't see a generated vtable. After adding one, I'm not convinced I'm reading it correctly...

I'd suggest actually running a real test rather than inspecting disassembly.

0xc0170 · 2021-01-20T09:03:51Z

@harmut01 have you run it on the target or can you? Is there a test case (if not we should add it) ?

harmut01 · 2021-01-20T11:16:28Z

Yes I have Martin, the build compiles for the targets listed in the issue, and I have also run the code Peter provided on a Nucleo board with the following results:

kjbracey · 2021-01-20T11:20:44Z

If that's the original code in the issue, it doesn't test the FileHandle virtual method though.

My later code using mbed_file_handle would exercise it, but you'd need to add something to see if the calls reached the right place. Check return codes, or actually test output being disabled.

pan- · 2021-01-20T11:28:04Z

I amended the Godbolt snippet to reflect more closely the problem: https://godbolt.org/z/r5vPe9 .

As we can see, if the using declaration is used, Parent::f will be called if the object is accessed through a Child pointer but Interface::f is called if the object is accessed through an Interface pointer.

If a proper override is provided, the function become consistent regardless of the pointer type accessing it.

I think this PR should provide proper overrides for functions marked as virtual in SerialBase or FileHandle and let using to members that are not virtual in any of these classes.

kjbracey · 2021-01-20T12:02:32Z

I think this PR should provide proper overrides for functions marked as virtual in SerialBase or FileHandle

I believe that only applies to these 2 functions.

It does appear then that there's no way to have Child's vtable point directly to Parent::f. You'd kind of want there to be a using fill-in there for that case, but we have to invent Child::f. Bit less efficient. Oh well.

drivers/source/UnbufferedSerial.cpp

drivers/include/drivers/UnbufferedSerial.h

Pull request has been modified.

drivers/include/drivers/UnbufferedSerial.h

kjbracey · 2021-01-21T13:51:28Z

To be more explicit - this is following the FileHandle API.

Either you document explicitly that UnbufferedSerial enable_input calls can never and will never fail as part of API, preventing us from changing that in future, or you restate the standard API - negative error codes on failure.

I see no hugely strong reason to lock this in to "can never fail" as part of the API, even if that's true of the current implementation.

(Is it even true? Or is it the case that SerialBase just fails to report failure?)

ithinuel · 2021-01-21T14:28:25Z

Or is it the case that SerialBase just fails to report failure?

I don't know if the introduction of that feature is related to the time I had to hack in the serial HAL that ability to disable individually rx and/or tx. So for what it's worth, I the time only 2 device where of interest to me and that operation couldn't fail.

Now, If we consider the likely possibility of convergence of USBCDC/USBSerial and SerialBase through mbed::Stream, what would we expect from enable_input/enable_output ?

to fail
to accept but ignore the request (i.e. still be able to send/receive)
to accept and ignore what ever is received over USB when input is disabled / discard write operation when output is disabled ?

It may feel like we're diverging from the point here but this is what needs to be considered now if we lock into will never fail.
On the other hand, we can keep this API as may fail and adjust the under lying implementation if ever need be.

Remember that compilers are really smart. If it sees that it always return 0, it'll eliminate unnecessary code on it's own.

kjbracey · 2021-01-21T14:40:40Z

mbed::Stream

Still think that should be deprecated, as per #5655. Guess it still hasn't happened yet. Did kill Serial at least.

Anyway, this is implementing the FileHandle API (as does Stream). The FileHandle API already permits failure, which is why the return code is there. Indeed, failure is the default implementation in FileHandle.

This API is slightly unusual though in that it was not intended to actually be a "mute" - the device wasn't /required/ to actually block all input/output, it was just for power saving purposes. Something a bit like the "deep sleep lock", which is what it originally was just an abstraction for. This isn't actually clear in the current Doxygen though. So actually looking for "success"/"failure" isn't really that useful anyway. It's not an API that does something for you, it's an API that tells the system what you're up to.

I can't honestly think of any case an application would want to look at the return code. It's really there just for consistency with other FileHandle API calls. So discussing it this much is probably pointless :)

If it sees that it always return 0, it'll eliminate unnecessary code on it's own.

It could only do that if the definition was in the header or if you had Link-Time Optimisation on. That would be an argument for moving the definition to the header.

harmut01 · 2021-01-21T16:01:07Z

This force push reverts the function header comment. We can leave the API as is for now and if the behavior is ever needed we will at least have the option to implement it.

0xc0170 · 2021-01-22T11:37:28Z

CI started

mbed-ci · 2021-01-22T13:28:33Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

Add declaration to expose enable_* functions from SerialBase

7b8ca37

UnBufferedSerial is missing a declaration to expose enable_input and enable_output, which are inherited from the private base class Serial Base. Add the using-declaration to the class definition.

harmut01 requested review from hugueskamba, ithinuel and kjbracey January 19, 2021 14:47

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jan 19, 2021

ciarmcom requested review from a team January 19, 2021 15:00

ciarmcom added the needs: review label Jan 19, 2021

ithinuel approved these changes Jan 19, 2021

View reviewed changes

Implement override of enable_* functions

0214a15

kjbracey reviewed Jan 21, 2021

View reviewed changes

drivers/source/UnbufferedSerial.cpp Outdated Show resolved Hide resolved

Remove lock from enable_* function overrides

619ca54

kjbracey previously approved these changes Jan 21, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jan 21, 2021

hugueskamba previously requested changes Jan 21, 2021

View reviewed changes

drivers/include/drivers/UnbufferedSerial.h Show resolved Hide resolved

drivers/include/drivers/UnbufferedSerial.h Show resolved Hide resolved

mergify bot added needs: work and removed needs: CI labels Jan 21, 2021

kjbracey reviewed Jan 21, 2021

View reviewed changes

drivers/include/drivers/UnbufferedSerial.h Show resolved Hide resolved

harmut01 force-pushed the serial_fix branch from 9b259af to 619ca54 Compare January 21, 2021 15:54

harmut01 requested review from hugueskamba and kjbracey January 21, 2021 15:54

hugueskamba approved these changes Jan 21, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jan 21, 2021

ithinuel approved these changes Jan 21, 2021

View reviewed changes

0xc0170 approved these changes Jan 22, 2021

View reviewed changes

kjbracey approved these changes Jan 22, 2021

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Jan 22, 2021

0xc0170 merged commit 1b7b620 into ARMmbed:master Jan 25, 2021

mergify bot removed the ready for merge label Jan 25, 2021

mbedmain added release-version: 6.7.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jan 25, 2021

kjbracey mentioned this pull request Apr 9, 2021

UnbufferedSerial: add is_tx_active() #14420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add declaration to expose enable_* functions from SerialBase #14175

Add declaration to expose enable_* functions from SerialBase #14175

harmut01 commented Jan 19, 2021 •

edited

Loading

ciarmcom commented Jan 19, 2021

kjbracey commented Jan 20, 2021

ithinuel commented Jan 20, 2021 •

edited

Loading

kjbracey commented Jan 20, 2021 •

edited

Loading

0xc0170 commented Jan 20, 2021

harmut01 commented Jan 20, 2021

kjbracey commented Jan 20, 2021

pan- commented Jan 20, 2021 •

edited

Loading

kjbracey commented Jan 20, 2021

kjbracey commented Jan 21, 2021

ithinuel commented Jan 21, 2021

kjbracey commented Jan 21, 2021

harmut01 commented Jan 21, 2021

0xc0170 commented Jan 22, 2021

mbed-ci commented Jan 22, 2021

Add declaration to expose enable_* functions from SerialBase #14175

Add declaration to expose enable_* functions from SerialBase #14175

Conversation

harmut01 commented Jan 19, 2021 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Jan 19, 2021

kjbracey commented Jan 20, 2021

ithinuel commented Jan 20, 2021 • edited Loading

kjbracey commented Jan 20, 2021 • edited Loading

0xc0170 commented Jan 20, 2021

harmut01 commented Jan 20, 2021

kjbracey commented Jan 20, 2021

pan- commented Jan 20, 2021 • edited Loading

kjbracey commented Jan 20, 2021

kjbracey commented Jan 21, 2021

ithinuel commented Jan 21, 2021

kjbracey commented Jan 21, 2021

harmut01 commented Jan 21, 2021

0xc0170 commented Jan 22, 2021

mbed-ci commented Jan 22, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

harmut01 commented Jan 19, 2021 •

edited

Loading

ithinuel commented Jan 20, 2021 •

edited

Loading

kjbracey commented Jan 20, 2021 •

edited

Loading

pan- commented Jan 20, 2021 •

edited

Loading