Skip to content

mbed test with dual core targets#12630

Merged
0xc0170 merged 1 commit intoARMmbed:masterfrom
jeromecoutant:PR_TEST_DUALCORE
Mar 31, 2020
Merged

mbed test with dual core targets#12630
0xc0170 merged 1 commit intoARMmbed:masterfrom
jeromecoutant:PR_TEST_DUALCORE

Conversation

@jeromecoutant
Copy link
Copy Markdown
Collaborator

@jeromecoutant jeromecoutant commented Mar 16, 2020 •
edited
Loading

Summary of changes

mbed test -m
is using mbedls result to detect the correct plugged target on host.

There is an issue with dual core chips,
because 2 different targets exist in the targets.json file for the same HW

Patch proposal:

  • remove _CM4 and _CM7 before detect HW

@MarceloSalazar

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

Tested with a beta version of STLinkFW supporting hex format

[] 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

@ciarmcom ciarmcom requested review from a team March 16, 2020 14:00
@ciarmcom
Copy link
Copy Markdown
Member

ciarmcom commented Mar 16, 2020

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

@jeromecoutant jeromecoutant mentioned this pull request Mar 16, 2020
Merged
@0xc0170 0xc0170 requested review from jamesbeyond and removed request for a team March 23, 2020 18:55
@0xc0170
Copy link
Copy Markdown
Contributor

0xc0170 commented Mar 23, 2020 •
edited
Loading

Can you amend the commit message to explain there how remove _CM4 and _CM7 before detect HW this fixes the problem?

That is also my question. Looking at the code change, I do not fully understand how removing these suffixes solves the dual core test issue (what is the actual issue there).

@jeromecoutant
Copy link
Copy Markdown
Collaborator Author

jeromecoutant commented Mar 24, 2020

OK

Behind DISCO_H747I target, there are in fact 2 sub-targets, CM4 and CM7 targets.
Both DISCO_H747I_CM4 and DISCO_H747I_CM7 are defined in targets.json, in order to make application for 1 core or the other.

From USB connection point of view, there is only 1 cable.
Binary file drag and drop feature is flashing CM7 side. That's why, in targets.json, DISCO_H747I is an alias to DISCO_H747I_CM7.

From mbedls point of view, and then greentea tests point of view, DISCO_H747I is defined.

  • so testing DISCO_H747I will work on CM7
  • testing DISCO_H747I_CM7 will not work, even if it is the same
  • testing DISCO_H747I_CM4 will not work, as binary file drag and drop is not implemented for CM4

Coming soon: hex format drag and drop feature
=> means that we will be able to execute greentea tests on both CM4 and CM7.
Conditions is this PR, ie to detect DISCO_H747I when user is requesting DISCO_H747I_CM4 or _CM7

@jeromecoutant
mbed test with dual core targets
6c57c9f 
For Dual Core targets, there are in fact 2 sub-targets, CM4 and CM7 targets.
This allows to execute tests with boards detected by mbedls.
@mergify mergify bot added needs: CI and removed needs: review labels Mar 24, 2020
mark-edgeworth
mark-edgeworth reviewed Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mark-edgeworth mark-edgeworth left a comment

Choose a reason for hiding this comment

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

@jamesbeyond might have a view on this as it seems to affect test code only. I am not in favour of arbitrarily stripping parts of a device ID as this implies the ID is compound. There should be a better way of implementing boards with multiple build targets...

@jamesbeyond
Copy link
Copy Markdown
Contributor

jamesbeyond commented Mar 24, 2020

@jamesbeyond might have a view on this as it seems to affect test code only. I am not in favour of arbitrarily stripping parts of a device ID as this implies the ID is compound. There should be a better way of implementing boards with multiple build targets...

Hi @mark-edgeworth, I think this is not only related to tests. this is related to builds API as well.

If I recalled the correctly, this is the exact case like what we been discussed in the new mbed devices demo. DISCO_H747I is the board name, and DISCO_H747I_CM4 or DISCO_H747I_CM7 can be considered as build targets, which are different MCUs on the same board

The old tool couldn't handle and clarify them very well. I hope the new tool could take those use cases into consideration.

jamesbeyond
jamesbeyond approved these changes Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

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

as for this PR, I am OK with the changes, because there is no better option to do it on the current tools.

@mark-edgeworth
Copy link
Copy Markdown
Contributor

mark-edgeworth commented Mar 24, 2020

Yes, this is the same problem, and it is being considered in the new build tools.

@0xc0170
Copy link
Copy Markdown
Contributor

0xc0170 commented Mar 25, 2020

CI started

@mbed-ci
Copy link
Copy Markdown

mbed-ci commented Mar 25, 2020

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels Mar 25, 2020
@adbridge
Copy link
Copy Markdown
Contributor

adbridge commented Mar 27, 2020

Failure is unrelated so restarting CI

@mbed-ci
Copy link
Copy Markdown

mbed-ci commented Mar 27, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Copy Markdown
Contributor

0xc0170 commented Mar 30, 2020

Tests have an issue, we are investigating

@0xc0170
Copy link
Copy Markdown
Contributor

0xc0170 commented Mar 30, 2020

test restarted

@mergify mergify bot removed the needs: CI label Mar 30, 2020
@0xc0170 0xc0170 merged commit d0917b0 into ARMmbed:master Mar 31, 2020
@mergify mergify bot removed the ready for merge label Mar 31, 2020
@jeromecoutant jeromecoutant deleted the PR_TEST_DUALCORE branch March 31, 2020 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

+2 more reviewers

@mark-edgeworth mark-edgeworth mark-edgeworth left review comments

@jamesbeyond jamesbeyond jamesbeyond approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-version: 6.0.0-beta-1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jeromecoutant @ciarmcom @0xc0170 @jamesbeyond @mark-edgeworth @mbed-ci @adbridge