Skip to content

enable MOTE_L152 for OS5 #7534

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

Merged
merged 2 commits into from
Aug 27, 2018
Merged

enable MOTE_L152 for OS5 #7534

merged 2 commits into from
Aug 27, 2018

Conversation

bentcooke
Copy link
Contributor

@bentcooke bentcooke commented Jul 17, 2018

Description

Enabling OS5 support for the MOTE_L152RC target. This supersedes the now closed #6250.

Pull request type

[ ] Fix
[ ] Refactor
[x] New target
[ ] Feature
[ ] Breaking change

Test logs from mbed-os v5.9.2:
MOTE_L152RC_GCC_20180716-163839.log
MOTE_L152RC_IAR_20180717-111451.log
MOTE_L152RC_ARM_20180717-111451.log

NVStore test failures appear to be heap related (32K RAM) and similar to comments on #7127. Needs further investigation. fixed! Depends on #7746
IAR Filesystem test failures are also likely due to lack of heap and this target should have these tests disabled. Issue #6008. fixed with rebase.
lpticker test failures being reported in separate issue. fixed with commit "Increase max func exec time to allow slower sys clock" in this PR

@dudmuck

Updated logs showing all previously failing tests now passing:
MOTE_L152RC_ARM_20180822-111451.log
MOTE_L152RC_GCC_20180822-163839.log
MOTE_L152RC_IAR_20180822-111451.log

Dependent PR: #7746

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

Gonna ask that the tabs be replaced with spaces.

Otherwise, looks good and should be able to start CI.

@cmonr
Copy link
Contributor

cmonr commented Jul 18, 2018

@ARMmbed/mbed-os-storage Thoughts on possible rememdies for the NVstore and IAR filesystem issues?

@bentcooke Mind mentioning the issue number here?

@bentcooke
Copy link
Contributor Author

@cmonr tabs removed! What issue are you referring to?

@cmonr
Copy link
Contributor

cmonr commented Jul 19, 2018

@bentcooke I was asking about the issues referenced in the PR description.

@cmonr cmonr requested a review from a team July 19, 2018 15:23
@cmonr cmonr removed the needs: work label Jul 19, 2018
@bentcooke
Copy link
Contributor Author

Here they are:
#7557
#7555

@cmonr
Copy link
Contributor

cmonr commented Jul 20, 2018

@bentcooke Thanks.

For reference though, this will need to pass all tests before it can be brought in.

@cmonr
Copy link
Contributor

cmonr commented Jul 30, 2018

@bentcooke In reference to #7557 (comment), would you mind making a comment here once that PR is ready so we can track the dependency?

@bentcooke
Copy link
Contributor Author

@cmonr I already pulled the commits into this PR to update the lp ticker speed test to allow for slower sys clocks (Issue #7557) and to remove this target from the filesystem tests due to the heap limitations(Issue #6008).
Would you prefer separate PRs for these changes?

@cmonr
Copy link
Contributor

cmonr commented Jul 31, 2018

@bentcooke This is fine, since they're seperate commits.

@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-hal Could y'all take a look at the small test changes?

@cmonr cmonr requested a review from a team July 31, 2018 18:38
@cmonr cmonr removed the needs: work label Jul 31, 2018
@bentcooke
Copy link
Contributor Author

@cmonr How can we get this moving again?

@cmonr
Copy link
Contributor

cmonr commented Aug 8, 2018

@bentcooke Still waiting on @mbed-os-hal and @ARMmbed/mbed-os-storage to take a look at the test changes. I recall there being some lengthy discussion about the test timings, so I want to get their input on the chance.

In the meanwhile, I think we can start CI since enabling Mbed OS 5 might bring up some issues to correct in the meanwhile.

/morph build

@dannybenor
Copy link

@davidsaada is there a better way to run these tests?

@@ -23,6 +23,11 @@

using namespace utest::v1;

// TODO HACK, replace with available ram/heap property
#if defined(TARGET_MTB_MTS_XDOT) || defined(TARGET_MOTE_L152RC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Excluding specific boards is probably not the best way to handle these failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is correct, TODO HACK shall be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this is not ideal, but I was following precedent set here:
dcd0e6b#diff-4e5d26193a7c8cf8e05e95d7775dcf84

@davidsaada
Copy link
Contributor

davidsaada commented Aug 8, 2018

@dannybenor et al. Excluding specific platforms from tests is probably not the best way to solve these problems. Now, I believe that the problem is similar to the one we see in #7712. To make a long story short, the problem is not with the test itself, but with the stack stats that generate a report, allocating more memory for this purpose. Problem is in the NVStore test itself. I have created #7746 to fix this issue. @bentcooke Please check whether this PR resolves your problem (without the need to exclude your platform specifically).

@bentcooke
Copy link
Contributor Author

Followed up on #7746

@cmonr
Copy link
Contributor

cmonr commented Aug 15, 2018

@davidsaada @bentcooke I'm not sure I follow the latest comments. Has work migrated to another PR? Is this one good to go?

@davidsaada
Copy link
Contributor

I'm not sure I follow the latest comments. Has work migrated to another PR? Is this one good to go?

Not exactly. Wasn't fond of some the changes here, specifically all the ones excluding this platform from failing tests. The failing BD tests were fixed in other PRs. The NVStore test, however, keeps failing on hard fault, something which needs to be debugged.

@davidsaada
Copy link
Contributor

@bentcooke
After rebasing this PR on top of master and #7746 all the problematic tests pass, including the NVStore one (tested with your MOTE_L152 board). This can be found under davidsaada/mbed-os:david_nvstore_test_low_mem_mote_l152rc branch.
What I suggest is the following:

  • Revert all changes in test codes, ones that exclude this target specifically (4 files). This will leave you with just two files in this PR (common_tickers main and tagets.json).
  • Rebase (this will handle failures in block device tests).
  • Make this PR depend on #7746. This will handle the NVStore failure.

@bentcooke
Copy link
Contributor Author

@davidsaada The fork has been rebased, commit omitting target removed and retested. It appears that all tests are now passing after the rebase and applying the fix from #7746. Thanks for the help with this one!

I have updated the top comment in this thread with the new logs and comments.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 23, 2018

@davidsaada Please review again

Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

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

Looks great now. Thanks.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 24, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

Build : SUCCESS

Build number : 2895
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7534/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Aug 24, 2018

@mbed-ci
Copy link

mbed-ci commented Aug 25, 2018

@0xc0170 0xc0170 merged commit ce28c91 into ARMmbed:master Aug 27, 2018
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