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

Fix CY8CPROTO_062_4343W baremetal build #12438

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

hugueskamba
Copy link
Collaborator

@hugueskamba hugueskamba commented Feb 14, 2020

Summary of changes

Make a Mbed library with Cypress WHD files so it is automatically excluded when building with the bare metal profile. Create another Mbed library to group network files that use WHD so they can also be excluded from the bare metal profile.

Impact of changes

It is now possible to build the CY8CPROTO_062_4343W target with the baremetal profile.
i.e It is now possible to build mbed-os-example-blinky-baremetal with $ mbed compile -t <TOOLCHAIN> -m CY8CPROTO_062_4343W

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

@evedon @bulislaw


@hugueskamba hugueskamba requested a review from vmedcy February 14, 2020 15:49
@@ -23,7 +23,7 @@
* limitations under the License.
*******************************************************************************/

#if defined(CYBSP_WIFI_CAPABLE)
#if MBED_CONF_CY_PSCOC6_WHD_PRESENT
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@hugueskamba hugueskamba Feb 15, 2020

Choose a reason for hiding this comment

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

The problem is that both modules pull in WHD which depends on RTOS functionality. RTOS is not present with the bare metal profile. Adding the mbed_lib.json file allows all files in targets/TARGET_Cypress/TARGET_PSOC6/COMPONENT_WHD to not be included for the bare metal profile.

The alternative was to add #if MBED_CONF_RTOS_PRESENT to all the files in the targets directory that are dependent on RTOS functionality as I have shown here. I opted for the present PR as there are less file changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have made changes to address the comment here.

