Status callbacks #6032

jarlamsa · 2018-02-07T10:27:45Z

Description

Original PR to a feature branch: #5457
This PR adds cases when disconnect happens without user calling disconnect()
For lwip, if ethernet cable is disconnected
Nanostack 6LoWPAN - if connection to border router / outside is lost
Nanostack Thread - if connection to border router / outside is lost
Nanostack Ethernet - if ethernet cable is disconnected

Status

READY

Migrations

YES

These were discussed in the original PR: #5457
The new commits are not modifying these already discussed changes

In NetworkInterface new APIs:
-attach() for connection status callback registering
-get_connection_status()
-set_blocking()

In PPPCellularInterface:
-connection_status_cb() removed

Related PRs

feature-status-callbacks | #5457

Todos

Documentation needs to be updated

Deploy notes

Steps to test or reproduce

kjbracey · 2018-02-07T10:40:06Z

Looking at that, should we squash the entire original branch to 1 commit, as all the steps are review comments?

jarlamsa · 2018-02-07T10:45:33Z

Please review @kjbracey-arm @TeemuKultala @geky @AnttiKauppila @sg- @0xc0170 @mikaleppanen

jarlamsa · 2018-02-07T10:49:09Z

And we have a binary compatibility breakage with UBLOX_EVK_ODIN_W2
https://jenkins-internal.mbed.com/job/ARMmbed/job/mbed-os-cliapp/job/master/8977/artifact/mbed-os-cliapp/UBLOX_EVK_ODIN_W2_armcc_wlan0.log

0xc0170 · 2018-02-07T11:33:55Z

This targets master, not feature-status-callbacks ?

kjbracey · 2018-02-07T11:38:59Z

Yes, the aim was to land the feature branch on master for 5.8. This is doing that in one step, incorporating implement fixes found during testing.

Do you want to do a three-step process - you rebase the feature branch, then PR for fixes to it, then PR to merge feature to master?

I guess now we know we have a compatibility break, going to feature branch first would give Ublox an opportunity to submit a PR for new binary to that feature-branch.

kjbracey · 2018-02-07T11:43:13Z

Possible immediate workaround for ODIN_W2 - temporary hack to NetworkInterface that makes the new methods non-virtual if target is Odin W2?

Will render the feature non-functional on that board for now.

kjbracey

Content looks fine to me - just that 1 query.

I'd be inclined to squash all the original feature branch work to 1 commit rather than including all the review changes on master. Not sure what others think.

kjbracey · 2018-02-07T12:19:12Z

features/nanostack/FEATURE_NANOSTACK/mbed-mesh-api/source/thread_tasklet.c

