modified Wi-SUN tasklet to return success when calling connect twice #11556

mikter · 2019-09-24T06:34:14Z

Description

If connect to be called multiple times it will return NSAPI_ERROR_IS_CONNECTED status.

Pull request type

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

Reviewers

@JarkkoPaso @artokin @SeppoTakalo @AnttiKauppila @teetak01

Release Notes

Summary of changes

As stated in Mesh API documentation, we should return the status NSAPI_ERROR_IS_CONNECTED if connect is called twice. This PR (11556) fixes it.

Impact of changes

If connect method is called twice then different status code is returned.

Migration actions required

Implementations that rely on NSAPI_ERROR_DEVICE_ERROR status code when calling connect twice as their recovery method needs to change the handling to the correct status NSAPI_ERROR_IS_CONNECTED.

ciarmcom · 2019-09-24T07:00:24Z

@mikter, thank you for your changes.
@JarkkoPaso @teetak01 @AnttiKauppila @SeppoTakalo @artokin @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

artokin · 2019-09-24T07:22:22Z

features/nanostack/mbed-mesh-api/source/LoWPANNDInterface.cpp

@@ -171,7 +171,7 @@ bool LoWPANNDInterface::getRouterIpAddress(char *address, int8_t len)
 }

 #define LOWPAN 0x2345
-#if MBED_CONF_NSAPI_DEFAULT_MESH_TYPE == LOWPAN && DEVICE_802_15_4_PHY


Why 802_15_4_PHY is not requires for mesh anymore?

artokin · 2019-09-24T07:23:28Z

features/nanostack/mbed-mesh-api/source/wisun_tasklet.c

@@ -133,6 +133,7 @@ static void wisun_tasklet_main(arm_event_s *event)
             * The event is delivered when the NanoStack OS is running fine.
             * This event should be delivered ONLY ONCE.
             */
+            tr_debug("event_Init");


too detailed trace

artokin · 2019-09-24T07:26:26Z

features/nanostack/mbed-mesh-api/source/wisun_tasklet.c