@@ -0,0 +1,6 @@
{
"name": "cy_pscoc6csp_rtos",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@hugueskamba hugueskamba Feb 15, 2020

Choose a reason for hiding this comment

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

The baremetal configuration does not have an RTOS. This Mbed library file has been added to make all files in targets/TARGET_Cypress/TARGET_PSOC6/psoc6csp/abstraction/rtos part of a new library called cy_pscoc6csp_rtos. With this, I am able to to exclude all files in this directory when building with the bare metal profile.

The alternative was to add #if MBED_CONF_RTOS_PRESENT to all the files in that directory that are dependent on RTOS functionality as I have shown here.

Although the target supports all kinds of functionalities (specified in targets/targets/json) not all functionalities are available when building with the bare metal profile.

@ciarmcom ciarmcom requested review from bulislaw, evedon, maclobdell and a team February 14, 2020 16:00
@ciarmcom
Copy link
Member

@hugueskamba, thank you for your changes.
@maclobdell @bulislaw @evedon @ARMmbed/mbed-os-maintainers please review.

@hugueskamba
Copy link
Collaborator Author

This force-push removes the changes made to Cypress files (in response to this comment). Instead the files that are dependent on RTOS are moved and used to create a new library that is automatically excluded when building with the bare metal profile.

@hugueskamba hugueskamba force-pushed the hk-fix-baremetal-cy9cproto branch from 6c0b80f to f52597f Compare February 16, 2020 12:57
Make a Mbed library with Cypress WHD files so it is automatically excluded
when building with the bare metal profile. Create another Mbed library to
group network files that use WHD so they can also be excluded fro the bare
metal profile.
@hugueskamba hugueskamba force-pushed the hk-fix-baremetal-cy9cproto branch from f52597f to 18193ab Compare February 16, 2020 13:02
@0xc0170 0xc0170 requested a review from vmedcy February 17, 2020 13:26
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 17, 2020

To summarize - baremetal ignores whd, rtos and common network component and building full Mbed OS will include them, correct? This is important detail - we should cover this in our design guide (I dont recall seeing it there, is it there?).

@ARMmbed/team-cypress Please review

@mergify mergify bot added needs: CI and removed needs: review labels Feb 17, 2020
@hugueskamba
Copy link
Collaborator Author

To summarize - baremetal ignores whd, rtos and common network component and building full Mbed OS will include them, correct?

Yes. The PR CI tests should demonstrate that building with full Mbed OS works fine.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 17, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 17, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@maclobdell
Copy link
Contributor

@ARMmbed/team-cypress please review. this should be gated by your approval.

@kyle-cypress
Copy link

I'm not too familiar with the bare metal profile and it's not clear from https://os.mbed.com/docs/mbed-os/v5.15/reference/mbed-os-bare-metal.html how you can change the components, macros etc when building under that profile. But I'm assuming that there is some fairly flexible mechanism given that Cypress-specific macro values and library names have been proposed in this PR.
Based on that premise, I believe that the undesired code can be disabled almost entirely using pre-existing mechanisms (the change in mbed_overrides.c is still required):

  1. Remove the components WHD and RTX
  2. Remove the CYBSP_WIFI_CAPABLE macro (it is currently defined in targets.json)

@hugueskamba
Copy link
Collaborator Author

I'm not too familiar with the bare metal profile and it's not clear from https://os.mbed.com/docs/mbed-os/v5.15/reference/mbed-os-bare-metal.html how you can change the components, macros etc when building under that profile. But I'm assuming that there is some fairly flexible mechanism given that Cypress-specific macro values and library names have been proposed in this PR.
Based on that premise, I believe that the undesired code can be disabled almost entirely using pre-existing mechanisms (the change in mbed_overrides.c is still required):

  1. Remove the components WHD and RTX
  2. Remove the CYBSP_WIFI_CAPABLE macro (it is currently defined in targets.json)

@kyle-cypress,
Thank you for your input.
Please note that the intent is to exclude it ONLY for the baremetal profile.
Your proposed solution above will also remove the components for non baremetal builds (with full Mbed OS) which is not what we want to do.

@kyle-cypress
Copy link

I'm not too familiar with the bare metal profile and it's not clear from https://os.mbed.com/docs/mbed-os/v5.15/reference/mbed-os-bare-metal.html how you can change the components, macros etc when building under that profile. But I'm assuming that there is some fairly flexible mechanism given that Cypress-specific macro values and library names have been proposed in this PR.
Based on that premise, I believe that the undesired code can be disabled almost entirely using pre-existing mechanisms (the change in mbed_overrides.c is still required):

  1. Remove the components WHD and RTX
  2. Remove the CYBSP_WIFI_CAPABLE macro (it is currently defined in targets.json)

@kyle-cypress,
Thank you for your input.
Please note that the intent is to exclude it ONLY for the baremetal profile.
Your proposed solution above will also remove the components for non baremetal builds (with full Mbed OS) which is not what we want to do.

@hugueskamba
To clarify, my suggestion is to remove those components and macro ONLY when building for the baremetal profile. Given the previous proposals (e.g. relying on a new macro MBED_CONF_CY_PSCOC6_WHD_PRESENT) I am assuming that you have a mechanism for customizing the app configuration for baremetal (is there somewhere I can read about this?)

@hugueskamba
Copy link
Collaborator Author

I'm not too familiar with the bare metal profile and it's not clear from https://os.mbed.com/docs/mbed-os/v5.15/reference/mbed-os-bare-metal.html how you can change the components, macros etc when building under that profile. But I'm assuming that there is some fairly flexible mechanism given that Cypress-specific macro values and library names have been proposed in this PR.
Based on that premise, I believe that the undesired code can be disabled almost entirely using pre-existing mechanisms (the change in mbed_overrides.c is still required):

  1. Remove the components WHD and RTX
  2. Remove the CYBSP_WIFI_CAPABLE macro (it is currently defined in targets.json)

@kyle-cypress,
Thank you for your input.
Please note that the intent is to exclude it ONLY for the baremetal profile.
Your proposed solution above will also remove the components for non baremetal builds (with full Mbed OS) which is not what we want to do.

@hugueskamba
To clarify, my suggestion is to remove those components and macro ONLY when building for the baremetal profile. Given the previous proposals (e.g. relying on a new macro MBED_CONF_CY_PSCOC6_WHD_PRESENT) I am assuming that you have a mechanism for customizing the app configuration for baremetal (is there somewhere I can read about this?)

The way the baremetal profile is built is by only selecting what it needs (as opposed to removing what it does not need). This is done by using the "requires" attribute to select only what is needed in the baremetal profile configuration file.

The macro MBED_CONF_CY_PSOC6_WHD_PRESENT is automatically generated by the addition of the mbed_lib.json and can be used if needed for a pre-processor condition. However, the simple fact of adding mbed_lib.json makes the files within that directory part of a new library (called cy_psoc6_whd) and the library is not included by the baremetal profile configuration file. Since the files in the new library are not included in the build the problem is solved.

I am assuming that you have a mechanism for customizing the app configuration for baremetal (is there somewhere I can read about this?)

https://os.mbed.com/docs/mbed-os/v5.15/reference/configuration.html

@vmedcy
Copy link
Contributor

vmedcy commented Feb 20, 2020

Hello @hugueskamba,

I have just reviewed https://github.com/ARMmbed/mbed-os-example-blinky-baremetal/blob/master/mbed_app.json and now understand the application-defined "requires": ["bare-metal"] overrides the default list of included libraries. This effectively means that any customers that already have mbed_app.json with custom "requires:" in their mbed_app.json will get sudden change in behavior (WHD, common/network and abstraction-rtos excluded from build). Of course, such change in behavior is acceptable in new major Mbed OS version.

I understand the goal to get the mbed-os-example-blinky-baremetal app working out-of-box for PSoC 6 targets - this is very good goal. Alternative would be to add custom target_overrides for PSoC 6 targets to exclude COMPONENT_WHD and COMPONENT_RTX - but this doesn't solve issue with common/network. Hence I approve this PR. @kyle-cypress do you see any issues?

@0xc0170 0xc0170 merged commit 3662759 into ARMmbed:master Feb 20, 2020
@hugueskamba hugueskamba deleted the hk-fix-baremetal-cy9cproto branch February 20, 2020 13:05
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.

7 participants