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-manager to 0.2.3 #8757

Merged
merged 17 commits into from Mar 6, 2019

Conversation

Projects
None yet
@theotherjimmy
Copy link
Contributor

commented Nov 15, 2018

Description

This comes with quite a few features:

  • Variant parsing
  • Memory permission bits for:
    • Read
    • Write
    • Execute
    • Secure
    • Non-Secure
    • Non-Secure Callable
    • Peripheral
  • Memory default information
  • Descriptions of core as either:
    • Semetric Multi-core
    • Asymetric Multi-core

Further, The index has been re-formatted to be human-readable and all
keys are sorted so that diffs are also human-readable. Be aware, it's
big.

NOTE: This will conflict with #8607 Rebased after #8607

Pull request type

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

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Looks at changes
Sees 450k+ changes
Uhhh....

Sees tools/arm_pack_manager/index.json was updated
Oh. Carry on.

@theotherjimmy Would/could that file benefit from getting new lines added to it, similar to how target.json was updated, so that the diff generated isn't 99.9% if the PR when it's updated?

@bridadan
Copy link
Contributor

left a comment

Few clarifications

Show resolved Hide resolved tools/config/__init__.py Outdated
]
if all_matching_memories:
memory = sorted(
all_matching_memories, key=self._memory_ordering

This comment has been minimized.

Copy link
@bridadan

bridadan Nov 15, 2018

Contributor

I'm a bit tripped up on the key function here. Is this essentially doing the following for the comparison:

"
For each memory in all_matching_memories, the one that is the "least" is the one that:

  1. Has the smallest value for memory['default']
  2. If they are the same, pick the smallest value for memory['size']
  3. If that is the same, pick the smallest value for memory['start']

"

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Nov 16, 2018

Author Contributor

@bridadan I was under the impression that it would be largest first. If I need a quick reverse to make them all "largest" then that's what I'll do.

This comment has been minimized.

Copy link
@bridadan

bridadan Nov 16, 2018

Contributor

sorted will return an ascending order sorted list by default: https://docs.python.org/3/howto/sorting.html. So that would mean the first one in the list would be the "smallest" if I'm interpreting the doc correctly.

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Nov 16, 2018

Author Contributor

Right. Adding a reverse now.

@cmonr

cmonr approved these changes Nov 15, 2018

Copy link
Contributor

left a comment

Nice!

@@ -19,3 +19,4 @@ six==1.11.0
git+https://github.com/armmbed/manifest-tool.git@v1.4.6
mbed-cloud-sdk==2.0.1
icetea>=1.0.2,<1.1
cmsis-pack-manager>=0.2.2,<0.3.0

This comment has been minimized.

Copy link
@cmonr

cmonr Nov 15, 2018

Contributor

🎉


def _get_sectors(self, device):
"""Extract sector sizes from device FLM algorithm
Will return None if there is no algorithm, pdsc URL formatted in correctly
Will return None if there is no algorithm, pdsc URL formatted in

This comment has been minimized.

Copy link
@cmonr

cmonr Nov 15, 2018

Contributor

Not sure why, but this feels like weird english.

Is it missing an or?

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Nov 16, 2018

Author Contributor

Yes.

@@ -1,192 +0,0 @@
from __future__ import print_function, division, absolute_import

This comment has been minimized.

Copy link
@cmonr

cmonr Nov 15, 2018

Contributor

Goodnight sweet prince

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

@ARMmbed/mbed-os-test-team Heads up. A new python module is being added into mbed-os

https://github.com/ARMmbed/mbed-os/pull/8757/files#diff-b4ef698db8ca845e5845c4618278f29aR22

@theotherjimmy Do you know if cmsis-pack-manager has a minimum Py2/Py3 versions?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@cmonr It's compatible with most Python versions >= 2.7

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@cmonr

Would/could that file benefit from getting new lines added to it, similar to how target.json was updated, so that the diff generated isn't 99.9% if the PR when it's updated?

from the top comment:

Further, The index has been re-formatted to be human-readable and all
keys are sorted so that diffs are also human-readable. Be aware, it's
big.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

Further, The index has been re-formatted to be human-readable and all
keys are sorted so that diffs are also human-readable. Be aware, it's
big.

Ha. Silly GitHub not wanting to load the file...

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

/morph build

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

@0xc0170 , @cmonr I've advanced this one, any reason why it still needs work?

@mbed-ci

This comment has been minimized.

Copy link

commented Nov 18, 2018

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

@theotherjimmy this PR ran into a strange build failure, mind taking a look:
Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/mbed/mbed.py", line 3338, in main
status = pargs.command(pargs)
File "/usr/local/lib/python2.7/dist-packages/mbed/mbed.py", line 1985, in thunk
return command(**argv)
File "/usr/local/lib/python2.7/dist-packages/mbed/mbed.py", line 2769, in test_
icetea_supported = program.requirements_contains('icetea')
File "/usr/local/lib/python2.7/dist-packages/mbed/mbed.py", line 1603, in requirements_contains
with open(os.path.join(req_path, req_file), 'r') as f:
IOError: [Errno 2] No such file or directory: '/builds/ws/mbed-os-build-matrix/3666/default_GCC_ARM/requirements.txt'

@0xc0170 0xc0170 removed the needs: CI label Nov 19, 2018

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

NOTE: The history is not clean yet. I plan to squash/rewrite commit
messages to meet commit message standards.

@NirSonnenschein labeled as needs: work because of this

flm = pack.open(filename)
flm = pack.open(
algo["file_name"]
.replace("\\\\", "/")

This comment has been minimized.

Copy link
@OPpuolitaival

OPpuolitaival Nov 19, 2018

Contributor

This works also in Windows?

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Nov 19, 2018

Author Contributor

Maybe. I'm not sure.

@OPpuolitaival
Copy link
Contributor

left a comment

Is this tested also in Windows machine?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@OPpuolitaival This has not been tested on a windows machine that's next on my list.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

@0xc0170 The history is actually not so bad. I already did some squashing/rewriting.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

@theotherjimmy - as discussed internally should we move this to 5.12 as it will have broader impact on devices?

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@deepikabhavnani Perhaps "broader impact" is not the correct phrase. "it's a big risk" as it can impact many systems is probably what you were thinking. Especially because it's supposed to have no impact on any devices.

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

@theotherjimmy - Yes agree risk is higher and we might uncover some unknowns as well..

@theotherjimmy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

So, the NUMAKER_PFM_M2351 now has 2x ROMs, each 1/2 the size that it used to have, that are not mapped contiguously. That's very strange indeed.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 5, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Looking into this, but I don't think the jenkins-ci/mbed-os-ci_build-ARMC6 failure is due to the PR.

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 5, 2019

Test run: FAILED

Summary: 1 of 9 test jobs failed
Build number : 13
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Looking into this, but I don't think the jenkins-ci/mbed-os-ci_build-ARMC6 failure is due to the PR.

Any findings? I'll reviewed changes, do not look like this would hit assert in the linker script. I'll fetch this PR and reproduce locally

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

So, the NUMAKER_PFM_M2351 now has 2x ROMs, each 1/2 the size that it used to have, that are not mapped contiguously. That's very strange indeed.

This is what I have noticed (correction - RAM regions are split now) and possibly that is why this assert with ARMC6 now. @ARMmbed/team-nuvoton Should we just changed back to only one RAM region (as it was) and this will be addressed separately?

Here in this PR:

            "IRAM1": {
                "access": {
                    "execute": false, 
                    "non_secure": false, 
                    "non_secure_callable": false, 
                    "peripheral": false, 
                    "read": true, 
                    "secure": false, 
                    "write": true
                }, 
                "default": true, 
                "size": 32768, 
                "start": 536870912, 
                "startup": false
            }, 
            "IRAM2": {
                "access": {
                    "execute": false, 
                    "non_secure": false, 
                    "non_secure_callable": false, 
                    "peripheral": false, 
                    "read": true, 
                    "secure": false, 
                    "write": true
                }, 
                "default": false, 
                "size": 65536, 
                "start": 805339136, 
                "startup": false
            }, 

On master:

"memory": {
    "IRAM1": {"start": "0x20000000", "size": "0x18000"},

This asserts ScatterAssert(ImageLimit(ARM_LIB_HEAP) <= (MBED_RAM_APP_START + MBED_RAM_APP_SIZE))

Heap goes in this PR to RAM1 region that is 3x smaller and does not fit.

If we change regions in cmsis pack index, we need to change also linker scripts. I've checked all 3 linkers scripts, all of them contain one RAM region. I would revert RAM region change in this PR, and will request a new issue to address this (or fix cmsis pack).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Locally reverted RAM2 change for this nuvoton target, builds without error. I'll push this commit here, restart testing when we can. We will need to create an issue for this (RAM2 region addition).

Build successes:
  * NUMAKER_PFM_M2351::ARMC6::MBED-BUILD
  * NUMAKER_PFM_M2351::ARMC6::TESTS-MBED_PLATFORM-SYSTEM_RESET
nuvoton m2351: fix RAM regions - only one RAM1
Revert latest change to index. Linker scripts follow one RAM region. If index is updated,
requires further changes in the target that should be done separately.
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

I pushed a fix (using one RAM region, should have same attributes as it was). Builds locally, Same RAM1 as it was.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Mar 5, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 5, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 14
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@theotherjimmy Can you review the failures? It's not related to this change-set but rather on master already. The issue is being tracked here #9938 .

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 6, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 15
Build artifacts

Failed test jobs:

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

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

CI restarted.
Merge commit needs to use updated master

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 6, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 16
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Exporters restarted (license issue)

@alekla01

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

Restarted exporters

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 6, 2019

@0xc0170 0xc0170 merged commit c37ac83 into ARMmbed:master Mar 6, 2019

28 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-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 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-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 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 10265 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
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
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Mar 6, 2019

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.