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

CMake: Add support for Cypress targets #13934

Merged
merged 12 commits into from
Dec 7, 2020

Conversation

hugueskamba
Copy link
Collaborator

@hugueskamba hugueskamba commented Nov 19, 2020

Summary of changes

Add CMakeLists.txt input source files to build Cypress Mbed targets with CMake.

Please note the following:
mbed-tools 3.5.0 always generates the macro from the attribute "target.macros_add": ["CY_RTOS_AWARE"] in cmsis/device/rtos/mbed_lib.json. This causes an issue as the presence of the macro CY_RTOS_AWARE causes the files in targets/TARGET_Cypress/TARGET_PSOC6/psoc6csp/abstraction/rtos/ to be required thus making it impossible to build with the baremetal profile (using the mbed-baremetal CMake target instead of mbed-os in the application CMakeLists.txt, see here).

mbedtools should not process mbed_lib.json files that are excluded when the requires keyword is used. This behaviour will match the old Mbed CLI and will cause the CY_RTOS_AWARE to not be generated as it lives in an mbed_lib.json file that is excluded when the baremetal profile is used.

The bug in mbed-tools has been reported here: ARMmbed/mbed-tools#127

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

Reviewers


@hugueskamba hugueskamba force-pushed the hk_cmake_add_cypress_targets branch 2 times, most recently from 975e16a to c134a28 Compare November 19, 2020 20:20
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Nov 19, 2020
@ciarmcom ciarmcom requested review from a team November 19, 2020 20:30
@ciarmcom
Copy link
Member

@hugueskamba, thank you for your changes.
@ARMmbed/team-cypress @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@hugueskamba hugueskamba force-pushed the hk_cmake_add_cypress_targets branch 2 times, most recently from 3310849 to d08e4dd Compare November 20, 2020 13:14
@hugueskamba hugueskamba changed the title CMake: Add support for CY8CKIT064B0S2_4343W target CMake: Add support for Cypress targets Nov 20, 2020
@LDong-Arm
Copy link
Contributor

LDong-Arm commented Nov 20, 2020

An approach would be to not always add the CY_RTOS_AWARE for the baremetal profile but I am not sure if a sensible and generic rule in mbedtools can be used to not include it if the baremetal profile has been selected.

Thoughts @Patater @rwalton-arm ?

@hugueskamba initially tried to define CY_RTOS_AWARE in https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Cypress/TARGET_PSOC6/psoc6csp/abstraction/rtos/mbed_lib.json (obvious not a bare metal library), so that a bare metal application won't have this macro defined. This is the behaviour of the old build tools (Mbed CLI).
But it looks like with the new mbed-tools the macro gets defined nonetheless for bare metal, which should not be the case for any libraries outside "require" in mbed_lib.json. @Patater @rwalton-arm Is this a known limitation of the new tools?

@Patater
Copy link
Contributor

Patater commented Nov 23, 2020

An approach would be for mbedtools to not always generate the CY_RTOS_AWARE macro. mbedtools should not process mbed_lib.json files that are excluded when the requires keyword is used.

This sounds like a bug with how the new tools handle the json config. Could you explain a bit more about how mbed_lib.json exclusion works? Particularly useful would be a description of how, and which, requires keywords lead to what sorts of exclusions. Thanks.

@LDong-Arm
Copy link
Contributor

This sounds like a bug with how the new tools handle the json config. Could you explain a bit more about how mbed_lib.json exclusion works? Particularly useful would be a description of how, and which, requires keywords lead to what sorts of exclusions. Thanks.

An example: mbed-os-example-blinky-baremetal contains "requires": ["bare-metal"], so when you compile it mbed_config.h won't define any macros from any libraries that are not part of bare metal (e.g. those related to WiFi).
But with mbed-tools, the generated .mbedbuild/mbed_config.cmake contains macros from all libraries, even ones that are not used.

@Patater
Copy link
Contributor

Patater commented Nov 25, 2020

This sounds like a bug with how the new tools handle the json config. Could you explain a bit more about how mbed_lib.json exclusion works? Particularly useful would be a description of how, and which, requires keywords lead to what sorts of exclusions. Thanks.

An example: mbed-os-example-blinky-baremetal contains "requires": ["bare-metal"], so when you compile it mbed_config.h won't define any macros from any libraries that are not part of bare metal (e.g. those related to WiFi).
But with mbed-tools, the generated .mbedbuild/mbed_config.cmake contains macros from all libraries, even ones that are not used.

Thanks for the explanation. Which data file dictates if a library is part of bare metal? Is it this one? ./TESTS/configs/baremetal.json

@LDong-Arm
Copy link
Contributor

Thanks for the explanation. Which data file dictates if a library is part of bare metal? Is it this one? ./TESTS/configs/baremetal.json

This one: https://github.com/ARMmbed/mbed-os/blob/mbed-os-6.5.0/platform/bare_metal/mbed_lib.json
Libraries listed there are always parts of bare metal. Other libraries such as those listed in ./TESTS/configs/baremetal.json (used for Greentea only) are not parts of bare metal, but can be enabled for bare metal by adding to "requires" of an application.

@Patater
Copy link
Contributor

Patater commented Nov 26, 2020

@LDong-Arm @hugueskamba Could either of you please ensure there is a bug raised on https://github.com/ARMmbed/mbed-tools/issues for this? Thanks.

@LDong-Arm
Copy link
Contributor

LDong-Arm commented Nov 26, 2020

@LDong-Arm @hugueskamba Could either of you please ensure there is a bug raised on https://github.com/ARMmbed/mbed-tools/issues for this? Thanks.

Created ARMmbed/mbed-tools#127

If we defined CY_RTOS_AWARE in mbed_lib.json, we hit the issue I created and it breaks CMake build. If we define it in CMake instead (this PR as of now), it breaks the old Mbed CLI. As I mentioned in the linked issue, "requires" is a legacy of the old build system that duplicates CMake's target_link_libraries().

@Patater
Copy link
Contributor

Patater commented Nov 26, 2020

Yes, "requires" is a legacy of the old build system that duplicates CMake's target_link_libraries(). Given we need to support both build systems for the time being, we'll have to consider when any json has "requires" in it and avoid emitting macros that aren't reachable from the libraries listed in "requires".

Thanks for finding and raising the bug.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 3, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 3, 2020

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

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

@mergify mergify bot added needs: work and removed needs: CI labels Dec 3, 2020
@mbed-ci
Copy link

mbed-ci commented Dec 3, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 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-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-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_cmake-example-test ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@0xc0170 0xc0170 merged commit 65281b9 into ARMmbed:master Dec 7, 2020
@mergify mergify bot removed the ready for merge label Dec 7, 2020
@hugueskamba hugueskamba deleted the hk_cmake_add_cypress_targets branch December 11, 2020 09:47
@mbedmain mbedmain added release-version: 6.6.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Dec 11, 2020
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