Implement polymorphism for CAN driver #14336

AGlass0fMilk · 2021-02-23T21:23:30Z

Summary of changes

This PR introduces experimental polymorphism for the CAN C++ API. This allows alternate implementations (eg: external CAN interfaces) that are compatible with the internal implementation.

See #13209 for the surrounding discussion.

Impact of changes

None, it only affects the user if they have the EXPERIMENTAL_API feature enabled.

Migration actions required

None

Documentation

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

@kjbracey-arm @pan- @donatieng

ciarmcom · 2021-02-23T21:30:19Z

@AGlass0fMilk, thank you for your changes.
@pan- @kjbracey-arm @donatieng @ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

AGlass0fMilk · 2021-02-25T17:50:56Z

@pan- A possible consequence of this change is that client code referring to types previously defined inside the CAN class may need to change to use the fully-qualified typename. eg: IrqType -> CAN::IrqType.

I experience some issues with code referring simply to IrqType. I'll have to test further.

ciarmcom · 2021-02-28T00:50:25Z

This pull request has automatically been marked as stale because it has had no recent activity. @donatieng, @kjbracey-arm, @pan-, @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions.

0xc0170 · 2021-03-02T12:03:01Z

@AGlass0fMilk Is this Major? I would expect feature update. This is also enabled only if experimental api flag is set.

pan-

Looks good, I would like to preserve behavior regards to enum in the original class.
Please review my solution.

pan- · 2021-03-02T12:40:05Z

drivers/include/drivers/interfaces/InterfaceCAN.h

