Implement polymorphism for DigitalIn/Out/InOut

[AGlass0fMilk]

Summary of changes

In some IO-constrained applications, it is necessary to use a GPIO expander like the I2C-based MCP23008. To maintain compatibility with existing libraries that require a DigitalIn/Out/InOut handle, it would be beneficial to be able to subclass one of these classes and retarget the read/write, etc calls. This would enable an MCP23008 driver to provide DigitalOut-like objects that abstract away the intermediate I2C transfers. These objects can then be passed to older APIs using DigitalOut pointers.

For this to work properly, some of the DigitalIn/Out/InOut class methods must be made virtual. This PR accomplishes this.

I would encourage Mbed to consider "virtualizing" many of the other hardware drivers for similar use cases.

See #12387 for a similar discussion concerning retargetting Mbed's CAN API to an external SPI-based CAN controller/transceiver.

Impact of changes

None. This should not affect current users of these APIs in any functional way. Code size may be slightly increased due to the addition of virtual tables and new functions.

Migration actions required

None

Documentation

None

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] 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

---

@0xc0170 @ithinuel @bulislaw

[AGlass0fMilk]

See application for this PR here: https://github.com/AGlass0fMilk/ep-oc-mcu/blob/refine-io-drivers/devices/MCP23008/MCP23008.hpp

[ciarmcom]

@AGlass0fMilk, thank you for your changes.
@bulislaw @0xc0170 @ithinuel @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

This could have a noticeable performance impact for bitbangers. The DigitalIn/Out classes are set up as one of the very few HAL APIs that can be fully inlined all the way down to a register read/write, and polymorphism defeats that.

Although on the other hand, I guess in most cases people accessing DigitalIn/Out are doing it as direct objects, eg class members, rather than via a pointer or reference, so the polymorphism should be skipped in those cases?

I've been involved in quite a bit of "devirtualisation" work - trying to minimise the number of uses of virtual in Mbed OS to get code size down.

We haven't yet come up with a totally satisfactory pattern that gives you the best of both worlds - no polymorphism overhead when you have exactly one such type in a system, and pay for it only when you need it. The toolchains would ideally be able to figure that out themselves (LTO?)

Maybe you could have a config option for virtualised drivers - either global or per-type?

[AGlass0fMilk]

This could have a noticeable performance impact for bitbangers. The DigitalIn/Out classes are set up as one of the very few HAL APIs that can be fully inlined all the way down to a register read/write, and polymorphism defeats that.

Definitely don't want to mess with that.

Although on the other hand, I guess in most cases people accessing DigitalIn/Out are doing it as direct objects, eg class members, rather than via a pointer or reference, so the polymorphism should be skipped in those cases?

For software my company writes, I'm trying to make it standard practice to lean towards using references rather than direct objects. This allows drivers written in the past to accept abstracted objects in the future if ever necessary.

In my specific case we have an external SPI-based chip and the CS pin is tied to a GPIO on an MCP23008 I/O expander. The driver I originally wrote for the SPI-based chip sometimes does multiple accesses during one function call so controlling the CS pin external to the driver code isn't possible. The best solution for me was to create a subclass of DigitalOut and virtual-ize the functions in this PR.

This is one of the benefits of using C++ in embedded. While I accept the increase in latency from the vtable redirection may be unacceptable in some cases, the flexibility provided by polymorphism in C++ is why I like using Mbed in the first place. It's one of the only embedded platforms pushing modern C++ onto micros. Therefore I don't think polymorphism should be completely removed from Mbed's base drivers.

I've been involved in quite a bit of "devirtualisation" work - trying to minimise the number of uses of virtual in Mbed OS to get code size down.

We haven't yet come up with a totally satisfactory pattern that gives you the best of both worlds - no polymorphism overhead when you have exactly one such type in a system, and pay for it only when you need it. The toolchains would ideally be able to figure that out themselves (LTO?)

Maybe you could have a config option for virtualised drivers - either global or per-type?

I don't love the idea of yet another obscure configuration option but I don't see another path forward to satisfy both use cases simultaneously. Perhaps we can add a new macro to mbed_toolchain.h -- MBED_VIRTUAL or something and conditionally apply the virtual keyword based on the app configuration? What would be the default in this case?

EDIT: my suggestion above ignores the possibility of enabling/disabling virtual on a per-module basis. We could introduce a configuration option for each module that can be configured to have virtual members.

To avoid conflicting macro definitions we could:
1.) Use long, unique, ugly names on a per-file basis. eg: MBED_VIRTUAL_DIGITALIN
2.) Put a single macro definition for each module in mbed_toolchain.h. eg: MBED_VIRTUAL_IO
3.) Create a new header -- mbed_virtual.h -- and put these module-level definitions there.

[kjbracey]

I don't love the idea of yet another obscure configuration option but I don't see another path forward to satisfy both use cases simultaneously. Perhaps we can add a new macro to mbed_toolchain.h -- MBED_VIRTUAL or something and conditionally apply the virtual keyword based on the app configuration? What would be the default in this case?

Basically my thoughts too. It would be something a bit more specific like MBED_DRIVER_VIRTUAL. It shouldn't be too obscure, in that as long as you use override you would get a somewhat clear error if the config isn't set right - the derived class would be in an "#if` checking that it was okay.

I've been involved a bit too much in micro-optimisation recently, spending days to shave another few hundred bytes off an image, which is somewhat skewing my view :) And that makes me inclined to have per-class virtual options, rather than a single one.

There's another significant image size problem in that neither GCC nor ARMC6 can eliminate unused virtual functions (ARMC5 and IAR can). Which means all virtual functions get included into an image. Big effect.

[0Grit]

More and more my problems end up in the compiler bin...

[adbridge]

@AGlass0fMilk I've re-classified this as a feature update as you would be introducing new callable functions...

[mergify]

This PR cannot be merged due to conflicts. Please rebase to resolve them.

[0xc0170]

@donatieng @bulislaw What shall we do with this PR ? Please review

[hugueskamba]

@AGlass0fMilk Please rebase your branch if you still intend it to be merged.

[AGlass0fMilk]

@AGlass0fMilk Please rebase your branch if you still intend it to be merged.

I will rebase this week.

[ciarmcom]

This pull request has automatically been marked as stale because it has had no recent activity. @AGlass0fMilk, please carry out any necessary work to get the changes merged. Thank you for your contributions.

[AGlass0fMilk]

Hi @hugueskamba, I have rebased and implemented the discussed configuration options for "virtualizing" each class.

[AGlass0fMilk]

Another use case for this feature is linked to above:

EmbeddedPlanet/ep-oc-mcu#42

@0xc0170 I'd like to know your thoughts on that PR above... It's a simple problem with an infinite number of solutions...

[AGlass0fMilk]

@0xc0170 Please review

[AGlass0fMilk]

@0xc0170 @kjbracey-arm I have created a small blinky example showing a simple use case for this polymorphism:

https://github.com/EmbeddedPlanet/mbed-os-example-inverted-digitalout

It compiles and works as expected on an NRF52840_DK.

Let me know if there's anything else to get this pushed forward.

[0xc0170]

Reviewers, is this ready for integration ? The old approvals were cleared after the recent updates, is there anything outstanding?

@AGlass0fMilk Thanks for the example and doc PR, I'll check them

[AGlass0fMilk]

Please move this along! I put quite a bit of work into the documentation and PRs for this...

[0xc0170]

CI started

[0xc0170]

I'll restart CI, this should be ready once we release 6.9 today

[mbed-ci]

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 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_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-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	✔️

[AGlass0fMilk]

Let's get that merge!