@@ -434,7 +436,7 @@ int8_t wisun_tasklet_connect(mesh_interface_cb callback, int8_t nwk_interface_id
    int8_t tasklet_id = wisun_tasklet_data_ptr->tasklet;

    if (wisun_tasklet_data_ptr->network_interface_id != INVALID_INTERFACE_ID) {
-        return -3;  // already connected to network
+        return wisun_tasklet_data_ptr->tasklet;  // already connected to network


Is this change needed also to other mesh tasklets?

SeppoTakalo · 2019-09-24T08:58:04Z

If usage of DEVICE_802_15_4_PHY flag is now removed, should you remove that from all targets as well? There are still few boards defining it in targets.json

mikter · 2019-09-24T09:11:19Z

If usage of DEVICE_802_15_4_PHY flag is now removed, should you remove that from all targets as well? There are still few boards defining it in targets.json

My understanding was that this flag is only set if the board has 802 radio included. but mesh apis needs to be available if it is used with radio shield. So the && configuration was wrong

0xc0170 · 2019-09-24T11:17:26Z

started CI meanwhile

SeppoTakalo · 2019-09-24T11:23:22Z

If usage of DEVICE_802_15_4_PHY flag is now removed, should you remove that from all targets as well? There are still few boards defining it in targets.json

My understanding was that this flag is only set if the board has 802 radio included. but mesh apis needs to be available if it is used with radio shield. So the && configuration was wrong

True, but is there any use of the DEVICE_802_15_4 flag anymore? Only reference is in TEST_APPS/device/nanostack_mac_tester/main.cpp, but it is already flagged out with ICETEA_MAC_TESTER_ENABLED.

mbed-ci · 2019-09-24T12:55:11Z

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_mbed2-build-IAR
jenkins-ci/mbed-os-ci_cloud-client-test

SeppoTakalo · 2019-09-24T13:15:08Z

-11 again from Python.

0xc0170 · 2019-09-24T14:28:51Z

I restarted both, all good now!

0xc0170 · 2019-09-24T14:35:41Z

features/netsocket/mbed_lib.json

@@ -40,7 +40,7 @@
        },
        "default-mesh-type": {
            "help": "Configuration type for MeshInterface::get_default_instance(). [LOWPAN/THREAD/WISUN]",
-            "value": "THREAD"
+            "value": null


one question before integrating this: isn't this breaking change for apps - as the config value changes for default mesh type ?

As discussed, these changes are potentially breaking, adding proper labels

Questionable..
Is it a breaking change to return no default, instead of non-configured non-working default?

Or @mikter did the non-configured Thread work? My assumption is that it is a demo, with no real use cases.. should we optimise for testing and demo, or for real use cases.

I don't think anyone should have Thread configured without any configuration in application

It would however work as a non-commissioned Thread device, but that should still be application configuration not a default.

I can remove this change if you like from this commit as it is not really related. although the Breaking change is not only because of this configuration, but it is because we are changing status code on public API.

0xc0170 · 2019-09-30T14:08:53Z

CI restarted

0xc0170 · 2019-09-30T14:10:05Z

@AnttiKauppila Happy with changes and Release notes?

mbed-ci · 2019-09-30T16:27:44Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

bulislaw · 2019-10-02T09:34:18Z

Hold on here please, I'll start an email thread to confirm it.

juhhei01 · 2019-10-02T09:45:00Z

@mikter Are you sure that next sequence will work:

Connect and wait success status
Disconnect connection
reconnect by connect call. I think This is not working anymore because of removing stuff from original changes.

You demonstrate this and I see it was working earlier.

I think double connect is now supported but not mine sequence.

juhhei01 · 2019-10-02T10:36:27Z

@bulislaw , @adbridge and @0xc0170 I have not following this in early but we now loosing a bug fix for sequence that I was telling to Mika. That was working but not anymore.

adbridge · 2019-10-02T10:39:28Z

@juhhei01 Mika was supposed to just split the original PR into 2 separate ones, so presumably there should be another PR he has raised to cover your bug fix ?

juhhei01 · 2019-10-02T10:43:23Z

@adbridge I did not buy idea why you was blocking that fix which was reviewed and aproved by mesh stakeholders. I see that those blocked issues was even critical than current one.

juhhei01 · 2019-10-02T11:09:03Z

features/nanostack/nanostack-interface/Nanostack.cpp

@@ -116,7 +116,7 @@ nsapi_error_t map_mesh_error(mesh_error_t err)
        case MESH_ERROR_PARAM:
            return NSAPI_ERROR_PARAMETER;
        case MESH_ERROR_STATE:
-            return NSAPI_ERROR_DEVICE_ERROR;


Looks like that now we are hiding something bigger issue? If we can return connected state at mesh error state.

Current return value make more sense if we are already connected

bulislaw · 2019-10-02T14:04:44Z

Ok the 5.15 is confirmed, I don't have further comments.

adbridge · 2019-10-03T11:17:27Z

@adbridge I did not buy idea why you was blocking that fix which was reviewed and aproved by mesh stakeholders. I see that those blocked issues was even critical than current one.

@juhhei01 because it broke the PR guidelines. PRs should contain self-contained fixes not a mixture of things.

SeppoTakalo · 2019-10-04T08:52:23Z

@adbridge You are mixing commits and PRs.

https://os.mbed.com/docs/mbed-os/v5.14/contributing/workflow.html#guidelines-for-github-pull-requests

Each commit should be the minimum self-contained commit for a change. A commit should always result in a new state that is again in a compilable state. You should (if possible) split large changes into logical smaller commits that help reviewers follow the reasoning behind the full change.

One PR may contain more that one fix. Nothing wrong with that.

You seem to enforse the last rule in the list:

Smaller pull requests are easier to review and faster to integrate. Use dependencies – split your work by pull request type or functional changes. To add a third-party driver, send it in a separate pull request, and add it as a dependency to your pull request.

However, pay attention to easier to review as this PR was already reviewed. So I fail to see the reason for the split. After clicking the merge button, it does not much matter how many PR was the content split. Its all in the master then.

Having more merge commits just make history harder to read.

adbridge · 2019-10-04T11:07:21Z

@adbridge You are mixing commits and PRs.
https://os.mbed.com/docs/mbed-os/v5.14/contributing/workflow.html#guidelines-for-github-pull-requests

Each commit should be the minimum self-contained commit for a change. A commit should always result in a new state that is again in a compilable state. You should (if possible) split large changes into logical smaller commits that help reviewers follow the reasoning behind the full change.

One PR may contain more that one fix. Nothing wrong with that.
You seem to enforse the last rule in the list:

Smaller pull requests are easier to review and faster to integrate. Use dependencies – split your work by pull request type or functional changes. To add a third-party driver, send it in a separate pull request, and add it as a dependency to your pull request.
Having more merge commits just make history harder to read.

@SeppoTakalo multiple commits for the same fix are acceptable. This PR had two very different changes and in that case it should go to two separate PRs. We are getting more strict on this because too many people are just dumping PRs with multiple unconnected fixes in them.

0xc0170 · 2019-10-18T10:21:10Z

Will start CI soon here, just to confirm @mikter @@juhhei01 all ready for us to proceed with this fix?

mikter · 2019-10-18T10:58:43Z

yes

0xc0170 · 2019-10-18T11:05:50Z

CI started

mbed-ci · 2019-10-18T13:57:59Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 3
Build artifacts

ciarmcom requested review from AnttiKauppila, artokin, JarkkoPaso, SeppoTakalo, teetak01 and a team September 24, 2019 07:00

ciarmcom added the needs: review label Sep 24, 2019

JarkkoPaso approved these changes Sep 24, 2019

View reviewed changes

mikter changed the title ~~modified Wi-SUN tasklet to return success when calling connet twice~~ modified Wi-SUN tasklet to return success when calling connect twice Sep 24, 2019

artokin reviewed Sep 24, 2019

View reviewed changes

mikter force-pushed the master branch from f0a53d2 to bd2c122 Compare September 24, 2019 09:02

artokin approved these changes Sep 24, 2019

View reviewed changes

0xc0170 approved these changes Sep 24, 2019

View reviewed changes

SeppoTakalo approved these changes Sep 24, 2019

View reviewed changes

0xc0170 added ready for merge release-version: 5.14.1 and removed needs: review labels Sep 24, 2019

0xc0170 reviewed Sep 24, 2019

View reviewed changes

0xc0170 added needs: work and removed ready for merge labels Sep 25, 2019

mikter force-pushed the master branch from bd2c122 to 8167e53 Compare September 25, 2019 08:34

0xc0170 approved these changes Sep 30, 2019

View reviewed changes

0xc0170 added the release-version: 5.15.0-rc1 label Sep 30, 2019

0xc0170 added needs: review and removed needs: CI labels Oct 1, 2019

0xc0170 requested a review from bulislaw October 1, 2019 08:03

juhhei01 reviewed Oct 2, 2019

View reviewed changes

adbridge added needs: work and removed needs: review labels Oct 3, 2019

0xc0170 added needs: CI and removed needs: work labels Oct 18, 2019

0xc0170 added ready for merge and removed needs: CI labels Oct 18, 2019

0xc0170 merged commit 9e87200 into ARMmbed:master Oct 18, 2019

0xc0170 removed the ready for merge label Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modified Wi-SUN tasklet to return success when calling connect twice #11556

modified Wi-SUN tasklet to return success when calling connect twice #11556

mikter commented Sep 24, 2019 •

edited by 0xc0170

Loading

ciarmcom commented Sep 24, 2019

artokin Sep 24, 2019

artokin Sep 24, 2019

artokin Sep 24, 2019

SeppoTakalo commented Sep 24, 2019

mikter commented Sep 24, 2019

0xc0170 commented Sep 24, 2019

SeppoTakalo commented Sep 24, 2019

mbed-ci commented Sep 24, 2019

SeppoTakalo commented Sep 24, 2019

0xc0170 commented Sep 24, 2019

0xc0170 Sep 24, 2019

0xc0170 Sep 25, 2019

SeppoTakalo Sep 25, 2019

mikter Sep 26, 2019

0xc0170 commented Sep 30, 2019

0xc0170 commented Sep 30, 2019

mbed-ci commented Sep 30, 2019

bulislaw commented Oct 2, 2019

juhhei01 commented Oct 2, 2019 •

edited

Loading

juhhei01 commented Oct 2, 2019 •

edited

Loading

adbridge commented Oct 2, 2019

juhhei01 commented Oct 2, 2019

juhhei01 Oct 2, 2019 •

edited

Loading

juhhei01 Oct 2, 2019

bulislaw commented Oct 2, 2019

adbridge commented Oct 3, 2019

SeppoTakalo commented Oct 4, 2019

adbridge commented Oct 4, 2019 •

edited

Loading

0xc0170 commented Oct 18, 2019

mikter commented Oct 18, 2019

0xc0170 commented Oct 18, 2019

mbed-ci commented Oct 18, 2019

modified Wi-SUN tasklet to return success when calling connect twice #11556

modified Wi-SUN tasklet to return success when calling connect twice #11556

Conversation

mikter commented Sep 24, 2019 • edited by 0xc0170 Loading

Description

Pull request type

Reviewers

Release Notes

Summary of changes

Impact of changes

Migration actions required

ciarmcom commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeppoTakalo commented Sep 24, 2019

mikter commented Sep 24, 2019

0xc0170 commented Sep 24, 2019

SeppoTakalo commented Sep 24, 2019

mbed-ci commented Sep 24, 2019

SeppoTakalo commented Sep 24, 2019

0xc0170 commented Sep 24, 2019

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 commented Sep 30, 2019

0xc0170 commented Sep 30, 2019

mbed-ci commented Sep 30, 2019

bulislaw commented Oct 2, 2019

juhhei01 commented Oct 2, 2019 • edited Loading

juhhei01 commented Oct 2, 2019 • edited Loading

adbridge commented Oct 2, 2019

juhhei01 commented Oct 2, 2019

juhhei01 Oct 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw commented Oct 2, 2019

adbridge commented Oct 3, 2019

SeppoTakalo commented Oct 4, 2019

adbridge commented Oct 4, 2019 • edited Loading

0xc0170 commented Oct 18, 2019

mikter commented Oct 18, 2019

0xc0170 commented Oct 18, 2019

mbed-ci commented Oct 18, 2019

mikter commented Sep 24, 2019 •

edited by 0xc0170

Loading

juhhei01 commented Oct 2, 2019 •

edited

Loading

juhhei01 commented Oct 2, 2019 •

edited

Loading

juhhei01 Oct 2, 2019 •

edited

Loading

adbridge commented Oct 4, 2019 •

edited

Loading