+            return;
+        } else {
+            memcpy(thread_tasklet_data_ptr->ip, temp_ipv6, 16);
+            if (memcmp(thread_tasklet_data_ptr->ip, thread_tasklet_data_ptr->link_config.mesh_local_ula_prefix, 8) == 0) {


Do you need to re-read the mesh_local_ula prefix too? Is that only set on the initial connect, or is it being updated?

Yes, correct, that isn't updated, so need to read and update from the thread_management_configuration. I'll make a commit, I tested it already.

0xc0170 · 2018-02-07T12:40:21Z

I'd be inclined to squash all the original feature branch work to 1 commit rather than including all the review changes on master. Not sure what others think.

History clean up would be good (if its one commit or multiple but concise and clear) 👍

Will render the feature non-functional on that board for now.

We should avoid this. @jarlamsa How to make it compatible with odin?

cc @ARMmbed/team-ublox

jarlamsa · 2018-02-07T12:52:30Z

How to make it compatible with odin?

Ideal scenario would be that ublox would recompile their driver against this PR and push their recompiled binary to this PR
On our own, I don't think we can make this feature compatible with odin since there are API changes and the driver is currently compiled against old definitions.

andreaslarssonublox · 2018-02-07T15:55:58Z

Hi, we will update the driver and hopefully have something tomorrow if everything works as expected.

0xc0170 · 2018-02-08T15:27:47Z

Please rebase (mesh was updated recently)

andreaslarssonublox · 2018-02-09T01:07:07Z

Hi, I have created a new driver release but could not create a PR/merge towards the jarlamsa:status-callbacks branch so maybe you can merge in the binaries instead?
(Another way might be that I make a PR towards mbed-os master but then there will be problems if this branch is rebased I guess since my release branch is based on top of this?)

Driver release v2.4 rc1:
https://github.com/andreaslarssonublox/mbed/tree/ublox_odin_driver_os_5_v2.4_rc1

The driver release is also dependent on this fix otherwise the TCP connection might hard fault:
https://github.com/andreaslarssonublox/mbed/tree/ublox_fix_missing_init_free_lwip_md5

Note that the current wifi tests are not working for ODIN therefore they were not done. However internal u-blox tests have been run and the wifi example. This will be fixed in a later release.

jarlamsa · 2018-02-09T07:07:28Z

@andreaslarssonublox do you mean these commits?
andreaslarssonublox@0b05180
andreaslarssonublox@c2d478e

Note that the current wifi tests are not working for ODIN therefore they were not done.

What do you mean by this? What is breaking them?

andreaslarssonublox · 2018-02-09T07:49:51Z

@jarlamsa Yes, those are the commits.

Yes, the tests-network-wifi test cases were not run since we have not fixed some minor issues to get them to pass. However there is no change in behavior since the version 2.3 of the driver.

This is the original content of feature-status-callbacks, reviewed in ARMmbed#5457

ARMmbed#6003 (comment)

0xc0170 · 2018-02-09T11:14:05Z

/morph build

mbed-ci · 2018-02-09T12:59:15Z

Build : SUCCESS

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

Triggering tests

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

pan- · 2018-02-09T13:19:44Z

features/netsocket/NetworkInterface.h

+     *
+     *  @param status_cb The callback for status changes
+     */
+    virtual void attach(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);


Could you document what are the parameters of the callback in input ?

From the signature it looks like it may handle more than just status change otherwise why not just take in a nsapi_connection_status_t as the callback parameter ?

Some explanation is in nsapi_types.h, with nsapi_event_t. Is that clear enough that you understand it? (I know what it means but that may be because I was in the review meetings).

Agree it should be summarised here.

Is that clear enough that you understand it?

Looking at the documentation and signature; what is unclear for me is whether this status_cb will solely handle status change events or if it will receive other kind of events if nsapi_event_t is updated.

If the parameter of the attach function is not destined to handle events other than connection status change events then the interface is overly complex.

If the callback attached handles more than status events in the future then it is a breaking change: the contract with application code stated in comments would have been broken.

Would this suffice for the documentation:

/** Register callback for status reporting * * The specified status callback function will be called on status changes * on the network. The parameters on the callback are the event type and * event-type dependent reason parameter. * * @param status_cb The callback for status changes */ virtual void attach(mbed::Callback<void(nsapi_event_t, intptr_t)> status_cb);

The intent is that new events will be added to nsapi_event_t, and users would be expected to ignore event types they didn't know about. That should be documented. There is one new event type already being plotted.

The use of an enum for a set that may be extended is a little iffy; it seems to be a C++-ism to use them instead of #defines, despite that. Precedent has already been set by nsapi_error and nsapi_socket_option_t, which are gradually being extended.

@pan- Are you happy with the proposed change in my comment or does it need some refinement?

Looks good (better than previous version) to me

mbed-ci · 2018-02-09T13:44:35Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-02-09T15:06:54Z

Test : SUCCESS

Build number : 908
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6032/908

0xc0170 · 2018-02-12T09:34:48Z

/morph build

0xc0170 · 2018-02-12T09:38:18Z

@AnotherButler New API

mbed-ci · 2018-02-12T10:11:52Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-02-12T11:26:54Z

Test : SUCCESS

Build number : 916
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6032/916

mbed-ci · 2018-02-12T11:34:04Z

Exporter Build : FAILURE

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

0xc0170 · 2018-02-12T11:59:40Z

/morph export-build

mbed-ci · 2018-02-12T15:47:59Z

Exporter Build : SUCCESS

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

cmonr · 2018-02-12T16:34:20Z

@jarlamsa Does this PR effectively replace the ARMmbed:feature-status-callbacks branch? Also, will this PR need to be squashed before merging in?

jarlamsa · 2018-02-13T07:35:55Z

@cmonr Yes, this contains all the original changes from ARMmbed:feature-status-callbacks branch. Not sure about squashing though, 2 cherry-picked commits are coming from ublox, and I would like those to be separate commits, 2 commits on top of that.

And if you refer to the squashing discussion earlier, that has already been done.

kjbracey · 2018-02-15T11:00:20Z

@VeijoPesonen is preparing handbook documentation - please link that PR to here when created.

VeijoPesonen · 2018-02-23T11:02:45Z

ARMmbed/mbed-os-5-docs#416

0xc0170 added the needs: review label Feb 7, 2018

0xc0170 requested review from sg-, kjbracey, AnttiKauppila, mikaleppanen and a user February 7, 2018 11:35

mikaleppanen approved these changes Feb 7, 2018

View reviewed changes

kjbracey approved these changes Feb 7, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Feb 7, 2018

0xc0170 added the release-version: 5.8.0-rc1 label Feb 8, 2018

Teemu Kultala and others added 2 commits February 9, 2018 12:44

NSAPI status callback changes

5a74826

This is the original content of feature-status-callbacks, reviewed in ARMmbed#5457

Add support for error cases for lwip and nanostack

282f3c9

jarlamsa force-pushed the status-callbacks branch from 3c492c1 to 282f3c9 Compare February 9, 2018 10:45

Andreas Larsson and others added 2 commits February 9, 2018 12:54

Added u-blox ODIN-W2 driver binaries v2.4 rc1

67772a5

Added missing lwip_md5_init and lwip_md5_free as suggested by:

34099ba

ARMmbed#6003 (comment)

pan- reviewed Feb 9, 2018

View reviewed changes

Add some documentation for the callback

8bea5ef

0xc0170 added needs: CI and removed needs: work labels Feb 12, 2018

0xc0170 approved these changes Feb 12, 2018

View reviewed changes

cmonr added needs: review and removed needs: CI labels Feb 12, 2018

cmonr added ready for merge and removed needs: review labels Feb 13, 2018

kjbracey merged commit 984cc1a into ARMmbed:master Feb 15, 2018

sg- removed the ready for merge label Feb 15, 2018

juhoeskeli mentioned this pull request Feb 15, 2018

Mbed OS cellular connectivity #6082

Merged

juhaylinen mentioned this pull request Feb 19, 2018

mbed-os-example-wifi hangs on ODIN-W2 #6003

Closed

0xc0170 mentioned this pull request Feb 19, 2018

Connection status cb #6134

Closed

2 tasks

RobMeades mentioned this pull request Feb 21, 2018

Cellular Generic Driver unit tests fail #6150

Closed

juhaylinen mentioned this pull request Apr 13, 2018

Hardware acceleration is not functional #6545

Closed

Status callbacks #6032

Status callbacks #6032

Conversation

jarlamsa commented Feb 7, 2018

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

kjbracey commented Feb 7, 2018

jarlamsa commented Feb 7, 2018

jarlamsa commented Feb 7, 2018

0xc0170 commented Feb 7, 2018

kjbracey commented Feb 7, 2018

kjbracey commented Feb 7, 2018

kjbracey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Feb 7, 2018

jarlamsa commented Feb 7, 2018 • edited Loading

andreaslarssonublox commented Feb 7, 2018

0xc0170 commented Feb 8, 2018

andreaslarssonublox commented Feb 9, 2018

jarlamsa commented Feb 9, 2018

andreaslarssonublox commented Feb 9, 2018

0xc0170 commented Feb 9, 2018

mbed-ci commented Feb 9, 2018

Triggering tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

mbed-ci commented Feb 9, 2018

mbed-ci commented Feb 9, 2018

0xc0170 commented Feb 12, 2018

0xc0170 commented Feb 12, 2018

mbed-ci commented Feb 12, 2018

Triggering tests

mbed-ci commented Feb 12, 2018

mbed-ci commented Feb 12, 2018

0xc0170 commented Feb 12, 2018

mbed-ci commented Feb 12, 2018

cmonr commented Feb 12, 2018

jarlamsa commented Feb 13, 2018 • edited Loading

kjbracey commented Feb 15, 2018

VeijoPesonen commented Feb 23, 2018

jarlamsa commented Feb 7, 2018 •

edited

Loading

0xc0170 Feb 12, 2018 •

edited

Loading

jarlamsa commented Feb 13, 2018 •

edited

Loading