CAN: Use uintptr_t for can_irq_ids #15077

hazzlim · 2021-09-16T16:05:33Z

Summary of changes

CAN: Use uintptr_t for can_irq_ids

The HAL can_api stores an array of IDs in order to dispatch interrupts
to the correct CAN object. The drivers level CAN Class casts a pointer
to itself to an uint32_t, which is stored as the ID and then cast back
to a CAN * in order to call the correct handler. This results in
compilation failure when the size of an object pointer is greater than
uint32_t, for example when building on a PC for unit testing.

In order to allow Unit Testing of the CAN Class, we replace the use of
uint32_t with uintptr_t (type capable of holding a pointer), which
allows portability and expresses intentions more clearly. In aid of this
latter goal, we also replace the use of the name "id" with "context",
to improve clarity. These are addresses of the context related to that
callback.

This pull-request is a proposed solution to this problem, for discussion, which can then be applied to other Drivers APIs which exhibit similar barriers to compilation for unit testing (e.g. similar changes required for gpio_irq_api).

Impact of changes

Migration actions required

Documentation

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)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Tested by verifying that `mbed-os-snippet-CAN_ex_1` builds successfully for changed targets.

Reviewers

@ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal

ciarmcom · 2021-09-16T16:30:17Z

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

ithinuel

Out of curiosity what target are you using?
Most mbed os targets being cortex-m their pointer size if 32bits.

ithinuel · 2021-09-16T18:44:09Z

hal/include/hal/can_api.h

@@ -63,7 +63,7 @@ typedef struct {
    int td_function;
 } can_pinmap_t;

-typedef void (*can_irq_handler)(uint32_t id, CanIrqType type);
+typedef void (*can_irq_handler)(uintptr_t id, CanIrqType type);


This carries the additional meaning that the id is a pointer. size_t may be more appropriate (and passing in the index in the table rather than a pointer).

My understanding of how the drivers CAN Class and the HAL can_api interoperate is that the CAN constructor passes the address of itself cast to an integer type via can_irq_init, which becomes the id in the table. CAN has a static method _irq_handler which takes the id/address and then casts it back to a CAN * in order to call the correct callbacks.

As this _irq_handler method is what the can_irq_handler type is used to refer to in all the can_api implementations, I was intentionally using a pointer type because the id is indeed a pointer - it's pointers all the way down if you will. The id here is not an index into the table, it's the value from the table. But maybe I am peeling back the curtain too much here and size_t would still be more appropriate for the api?

I had a deeper look at a few hal impl and as far as I can see the ID is never used in the lowlevel driver, it is only passed back to the callbacks.

The fact that this ID is a pointer instead of an plain integer is a shortcut that the Driver takes in the definition/management of IDs. By changing the type of the HAL here we are bending the definition of ID from the HAL to accommodate the hacky thing the Driver does (casting its this to something not guaranteed to be able to hold its value).

Other HAL have similar needs and some made the choice to make it an intptr_t with the nuance that it's not called an ID but a context. (id also to some extend imply unicity which is not even required by the driver itself).

So if we intend to keep the pointer shortcut (which makes sense for efficiency), I recommand we use "context" rather than "id". This way we have a clear "pointer to the context related to that callback" rather than "and Id that happen to also be a pointer so we changed its type to make sure it cal hold that pointer's value".

This makes sense - I'll amend with this naming change, for greater clarity. Is there a preference between intptr_t and uintptr_t here?

No strong preference for me. If sign does not add value (e.g. as in the distance between two pointers) I'd probably use the unsigned version but if someone thinks otherwise I'm ok with either.

So if we intend to keep the pointer shortcut (which makes sense for efficiency), I recommand we use "context" rather than "id". This way we have a clear "pointer to the context related to that callback" rather than "and Id that happen to also be a pointer so we changed its type to make sure it cal hold that pointer's value".

Revisiting, this makes even more sense - code currently uses id to refer to both the context itself and the index into the array used to select the context. Now we have a clear distinction.

Rebased with naming changes :)

hazzlim · 2021-09-16T18:48:18Z

Out of curiosity what target are you using?
Most mbed os targets being cortex-m their pointer size if 32bits.

Sorry, I hoped it was clear - the change is in order to allow building of CAN API on host machines for Unit Testing, which is currently not possible!

The HAL can_api stores an array of IDs in order to dispatch interrupts to the correct CAN object. The drivers level CAN Class casts a pointer to itself to an uint32_t, which is stored as the ID and then cast back to a CAN * in order to call the correct handler. This results in compilation failure when the size of an object pointer is greater than uint32_t, for example when building on a PC for unit testing. In order to allow Unit Testing of the CAN Class, we replace the use of uint32_t with uintptr_t (type capable of holding a pointer), which allows portability and expresses intentions more clearly. In aid of this latter goal, we also replace the use of the name "id" with "context", to improve clarity. These are addresses of the context related to that callback.

ithinuel

LGTM

ciarmcom · 2021-09-22T15:50:23Z

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

0xc0170 · 2021-09-28T13:48:26Z

CI started

mbed-ci · 2021-09-28T15:13:19Z

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

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

ciarmcom requested a review from a team September 16, 2021 16:30

ciarmcom added needs: review devices: silicon labs devices: st devices: nuvoton devices: renesas component: hal devices: gigadevice devices: nxp component: core labels Sep 16, 2021

ithinuel reviewed Sep 16, 2021

View reviewed changes

LDong-Arm requested a review from a team September 17, 2021 10:14

hazzlim force-pushed the make_can_api_portable branch from 55ee54f to b493a15 Compare September 17, 2021 12:41

ithinuel approved these changes Sep 17, 2021

View reviewed changes

ciarmcom added the stale Stale Pull Request label Sep 22, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 24, 2021

0xc0170 approved these changes Sep 28, 2021

View reviewed changes

0xc0170 removed the stale Stale Pull Request label Sep 28, 2021

mergify bot added needs: CI and removed needs: review labels Sep 28, 2021

mergify bot added ready for merge and removed needs: CI labels Sep 28, 2021

0xc0170 merged commit 0fac696 into ARMmbed:master Sep 29, 2021

mergify bot removed the ready for merge label Sep 29, 2021

mbedmain added release-version: 6.15.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Nov 22, 2021

hazzlim mentioned this pull request Dec 9, 2021

Make gpio irq api portable #15190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAN: Use uintptr_t for can_irq_ids #15077

CAN: Use uintptr_t for can_irq_ids #15077

hazzlim commented Sep 16, 2021 •

edited

Loading

ciarmcom commented Sep 16, 2021

ithinuel left a comment

ithinuel Sep 16, 2021

hazzlim Sep 16, 2021 •

edited

Loading

ithinuel Sep 16, 2021 •

edited

Loading

hazzlim Sep 16, 2021

ithinuel Sep 16, 2021

hazzlim Sep 17, 2021

hazzlim Sep 17, 2021

hazzlim commented Sep 16, 2021

ithinuel left a comment

ciarmcom commented Sep 22, 2021

0xc0170 commented Sep 28, 2021

mbed-ci commented Sep 28, 2021

CAN: Use uintptr_t for can_irq_ids #15077

CAN: Use uintptr_t for can_irq_ids #15077

Conversation

hazzlim commented Sep 16, 2021 • edited Loading

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

@ARMmbed/mbed-os-core @ARMmbed/mbed-os-hal

ciarmcom commented Sep 16, 2021

ithinuel left a comment

Choose a reason for hiding this comment

ithinuel Sep 16, 2021

Choose a reason for hiding this comment

hazzlim Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

ithinuel Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

hazzlim Sep 16, 2021

Choose a reason for hiding this comment

ithinuel Sep 16, 2021

Choose a reason for hiding this comment

hazzlim Sep 17, 2021

Choose a reason for hiding this comment

hazzlim Sep 17, 2021

Choose a reason for hiding this comment

hazzlim commented Sep 16, 2021

ithinuel left a comment

Choose a reason for hiding this comment

ciarmcom commented Sep 22, 2021

0xc0170 commented Sep 28, 2021

mbed-ci commented Sep 28, 2021

Jenkins CI Test : ✔️ SUCCESS

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

hazzlim commented Sep 16, 2021 •

edited

Loading

hazzlim Sep 16, 2021 •

edited

Loading

ithinuel Sep 16, 2021 •

edited

Loading