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

Improve PSoC 6 post-build hooks, whitelist makefile export #9466

Merged
merged 3 commits into from Jan 28, 2019

Conversation

Projects
None yet
9 participants
@vmedcy
Copy link
Contributor

vmedcy commented Jan 23, 2019

Description

Various improvements to the Cypress PSoC 6 post-build python hooks that bring compatibility with Cypress PSoC 6 boards (pull request pending). The fixes have no functional impact on FUTURE_SEQUANA port and enable export to makefile of PSoC 6 targeting projects.

This pull request addresses some review feedback from #8491 related to hex file discovery in the exported projects.

Pull request type

[ ] Fix
[ ] Refactor
[X] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@theotherjimmy @lrusinowicz @orenc17

vmedcy added some commits Jan 22, 2019

PSOC6.py: remove silicon ID check
PSoC 6 hex files contain 4-byte chip ID at virtual offset 0x90500002
added by PSoC Creator or cymcuelftool from .cymeta ELF section.
merge_images compares chip ID in CM0+ and CM4 hex files and raises
an exception in case of mismatch. Chip ID is different for each MPN
(for example, 0xE2072100 for CY8C6347BZI-BLD53 and 0xE2062100 for
CY8C6247BZI-D54). CM0+ prebuilt images target CY8C6347BZI-BLD53
but should be compatible with other PSoC6 MPNs.
Remove the check to enable merging CM0+ images with CM4 applications
built for different MPNs, with empty or absent cymetadata.
PSOC6.py: do not require metadata during HEX merging
Replace hard-coded numeric offsets of PSoC 6 hex file sections
with sensible constants.
Do not attempt to update the checksum and metadata contents
if the sections are not found in the original HEX file.
PSOC6: refactor M0 image merging, enable export to makefile
Rename the existing PSoC-specific m0_core_img key in targets.json
as a more generic hex_filename key. Update makefile exporter to select
the subset of resources.hex_files matching the hex_filename value.
Without this fix, multiple prebuilt CM0+ hex files are found in the
target resources and erroneously passed to the srec_cat tool.

The fix is generic so other targets that need post-build hex merging
can use this key to pass the correct image to srecord tool.

The fix also removes sub_target key: instead, rely hex_filename json
key to detect if the hex image merging needs to be done.
The sub_target is not used in mbed-os codebase for anything else.