+
+namespace interface {
+
+namespace can {


That won't play well with code using the enum values: CAN::Reset won't be found as a member for example. As a workaround, this can be wrapped inside a structure instead of a namespace and interface::Can can inherit from it . If polymorphism is disabled then mbed::Can inherits from it too.

struct can { enum Mode { // enum definition }; // Prevent slicing and user creation of base class. protected: can() = default; ~can() = default; can(const can&) = default; can& operator=(const can&) = default; };

ciarmcom · 2021-03-04T12:50:33Z

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

adbridge · 2021-03-10T12:20:55Z

@AGlass0fMilk this PR is awaiting your responses...

mbed-ci · 2021-03-10T20:11:42Z

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-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-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_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️

This commit changes the `interface::can` namespace to a `struct`. This allows the enum types to be inherited and prevents breaking old code relying on referencing eg: `CAN::RxIrq`. When enabled, the polymorphic CAN interface class inherits from this `interface::can` struct. If not enabled, the `mbed::CAN` class inherits from `interface::can` directly. Co-authored-by: Vincent Coubard <[email protected]>

This commit adds provisions to enable using interface::CAN on targets that don't have DEVICE_CAN set to 1 (ie: they don't normally have a CAN peripheral).

AGlass0fMilk · 2021-05-04T06:28:15Z

@pan- @adbridge I have rebased and updated this PR with suggested edits.

I have tested that this builds in several configurations:

Builds for NUCLEO_G474RE target (which has a native CAN peripheral) without FEATURE_EXPERIMENTAL_API enabled
Builds for NUCLEO_G474RE target with FEATURE_EXPERIMENTAL_API enabled
Builds for NRF5284_DK target (which does not have a native CAN peripheral) with FEATURE_EXPERIMENTAL_API enabled

You can review the code I built in each of these cases to verify this doesn't break any existing code using mbed::CAN in any case:

Find that demo code here:

https://github.com/AGlass0fMilk/polymorphic-can-demo

Let me know if this requires any other changes.

drivers/include/drivers/CAN.h

This commit moves the deletion of copy constructor and copy assignment operators to the `mbed::interface::can` class, where both `mbed::CAN` and `mbed::interface::CAN` inherit enum types from. This allows `NonCopyable` to be removed from the inheritance list.

Pull request has been modified.

AGlass0fMilk · 2021-05-08T01:20:36Z

@pan- Updated to remove the NonCopyable inheritance and moved the deletion of copy constructor and copy assignment operator to the mbed::interface::can base class, where the enum types are inherited.

I tested to make sure the deletion of the copy methods was working (like to double check things like this...):

AGlass0fMilk/polymorphic-can-demo@63cfee2

I added these lines to my compilation example and they did indeed cause the build to fail each time:

    // Uncomment to test copy constructor deletion inheritance
    //CAN can3(can2);

    // Uncomment to test copy assignment operator deletio inheritance
    CAN can4 = can2;

With the following error messages:

[ERROR] ./main.cpp: In function 'int main()':
./main.cpp:49:18: error: use of deleted function 'mbed::CAN::CAN(const mbed::CAN&)'
   49 |     CAN can3(can2);
      |                  ^
In file included from ./mbed-os/mbed.h:70,
                 from ./main.cpp:6:
./mbed-os/drivers/include/drivers/CAN.h:39:7: note: 'mbed::CAN::CAN(const mbed::CAN&)' is implicitly deleted because the default definition would be ill-formed:
   39 | class CAN
      |       ^~~
./mbed-os/drivers/include/drivers/CAN.h:39:7: error: use of deleted function 'mbed::interface::can::can(const mbed::interface::can&)'
In file included from ./mbed-os/drivers/include/drivers/CAN.h:24,
                 from ./mbed-os/mbed.h:70,
                 from ./main.cpp:6:
./mbed-os/drivers/include/drivers/interfaces/InterfaceCAN.h:152:5: note: declared here
  152 |     can(const can &) = delete;
      |     ^~~
In file included from ./mbed-os/mbed.h:70,
                 from ./main.cpp:6:
./mbed-os/drivers/include/drivers/CAN.h:39:7: error: use of deleted function 'rtos::Mutex::Mutex(const rtos::Mutex&)'
   39 | class CAN
      |       ^~~

[ERROR] ./main.cpp: In function 'int main()':
./main.cpp:52:16: error: use of deleted function 'mbed::CAN::CAN(const mbed::CAN&)'
   52 |     CAN can4 = can2;
      |                ^~~~
In file included from ./mbed-os/mbed.h:70,
                 from ./main.cpp:6:
./mbed-os/drivers/include/drivers/CAN.h:39:7: note: 'mbed::CAN::CAN(const mbed::CAN&)' is implicitly deleted because the default definition would be ill-formed:
   39 | class CAN
      |       ^~~
./mbed-os/drivers/include/drivers/CAN.h:39:7: error: use of deleted function 'mbed::interface::can::can(const mbed::interface::can&)'
In file included from ./mbed-os/drivers/include/drivers/CAN.h:24,
                 from ./mbed-os/mbed.h:70,
                 from ./main.cpp:6:
./mbed-os/drivers/include/drivers/interfaces/InterfaceCAN.h:152:5: note: declared here
  152 |     can(const can &) = delete;
      |     ^~~
In file included from ./mbed-os/mbed.h:70,
                 from ./main.cpp:6:
./mbed-os/drivers/include/drivers/CAN.h:39:7: error: use of deleted function 'rtos::Mutex::Mutex(const rtos::Mutex&)'
   39 | class CAN
      |       ^~~

I also tested to make sure the compilations were successful on both NUCLEO_G474RE (with and without FEATURE_EXPERIMENTAL_API) and nRF52840_DK.

0xc0170 · 2021-05-10T15:13:44Z

Set to feature release, similar as others (new functionality, nothing breaking or?).

mbed-ci · 2021-05-10T16:03:38Z

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
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-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

0xc0170 · 2021-05-11T09:12:53Z

I'll merge this today if no objections

AGlass0fMilk · 2021-05-11T21:40:20Z

Set to feature release, similar as others (new functionality, nothing breaking or?).

I've tested this with existing code that takes the using namespace mbed declaration for granted and it didn't cause any issues. It shouldn't break anything already written for CAN, just introduces new polymorphism hierarchy when FEATURE_EXPERIMENAL_API is enabled.

ciarmcom added the release-type: major label Feb 23, 2021

ciarmcom requested review from donatieng, kjbracey, pan- and a team February 23, 2021 21:30

ciarmcom added needs: review component: hal component: core labels Feb 23, 2021

ciarmcom added the stale Stale Pull Request label Feb 28, 2021

pan- previously approved these changes Mar 2, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Mar 2, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Mar 2, 2021

mergify bot added ready for merge and removed needs: CI labels Mar 10, 2021

0xc0170 added needs: review and removed ready for merge stale Stale Pull Request labels Mar 15, 2021

AGlass0fMilk and others added 2 commits May 4, 2021 01:11

Implement polymorphism for CAN

be07771

Make interface::CAN buildable on targets without DEVICE_CAN

49e58dd

This commit adds provisions to enable using interface::CAN on targets that don't have DEVICE_CAN set to 1 (ie: they don't normally have a CAN peripheral).

mergify bot added needs: work and removed needs: review labels May 4, 2021

0xc0170 added needs: review and removed needs: work labels May 4, 2021

Fix astyle CI failures

d79446c

pan- previously approved these changes May 6, 2021

View reviewed changes

drivers/include/drivers/CAN.h Outdated Show resolved Hide resolved

mergify bot added needs: CI and removed needs: review labels May 6, 2021

pan- approved these changes May 10, 2021

View reviewed changes

0xc0170 added release-type: feature and removed release-type: major labels May 10, 2021

ciarmcom added the stale Stale Pull Request label May 10, 2021

mergify bot added ready for merge and removed needs: CI labels May 10, 2021

0xc0170 removed the stale Stale Pull Request label May 11, 2021

0xc0170 merged commit c02e101 into ARMmbed:master May 12, 2021

mergify bot removed the ready for merge label May 12, 2021

mbedmain added release-version: 6.11.0 Release-pending labels May 17, 2021

mbedmain removed release-type: feature Release-pending labels May 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement polymorphism for CAN driver #14336

Implement polymorphism for CAN driver #14336

AGlass0fMilk commented Feb 23, 2021 •

edited by 0xc0170

Loading

ciarmcom commented Feb 23, 2021

AGlass0fMilk commented Feb 25, 2021

ciarmcom commented Feb 28, 2021

0xc0170 commented Mar 2, 2021

pan- left a comment

pan- Mar 2, 2021

ciarmcom commented Mar 4, 2021

adbridge commented Mar 10, 2021

mbed-ci commented Mar 10, 2021

AGlass0fMilk commented May 4, 2021

AGlass0fMilk commented May 8, 2021

0xc0170 commented May 10, 2021

mbed-ci commented May 10, 2021

0xc0170 commented May 11, 2021

AGlass0fMilk commented May 11, 2021

Implement polymorphism for CAN driver #14336

Implement polymorphism for CAN driver #14336

Conversation

AGlass0fMilk commented Feb 23, 2021 • edited by 0xc0170 Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Feb 23, 2021

AGlass0fMilk commented Feb 25, 2021

ciarmcom commented Feb 28, 2021

0xc0170 commented Mar 2, 2021

pan- left a comment

Choose a reason for hiding this comment

pan- Mar 2, 2021

Choose a reason for hiding this comment

ciarmcom commented Mar 4, 2021

adbridge commented Mar 10, 2021

mbed-ci commented Mar 10, 2021

Jenkins CI Test : ✔️ SUCCESS

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

AGlass0fMilk commented May 4, 2021

AGlass0fMilk commented May 8, 2021

0xc0170 commented May 10, 2021

mbed-ci commented May 10, 2021

Jenkins CI Test : ✔️ SUCCESS

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

0xc0170 commented May 11, 2021

AGlass0fMilk commented May 11, 2021

AGlass0fMilk commented Feb 23, 2021 •

edited by 0xc0170

Loading