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

Update CMSIS pack index json for STM32H7xx family #11709

Merged
merged 5 commits into from Nov 5, 2019

Conversation

@JanneKiiskila
Copy link
Contributor

JanneKiiskila commented Oct 18, 2019

Description

Manually replaced the existing STM32H7 section and replaced it with the
context of updated index.json that pulled in the Keil packs available
today, as of 18th October, 2019.

See related PR; #11707

Pull request type

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

Reviewers

@0xc0170 @ARMmbed/team-st-mcd @jeromecoutant

Release Notes

CMSIS pack index updated for STM32H7xx -family.

Copy link
Member

0xc0170 left a comment

Becoming hard to review (file too large), I got one unicorn earlier today for this file 🙄

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

jeromecoutant commented Oct 18, 2019

Hi
Could you explain which command you made to get this new file ?

"access": {
"execute": true,
"non_secure": false,

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Oct 18, 2019

Contributor

Can you remove IROM2 part for H743, and double IROM1 size?
as it is a single core, and full ROM can be used.

"access": {
"execute": true,
"non_secure": false,

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Oct 18, 2019

Contributor

How can we rename IROM1 into IROM_CM7 ?
and IROM2 into IROM_CM4 ?

@0xc0170 0xc0170 added the needs: work label Oct 21, 2019
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Oct 28, 2019

@jeromecoutant - for the modifications here - all the information is coming from the cmsis-pack you have supplied to KEIL. Nothing is generated manually and if we start changing things manually, it means we must maintain this file manually forever. Which might or might not be something we really want. @jeromecoutant - in an ideal world, STM would re-generate the STM file, after which I would re-generate the CMSIS-pack to match.

@MarceloSalazar @mark-edgeworth @bulislaw @kjbracey-arm @madchutney - we really need to think what exactly we need and where, as it seems to me have lots of duplicate information. Flash sectors and sizes are in the cmsis-packs (in this PR) AND the target files. Either one of them is a duplicate. We have some similar information also in the compiler linker/scatter files, too.

Also, having one 13 MB JSON file also does not sound like a feasible plan going into the future, we should consider splitting the file into vendor parts (if we truly even need that one .JSON file) - too large files become unmanageable/hard to review.

@mark-edgeworth

This comment has been minimized.

Copy link
Contributor

mark-edgeworth commented Oct 28, 2019

@JanneKiiskila: @ARMmbed/mbed-os-tools are looking at this right now...

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 29, 2019

@jeromecoutant - for the modifications here - all the information is coming from the cmsis-pack you have supplied to KEIL. Nothing is generated manually and if we start changing things manually, it means we must maintain this file manually forever. Which might or might not be something we really want. @jeromecoutant - in an ideal world, STM would re-generate the STM file, after which I would re-generate the CMSIS-pack to match.

@jeromecoutant can you rereview? As data in this PR are coming from the pack, can we use these and once pack is fixed, it will be fixed as well.

We would like to have this update on master as soon as possible.

Also, having one 13 MB JSON file also does not sound like a feasible plan going into the future, we should consider splitting the file into vendor parts (if we truly even need that one .JSON file) - too large files become unmanageable/hard to review.

Separation should help here, it will just grow in size day-by-day.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 29, 2019

@MarceloSalazar @mark-edgeworth @bulislaw @kjbracey-arm @madchutney - we really need to think what exactly we need and where, as it seems to me have lots of duplicate information. Flash sectors and sizes are in the cmsis-packs (in this PR) AND the target files. Either one of them is a duplicate. We have some similar information also in the compiler linker/scatter files, too.

I don't think these are duplicates. Not all drivers provide memory layout in the driver itself like the reference above (they could but there is no way we could get it writing one script without changes per each target family and its SDK). This information is in the cmsis packs - what we use, also what other tools use. This is standardized way to get this data.

Regarding linker scripts - this is one of the things we would like to have - one linker file per toolchain and generate it per target.

@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:CMSIS-STM32H7xx branch from 6325b3c to 652a00d Oct 31, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 31, 2019

Travis reports the device name is not valid:

E           AssertionError: Target DISCO_H747I contains invalid device_name STM32H747XIH6
@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:CMSIS-STM32H7xx branch from 652a00d to 3f8c9e6 Nov 1, 2019
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Nov 1, 2019

Oh yes, the device name for DISCO_H747I was still wrong - it's not STM32H747XIH6 but actually STM32H747AGIx (I presume). @ARMmbed/team-st-mcd - please review.

On the other hand, I'd like to get this one merged in pretty soon - the problem is that the index.json file is now so large, that if a merge conflict happens -> doing a merge for example with P4Merge seems to be near impossible now, as it just goes into non-responsive mode. Seems we've hit some kind of file limit for that application...

targets/targets.json Outdated Show resolved Hide resolved
targets/targets.json Show resolved Hide resolved
targets/targets.json Outdated Show resolved Hide resolved
JanneKiiskila and others added 4 commits Oct 18, 2019
Manually replaced the existing STM32H7 section and replaced it with the
context of updated `index.json` that pulled in the Keil packs available
today, as of 18th October, 2019.

See related PR; #11707
Compilation of bootloader requires these parameters.
Add definitions for H743ZI2, required by bootloader.
The proposed review fixes by Jerome, cherry-picked in.

(cherry picked from commit fb15f02)
Signed-off-by: Janne Kiiskila <janne.kiiskila@arm.com>
@JanneKiiskila JanneKiiskila force-pushed the JanneKiiskila:CMSIS-STM32H7xx branch from 3f8c9e6 to eed920e Nov 4, 2019
"mbed_rom_start": "0x08000000",
"mbed_rom_size" : "0x200000",
Comment on lines +3218 to +3219

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Nov 5, 2019

Contributor

I am sorry, but in order to remove warnings,
we should also add (for both H743ZI and ZI2):
"mbed_ram_start": "0x24000000",
"mbed_ram_size" : "0x80000",

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Nov 5, 2019

Author Contributor

Which warnings? I don't recall seeing any?

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Nov 5, 2019

Author Contributor

Done, anyways.

Adding the RAM definitions per request of Jerome Coutant.
@0xc0170 0xc0170 requested a review from jeromecoutant Nov 5, 2019
@0xc0170 0xc0170 added needs: review and removed needs: work labels Nov 5, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 5, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 5, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

JanneKiiskila commented Nov 5, 2019

Failure seems to be related to exporter tests;

12:33:22 [2019-11-05T10:33:22.164Z] [EXAMPLES]> ERROR FAILURE building mbed-os-example-blinky K64F make_armc6

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 5, 2019

License error, restarting exporters

@0xc0170 0xc0170 merged commit cdcef4b into ARMmbed:master Nov 5, 2019
26 checks passed
26 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-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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8680 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Nov 5, 2019
@JanneKiiskila JanneKiiskila deleted the JanneKiiskila:CMSIS-STM32H7xx branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.