Cellular: Convert to Chrono #12430

kjbracey · 2020-02-13T15:00:32Z

Summary of changes

Covert cellular code to use new Chrono APIs from #12142

Note that documentation for random_max_start_delay config setting has
been changed to indicate that the setting is in seconds, and always has
been. No functional change.

Impact of changes

Eliminate use of deprecated core APIs.

Some public cellular APIs now have dual Chrono and non-Chrono forms - non-Chrono not yet deprecated.

Migration actions required

Documentation

None

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] 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)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2020-02-13T16:00:43Z

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

AriParkkila · 2020-02-14T06:01:52Z

features/cellular/framework/device/CellularStateMachine.cpp

    // so that not every device don't start at the exact same time (for example after power outage)
-    _start_time = rand() % (MBED_CONF_CELLULAR_RANDOM_MAX_START_DELAY);
+    _start_time(rand() % (MBED_CONF_CELLULAR_RANDOM_MAX_START_DELAY)),


Is else _start_time(0) needed here?

No, because Chrono durations and time_points are initialised to zero by default. Still, could have it explicit. That would be more consistent with the existing code.

I guess I was pushing back a bit on the long C++98-style initialiser lists on constructors. Since C++11, member initialisation that doesn't depend on constructor parameters shouldn't be there. They should be in default member initialisers (if even necessary).

AriParkkila · 2020-02-14T06:10:18Z

features/cellular/framework/targets/QUECTEL/M26/QUECTEL_M26_CellularStack.cpp

-        uint64_t start_time = rtos::Kernel::get_ms_count();
-        while (!ready_to_send && start_time < rtos::Kernel::get_ms_count() + SOCKET_SEND_READY_TIMEOUT) {
+        auto start_time = rtos::Kernel::Clock::now() ;
+        while (!ready_to_send && rtos::Kernel::Clock::now() < start_time + SOCKET_SEND_READY_TIMEOUT) {


Good catch 👍

AriParkkila · 2020-02-14T06:23:16Z

features/cellular/mbed_lib.json

@@ -10,7 +10,7 @@
            "value": false
        },
        "random_max_start_delay": {
-            "help": "Maximum random delay value used in start-up sequence in milliseconds",
+            "help": "Maximum random delay value used in start-up sequence in seconds",


Another good catch 👍

That's kind of the point of the Chrono stuff - it catches it for you :)

kjbracey · 2020-02-14T09:11:14Z

Re-looking at code after review comments, noticed this print needed fixing:

    -    tr_info("Startup delay %d ms", _start_time);
    +    tr_info("Startup delay %d s", _start_time.count());

mergify · 2020-03-16T14:36:00Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

kjbracey · 2020-04-30T10:27:53Z

Rebased.

0xc0170 · 2020-05-08T08:56:18Z

CI started

mbed-ci · 2020-05-08T10:02:31Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

0xc0170 · 2020-05-08T10:46:01Z

Unittests failed, please review

kjbracey · 2020-05-08T14:21:54Z

One actual code fix - legacy ATHandler::set_at_timeout wasn't passing through default_timeout parameter to Chrono version.

Cellular unit tests updated.

kjbracey · 2020-05-11T06:49:17Z

CI started

mbed-ci · 2020-05-11T07:43:37Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

Note that documentation for random_max_start_delay config setting has been changed to indicate that the setting is in seconds, and always has been. No functional change.

kjbracey · 2020-05-11T11:20:03Z

Correction - _start_time not being properly initialised to 0.

kjbracey · 2020-05-11T11:24:25Z

CI started

mbed-ci · 2020-05-11T12:31:22Z

Test run: SUCCESS

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

kjbracey added the needs: preceding PR label Feb 13, 2020

mergify bot added the needs: work label Feb 13, 2020

ciarmcom requested review from a team February 13, 2020 16:00

ciarmcom added the needs: review label Feb 13, 2020

AriParkkila approved these changes Feb 14, 2020

View reviewed changes

kjbracey force-pushed the chrono_cellular branch from 04993b5 to 98664d9 Compare February 14, 2020 09:10

mergify bot removed the needs: review label Feb 14, 2020

kjbracey force-pushed the chrono_cellular branch from 98664d9 to 7ae283d Compare April 30, 2020 10:27

kjbracey removed the needs: preceding PR label Apr 30, 2020

kjbracey force-pushed the chrono_cellular branch from 7ae283d to 9e90f80 Compare April 30, 2020 11:33

0xc0170 added needs: CI and removed needs: work labels May 7, 2020

0xc0170 added needs: work and removed needs: CI labels May 8, 2020

kjbracey force-pushed the chrono_cellular branch from 9e90f80 to 23353a3 Compare May 8, 2020 14:21

kjbracey added needs: CI and removed needs: work labels May 11, 2020

kjbracey added 2 commits May 11, 2020 14:18

Cellular: Convert to Chrono

355b45a

Note that documentation for random_max_start_delay config setting has been changed to indicate that the setting is in seconds, and always has been. No functional change.

Add Kernel::impl::get_tick_count() stub

3b7eae1

kjbracey added 2 commits May 11, 2020 14:18

Add Semaphore Chrono stubs

ee88235

Add ThisThread Chrono stubs

74a51b6

kjbracey force-pushed the chrono_cellular branch from 23353a3 to 74a51b6 Compare May 11, 2020 11:18

0xc0170 approved these changes May 13, 2020

View reviewed changes

0xc0170 added ready for merge release-version: 6.0.0-rc1 and removed needs: CI labels May 13, 2020

0xc0170 merged commit 170931e into ARMmbed:master May 13, 2020

mergify bot removed the ready for merge label May 13, 2020

Cellular: Convert to Chrono #12430

Cellular: Convert to Chrono #12430

Uh oh!

Conversation

kjbracey commented Feb 13, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Feb 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Feb 14, 2020

Uh oh!

mergify bot commented Mar 16, 2020

Uh oh!

kjbracey commented Apr 30, 2020

Uh oh!

0xc0170 commented May 8, 2020

Uh oh!

mbed-ci commented May 8, 2020

Uh oh!

0xc0170 commented May 8, 2020

Uh oh!

kjbracey commented May 8, 2020

Uh oh!

kjbracey commented May 11, 2020

Uh oh!

mbed-ci commented May 11, 2020

Uh oh!

kjbracey commented May 11, 2020

Uh oh!

kjbracey commented May 11, 2020

Uh oh!

mbed-ci commented May 11, 2020

Uh oh!

Uh oh!