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.

Sign up for GitHub

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

Jump to bottom

Fix bare metal support on Cypress targets #13425

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Fix bare metal support on Cypress targets #13425

merged 1 commit into from
Aug 21, 2020

Conversation

LDong-Arm
Copy link
Contributor

Summary of changes

Fixes: #13108

Cypress support (targets/TARGET_Cypress/TARGET_PSOC6) uses the macro CY_RTOS_AWARE to determine RTOS availability. Setting it globally in targets.json doesn't taken into account the bare metal use case and leads to build failures.

This PR makes the macro dependent on MBED_CONF_RTOS_PRESENT. Now mbed-os-example-blinky-baremetal compiles on Cypress targets.

Impact of changes

The bare metal profile now works on Cypress targets.

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

If bare metal tests are disabled for Cypress targets in CI, please re-enable them.

Reviewers

@MarceloSalazar @ARMmbed/mbed-os-core

This was referenced Aug 13, 2020
Closed
Closed
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 13, 2020
@ciarmcom ciarmcom requested review from maclobdell, MarceloSalazar and a team August 13, 2020 10:30
@ciarmcom
Copy link
Member

@LDong-Arm, thank you for your changes.
@MarceloSalazar @maclobdell @ARMmbed/mbed-os-maintainers please review.

@MarceloSalazar MarceloSalazar requested a review from a team August 13, 2020 10:31
@vmedcy
Copy link
Contributor

vmedcy commented Aug 13, 2020

Hello, SDIO_HOST.c and cyhal_system.c are not Mbed-specific:

https://github.com/cypresssemiconductorco/psoc6hal/blob/release-v1.3.0/COMPONENT_PSOC6HAL/source/cyhal_system.c

https://github.com/cypresssemiconductorco/udb-sdio-whd/blob/release-v1.0.1/SDIO_HOST.c

It is not good to use MBED_CONF_RTOS_PRESENT in these sources.

@LDong-Arm
Copy link
Contributor Author

LDong-Arm commented Aug 13, 2020 •
edited
Loading

@vmedcy

Do you know any header files (e.g. config files) in TARGET_Cypress that belong to Mbed but get included by those? If one exists, we can move the check there.

Another approach is to define the macro in https://github.com/ARMmbed/mbed-os/blob/master/rtos/source/TARGET_CORTEX/mbed_lib.json using target_overrides, though we want to minimised target-specific configurations. We do override things for a few targets anyway.

Any thoughts?

@vmedcy
Copy link
Contributor

vmedcy commented Aug 13, 2020

I analyzed the include hierarchies and concluded there is no Mbed-specific header included by psoc6hal or udb-sdio-whd - this is on purpose. In TARGET_PSOC6 directory, we have standard base headers like cmsis.h and device.h, but those are included only by Mbed-specific HAL layer, hence not useful in this situation.

Is it possible to override this in mbed_lib.json for base TARGET_PSOC6, instead of listing all PSoC 6 boards?

@LDong-Arm
Copy link
Contributor Author

I analyzed the include hierarchies and concluded there is no Mbed-specific header included by psoc6hal or udb-sdio-whd - this is on purpose. In TARGET_PSOC6 directory, we have standard base headers like cmsis.h and device.h, but those are included only by Mbed-specific HAL layer, hence not useful in this situation.

Thanks for confirming this.

Is it possible to override this in mbed_lib.json for base TARGET_PSOC6, instead of listing all PSoC 6 boards?

Yes.

LDong-Arm
LDong-Arm commented Aug 13, 2020
View reviewed changes
rtos/source/TARGET_CORTEX/mbed_lib.json
@@ -81,6 +81,9 @@
},
"NUVOTON": {
"idle-thread-stack-size-debug-extra": 512
},
"MCU_PSOC6_M4": {
"target.macros_add": ["CY_RTOS_AWARE"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vmedcy This is the idea. Though it looks slightly less than ideal, since the macro itself is not generic Mbed. But I can't think of another way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@LDong-Arm - agree, this is the good way to achieve the desired level of configurability. I don't know if generic FW code provided by another vendors has similar macro switch to enable/disable RTOS awareness without depending on any specific RTOS.

Copy link
Contributor Author

@LDong-Arm LDong-Arm Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if generic FW code provided by another vendors has similar macro switch to enable/disable RTOS awareness without depending on any specific RTOS.

Not I'm aware of. But I guess some vendors may modify a few files when importing to Mbed OS, which probably means less struggle with the build configs but more work when updating to a newer version of the upstream code. I'm not exactly familiar with it.

@mergify mergify bot added needs: CI and removed needs: review labels Aug 14, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 14, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Aug 14, 2020

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_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 14, 2020

It's waiting for @ARMmbed/team-cypress approval

@LDong-Arm
Copy link
Contributor Author

If it gets approved, shall we also add baremetal tests for a few Cypress targets in CI? http://mbed-os-ci.s3-website-eu-west-1.amazonaws.com/?prefix=jenkins-ci/ARMmbed/mbed-os/mbed-os-ci/PR-13425/artifacts/13425/1/build-GCC_ARM/PASS/

@0xc0170 0xc0170 merged commit a775c7d into ARMmbed:master Aug 21, 2020
@mergify mergify bot removed the ready for merge label Aug 21, 2020
@LDong-Arm LDong-Arm deleted the cypress_rtos_check branch August 21, 2020 12:56
@mbedmain mbedmain added release-version: 6.3.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@vmedcy vmedcy vmedcy approved these changes

@maclobdell maclobdell Awaiting requested review from maclobdell

@MarceloSalazar MarceloSalazar Awaiting requested review from MarceloSalazar

Assignees
No one assigned
Labels
release-version: 6.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cypress targets fail to compile blinky-baremetal in Mbed OS 6
6 participants
@LDong-Arm @ciarmcom @vmedcy @0xc0170 @mbed-ci @mbedmain