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

Add proper power save mode command format for MTS_DRAGONFLY_L471 #14008

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

felser
Copy link
Contributor

@felser felser commented Dec 7, 2020

Summary of changes

Add +CPSMS command formatted properly for the ublox radio used on the MTS_DRAGONFLY_L471QG.
The default +CPSMS format does not work for this radio.

Impact of changes

Migration actions required

Documentation

None

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

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Tested values for the periodic and active timers in each granularity range on the MTS_DRAGONFLY_L471QG.

Reviewers


@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Dec 7, 2020
@ciarmcom ciarmcom requested review from a team December 7, 2020 23:30
@ciarmcom
Copy link
Member

ciarmcom commented Dec 7, 2020

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

0xc0170
0xc0170 previously approved these changes Dec 9, 2020
@mergify mergify bot added needs: CI and removed needs: review labels Dec 9, 2020
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.

I realized this is in generic AT file, adding TARGET_ exception. Isn't there a better way to do this?

@ARMmbed/mbed-os-connectivity Please review

@trowbridgec
Copy link

The set_power_save_mode() function is marked as virtual for AT_CellularDevice:

    virtual nsapi_error_t set_power_save_mode(int periodic_time, int active_time = 0);

Based on that, it might be best to override the function in the inheriting class instead of making the change universally in the base class.

@felser
Copy link
Contributor Author

felser commented Dec 9, 2020

I can certainly override the function instead. Should I do a fresh PR?

@mbed-ci
Copy link

mbed-ci commented Dec 9, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 10, 2020

I can certainly override the function instead. Should I do a fresh PR?

@felser or update this PR with description.

@mergify mergify bot dismissed 0xc0170’s stale review December 14, 2020 15:51

Pull request has been modified.

@felser
Copy link
Contributor Author

felser commented Dec 14, 2020

I updated this pull request. Now it overrides the set_power_save_mode(...) function in the derived SARA4_PPP class.

@pan-
Copy link
Member

pan- commented Dec 15, 2020

Thanks @felser. Could you provide the tests results associated with this change ?

@felser
Copy link
Contributor Author

felser commented Dec 15, 2020

Test results updated.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 17, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 17, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️

@adbridge
Copy link
Contributor

adbridge commented Jan 6, 2021

@pan- could you please confirm if you are ok with this now ?
Then we will need to re-run CI as it was last run 20 days ago.

@mergify mergify bot removed the needs: CI label Jan 12, 2021
@0xc0170 0xc0170 merged commit 8fb31d7 into ARMmbed:master Jan 12, 2021
@mergify mergify bot removed the ready for merge label Jan 12, 2021
@mbedmain mbedmain added release-version: 6.7.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jan 25, 2021
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.

None yet

8 participants