It is possible to override the hex file name in mbed_app.json:
{
  "target_overrides": {
    "*": {
      "target.hex_filename": "my_custom_m0_image.hex"
    }
}

@ciarmcom ciarmcom requested review from orenc17 , theotherjimmy and ARMmbed/mbed-os-maintainers Jan 23, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Jan 23, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 23, 2019

Very good commit messages 👍

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

This is a breaking change: the prior definition of the PSoC6 will no longer work after these changes (mostly the rename of target.m0_core_img to hex_filename). Is this target released?


Yeah, but nobody used it in the online compiler, or outside of commenters on this PR

@mikisch81

This comment has been minimized.

Copy link
Contributor

mikisch81 commented Jan 23, 2019

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Jan 23, 2019

@theotherjimmy:

This is a breaking change: the prior definition of the PSoC6 will no longer work after these changes (mostly the rename of target.m0_core_img to hex_filename). Is this target released?

I agree. The FUTURE_SEQUANA target was released with mbed-os-5.10.3. But the board is not yet available publicly, so the impact is limited to engineers working on preview version of the board AND customizing the "target.m0_core_img" in the mbed_app.json.

I dislike referencing target.m0_core_img in tools/export/makefile since the key name is too PSoC-specific.
I also dislike having different post-binary hooks for Cypress and Future PSoC 6 targets and only whitelisting Cypress hooks for makefile export (this functionality is critical for Cypress).
I believe renaming the key to hex_filename is a proper solution at this point of time.
I plan to submit similar change to tools/export/cmake whitelisting the PSOC6Code.complete hook, once this PR is approved.

@lrusinowicz

This comment has been minimized.

Copy link
Contributor

lrusinowicz commented Jan 23, 2019

@theotherjimmy:

This is a breaking change: the prior definition of the PSoC6 will no longer work after these changes (mostly the rename of target.m0_core_img to hex_filename). Is this target released?

I agree. The FUTURE_SEQUANA target was released with mbed-os-5.10.3. But the board is not yet available publicly, so the impact is limited to engineers working on preview version of the board AND customizing the "target.m0_core_img" in the mbed_app.json.

Agreed about the engineers affected. That's probably only me and @orenc17. Also, this is probably the last moment such a change could be pulled out.

I dislike referencing target.m0_core_img in tools/export/makefile since the key name is too PSoC-specific.

My point was that hex_filename was too generic, but on the other hand, using it works quite nicely in the makefile exporter.

I also dislike having different post-binary hooks for Cypress and Future PSoC 6 targets and only whitelisting Cypress hooks for makefile export (this functionality is critical for Cypress).

This, indeed, wouldn't make sense.

I believe renaming the key to hex_filename is a proper solution at this point of time.
I plan to submit similar change to tools/export/cmake whitelisting the PSOC6Code.complete hook, once this PR is approved.

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Jan 23, 2019

@lrusinowicz: thank you for your feedback, this is nice that we are aligned on this.

@lrusinowicz
Copy link
Contributor

lrusinowicz left a comment

I would consider changing this to generate warning only, rather than error, and to handle "no metadata" case to not generate warning. This way hex images for M0 fitting multiple targets could exists (without signature) and user at least will be warned when trying to use image targeting incompatible MPN.

@vmedcy

This comment has been minimized.

Copy link
Contributor Author

vmedcy commented Jan 23, 2019

@lrusinowicz I do not think metadata check should be performed, for various reasons:

  1. Cypress deprecates the silicon ID in .cymeta section. This belongs to legacy PSoC Programmer / PSoC Creator programming flow. The new Cypress SW offerings (ModusToolbox and Cypress Programmer) do not perform any metadata injection / verification.

  2. The same HEX file should easily work for multiple MPNs that belong to the same PSoC 6 device series. The warning can be confusing.

  3. The check would only prevent a case when both M0+ and M4 HEX images contain metadata. When .cymeta section is missing in any of the images, ihex.tobinarray(start=0x90500002, ) will not work.

  4. There are multiple (more significant) reasons why the CM0+ image might not work: invalid CY_CORTEX_M4_APPL_ADDR or FLASH/SRAM layout in the linker script.

  5. toolchain.notify.info is not available in merge_images context, do not want to pass another message_func callback

@lrusinowicz

This comment has been minimized.

Copy link
Contributor

lrusinowicz commented Jan 23, 2019

@vmedcy
OK, if that's the case, especially point 1 which I was not aware off, then I have no more comments.

@orenc17
Copy link
Contributor

orenc17 left a comment

Looks fine

@vmedcy vmedcy referenced this pull request Jan 24, 2019

Merged

Add Cypress PSoC 6 targets #9481

@theotherjimmy
Copy link
Contributor

theotherjimmy left a comment

I'm happy if we're confidant that very few people will be affected by the break.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 24, 2019

I don't think anyone used the online compiler for this platform, so we should be good to go.

@cmonr

This comment has been minimized.

Copy link
Contributor

cmonr commented Jan 25, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Jan 26, 2019

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 28, 2019

Test restarted

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Jan 28, 2019

I don't think anyone used the online compiler for this platform, so we should be good to go.

Just to confirm : this is fine for 5.11.4 (not 5.12) ?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

theotherjimmy commented Jan 28, 2019

@0xc0170 Correct. 5.11.4

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 28, 2019

@cmonr cmonr merged commit 7f8ebc7 into ARMmbed:master Jan 28, 2019

24 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10264 cycles (+1098 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Jan 28, 2019

vmedcy added a commit to vmedcy/mbed-os that referenced this pull request Feb 19, 2019

PSOC6: enable export to CMake
The approach for the hex_files subset selection is identical
to makefile exporter: ARMmbed#9466

Single hex file should be passed to srec_cat when hex_filename
is set in targets.json or mbed_app.json.

cmonr added a commit that referenced this pull request Feb 21, 2019

PSOC6: enable export to CMake
The approach for the hex_files subset selection is identical
to makefile exporter: #9466

Single hex file should be passed to srec_cat when hex_filename
is set in targets.json or mbed_app.json.

rajkan01 added a commit to rajkan01/mbed-os that referenced this pull request Mar 7, 2019

PSOC6: enable export to CMake
The approach for the hex_files subset selection is identical
to makefile exporter: ARMmbed#9466

Single hex file should be passed to srec_cat when hex_filename
is set in targets.json or mbed_app.json.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.