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

Cellular: Added power and sim greentea and unit tests. #7146

Merged
merged 2 commits into from Jun 19, 2018

Conversation

Projects
None yet
6 participants
@jarvte
Contributor

jarvte commented Jun 6, 2018

Description

  • created power and sim greentea tests
  • added more power and sim unit tests.
  • added 'int do_reset' param to API CellularPower method "virtual nsapi_error_t set_power_level(int func_level, int do_reset = 1);"
    This does not affect to current behaviour as default value is 1 and then it behaves in the old way.
  • AT_CellularSIM::get_imsi(char *imsi) now returns with NSAPI_ERROR_PARAMETER if the given imsi is null. This changes how method behaves but it fixes a crash (and invalid usage of method) so should not make a difference if one thinks this as a API change.

Internal ref to defect: IOTCELL-903-916

@mirelachirica @AnttiKauppila please review.

Pull request type

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

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Jun 6, 2018

@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 11, 2018

@0xc0170 could you please trigger the tests?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 12, 2018

/morph build

*
* @remark See 3GPP TS 27.007 CFUN for more details
*
* @return zero on success
*/
virtual nsapi_error_t set_power_level(int func_level) = 0;
virtual nsapi_error_t set_power_level(int func_level, int do_reset = 1) = 0;

This comment has been minimized.

@cmonr

cmonr Jun 12, 2018

Contributor

Marking for 5.10 due to the API addition.

Otherwise, LGTM.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 12, 2018

Build : SUCCESS

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

Triggering tests

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

MBED_ASSERT(err == NSAPI_ERROR_OK);
wait_for_power(pwr);
/*

This comment has been minimized.

@0xc0170

0xc0170 Jun 12, 2018

Member

Why are we leaving this dead code in? What is the purpose

This comment has been minimized.

@jarvte

jarvte Jun 12, 2018

Contributor

test code made but wasn't able to test it. We will test and uncomment it once we got bg96 to raas & ci.

This comment has been minimized.

@0xc0170

0xc0170 Jun 12, 2018

Member

I would rather leave it out rather commented-out code on master (stash it or push it to your local branch). Once tested, will be added

This comment has been minimized.

@jarvte

jarvte Jun 12, 2018

Contributor

commented test removed.

@mbed-ci

This comment has been minimized.

@jarvte jarvte dismissed stale reviews from cmonr and AnttiKauppila via 9056036 Jun 12, 2018

@jarvte jarvte force-pushed the jarvte:add_cellular_sim_and_power_tests branch from 7e21829 to 9056036 Jun 12, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 12, 2018

/morph build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 13, 2018

Test result is from the previous changeset, we will need to restart CI again

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 13, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 13, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added needs: work and removed needs: CI labels Jun 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 14, 2018

Crud. @jarvte Looks like this'll need a rebase and anther pass through CI...

@jarvte jarvte force-pushed the jarvte:add_cellular_sim_and_power_tests branch from 9056036 to 7e1b048 Jun 15, 2018

@0xc0170 0xc0170 removed the needs: work label Jun 15, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 16, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 18, 2018

@cmonr ci-morph-exporter failure not related to this pr

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 18, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 18, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-wan Jun 18, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 18, 2018

@ARMmbed/mbed-os-wan Please rereview (can't see here any approvals)

@cmonr cmonr added needs: review and removed needs: CI labels Jun 18, 2018

@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 19, 2018

@cmonr please approve and merge

@cmonr

cmonr approved these changes Jun 19, 2018

@cmonr cmonr added ready for merge and removed needs: review labels Jun 19, 2018

@cmonr cmonr merged commit d1dc1e8 into ARMmbed:master Jun 19, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 927 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9587 cycles (+389 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 19, 2018

@jarvte I was looking at this again, and was wondering something.

  • added 'int do_reset' param to API CellularPower method "virtual nsapi_error_t set_power_level(int func_level, int do_reset = 1);"
    This does not affect to current behaviour as default value is 1 and then it behaves in the old way.

I marked this for 5.10 because of the code addition in features/cellular/framework/AT/AT_CellularPower.cpp that adds _at.write_int(do_reset); and modifies the default behavior.

Could you explain how do_reset = 1 does not affect the previous behavior, when it looks like its addition results in a new reset behavior? I'm trying to make sure this is targeted to the proper release.

@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 20, 2018

@cmonr this is actually a bug, I read spec again and noticed that default value should be ''0'. If you don't give that do_reset if same as not doing the reset for the modem before applying the new functionality. I will fix default value to '0' in next PR for network tests.

@jarvte

This comment has been minimized.

Contributor

jarvte commented Jun 20, 2018

PR with the fix: #7269

@jarvte jarvte deleted the jarvte:add_cellular_sim_and_power_tests branch Jun 20, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 20, 2018

@jarvte Wonderful! Since that was my only holdover for making this target a feature release, I'll mark it back for the next patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment