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

Cellular: Power ON Wait Increased for C030_U201 #10229

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

mudassar-ublox
Copy link
Contributor

Description

Increased wait for power up of module in CellularDevice test case, as UBLOX_C030_U201 took 10-12 sec to power up.

Pull request type

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

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from a team March 26, 2019 14:00
@ciarmcom
Copy link
Member

@mudassar-ublox, thank you for your changes.
@ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

@cmonr
Copy link
Contributor

cmonr commented Mar 26, 2019

This change feels like the wrong thing to do.

Why does the single target need more time to wait?
(It would also be good to add this information into the commit message as well)

@fahimalavi
Copy link
Contributor

@cmonr
The duration of SARA-U2 series module's switch-on routine can vary depending on the application / network settings and the concurrent module activities. Hard-coded time of 5 seconds is unsafe.

@fahimalavi
Copy link
Contributor

@RobMeades

@mudassar-ublox
Copy link
Contributor Author

@fahim-ublox & @RobMeades

@RobMeades
Copy link
Contributor

RobMeades commented Mar 27, 2019

@cmonr: could also just make it 10 seconds for all case, to remove the conditional, it wouldn't do any harm.

@cmonr
Copy link
Contributor

cmonr commented Mar 27, 2019

@cmonr: could also just make it 10 seconds for all case, to remove the conditional, it wouldn't do any harm.

@RobMeades @mudassar-ublox I think I'd prefer that solution instead.

@cmonr
Copy link
Contributor

cmonr commented Mar 27, 2019

@AriParkkila Thoughts one way or another?

@AriParkkila
Copy link

@cmonr I think the proposed change is fine, however :)

It seems that cellular testing will be replaced by the generic Mbed OS netsocket and network-interface tests:

  • mbed test -n tests-netsocket-* --app-config=tools/test_configs/CellularInterface.json -vv
  • mbed test -n tests-network-interface --app-config=tools/test_configs/CellularInterface.json -vv

@cmonr
Copy link
Contributor

cmonr commented Mar 28, 2019

@AriParkkila Not sure I follow your last comment. Could you elaborate?

@AriParkkila
Copy link

@cmonr it's fine to add 10 seconds in all cases. However, I probably wouldn't use very much effort on cellular specific Greentea tests anymore due to the generic Mbed OS TESTS for network-interface and netsocket have much better test coverage are now good for cellular testing too.

@cmonr
Copy link
Contributor

cmonr commented Mar 29, 2019

and netsocket have much better test coverage are now good for cellular testing too.

Aaah, gotcha. This was the part I wasn't clear on.

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.

Woudl refer a blanket increase in wait time

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.

Would prefer a banket wait time.

@cmonr
Copy link
Contributor

cmonr commented Mar 29, 2019

@mudassar-ublox If you could update the change to simply be blanket wait time increase, I can approve and get CI started on this.

@mudassar-ublox
Copy link
Contributor Author

@cmonr pull request is updated. Please review now.

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

For future reference, increase fix like this should contain a reason for increase in the commit msg (why are we changing this from 5 to 10).

@cmonr
Copy link
Contributor

cmonr commented Apr 2, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 2, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 82e6b2f into ARMmbed:master Apr 2, 2019
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.

9 participants