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

Make gpio irq api portable #15190

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Conversation

hazzlim
Copy link
Contributor

@hazzlim hazzlim commented Dec 9, 2021

Summary of changes

GPIO: Use uintptr_t for gpio_irq_api context

The HAL gpio_irq_api stores object IDs, which serve as a form of context
for the dispatch of the interrupt handler in the drivers level
InterruptIn Class. The way this is achieved is that the InterruptIn
Class casts its address to uint32_t, which is stored as the ID.
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 InterruptIn 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 PR makes similar changes to that of #15077 for the purpose of building MbedOS drivers components for unit testing.

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 `tests-mbed_hal_fpga_ci_test_shield-gpio_irq` builds successfully for changed targets.

Reviewers

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

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Dec 10, 2021
@ciarmcom ciarmcom requested a review from a team December 10, 2021 00:00
@ciarmcom
Copy link
Member

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

Copy link
Collaborator

@jeromecoutant jeromecoutant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non regression tests OK with all STM32 families

0xc0170
0xc0170 previously approved these changes Dec 16, 2021
@mergify mergify bot added needs: CI and removed needs: review labels Dec 16, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 16, 2021

Ci started

@hazzlim
Copy link
Contributor Author

hazzlim commented Dec 16, 2021

Ci started

Maybe worth holding off merging before review from @ARMmbed/mbed-os-hal (?)

@mbed-ci
Copy link

mbed-ci commented Dec 16, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 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_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_tfm-integration ✔️
jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added the needs: work label Dec 16, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 16, 2021

I just wanted to give it a first go with CI if it finds anything. It failed (one target gpio test timeout)

@mergify mergify bot dismissed 0xc0170’s stale review January 26, 2022 11:16

Pull request has been modified.

@hazzlim
Copy link
Contributor Author

hazzlim commented Jan 26, 2022

I just wanted to give it a first go with CI if it finds anything. It failed (one target gpio test timeout)

I've re-run the failed test and it passed, looks like it was an issue with connecting to the board rather than with the changes.

Oddly though, looks like on the original CI run the test in question (mbed_hal_fpga_ci_test_shield-gpio_irq) is skipped for all other targets, which are definitely affected by the changes.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 26, 2022

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jan 26, 2022

Jenkins CI Test : ❌ FAILED

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

@hazzlim
Copy link
Contributor Author

hazzlim commented Jan 26, 2022

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

Sorry, there's new targets added since I originally opened this which need updating as well. Will update.

The HAL gpio_irq_api stores object IDs, which serve as a form of context
for the dispatch of the interrupt handler in the drivers level
InterruptIn Class. The way this is achieved is that the InterruptIn
Class casts its address to uint32_t, which is stored as the ID.
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 InterruptIn 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.
@hazzlim
Copy link
Contributor Author

hazzlim commented Jan 26, 2022

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

Sorry, there's new targets added since I originally opened this which need updating as well. Will update.

Have updated with changes for new target (TOSHIBA TMPM4KN).

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 27, 2022

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 27, 2022

Jenkins CI Test : ✔️ SUCCESS

Build Number: 5 | 🔒 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_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
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-example-ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_tfm-integration ✔️

@0xc0170 0xc0170 merged commit 26876c0 into ARMmbed:master Jan 31, 2022
@mergify mergify bot removed the ready for merge label Jan 31, 2022
@mbedmain mbedmain added release-version: 6.16.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants