Added new configuration parameters to Wi-SUN network interface and Border Router class #12657

mikaleppanen · 2020-03-19T16:20:13Z

Summary of changes

Added get/validate/set functions for following Wi-SUN mesh interface configuration options:

- Network name
- Regulatory domain
- Network size
- FHSS channel mask
- FHSS unicast channel function related parameters
- FHSS broadcast channel function related parameters
- Timing parameters

Moved Wi-SUN .json configuration parsing from wisun tasklet to Wi-SUN mesh interface class.

Added Wi-SUN Border Router class. Class can be used to start and stop Wi-SUN Border Router.

Border Router class supports following configuration options:

- RPL DIO trickle parameters
- PAN configuration

Added support of network interface name method to Nanostack Mesh, EMAC and PPP interfaces.

NOTE: this pull request depends on nanostack interfaces that will be released on the next 
nanostack release and can be merged only after the release has been made.

Impact of changes

None

Migration actions required

None

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

@mikter @artokin @juhhei01 @JarkkoPaso

ciarmcom · 2020-03-19T18:00:34Z

@mikaleppanen, thank you for your changes.
@juhhei01 @artokin @mikter @JarkkoPaso @ARMmbed/mbed-os-maintainers please review.

mikter · 2020-03-23T07:52:28Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     * \return MESH_ERROR_NONE on success.
+     * \return MESH_ERROR_UNKNOWN in case of failure.
+     * */
+    mesh_error_t set_fhss_broadcast_channel_function(uint8_t channel_function, uint16_t fixed_channel = 0xffff, uint8_t dwell_interval = 0x00, uint32_t broadcast_interval = 0x00);


FHSS is not needed in the name

mikter · 2020-03-23T07:54:11Z

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

+mesh_error_t WisunBorderRouter::start(NetworkInterface *mesh_if, NetworkInterface *backbone_if)
+{
+    InterfaceNanostack *nano_mesh_if = reinterpret_cast<InterfaceNanostack *>(mesh_if);
+    int8_t mesh_if_id = nano_mesh_if->get_interface_id();


Is there null check needed it could be in if?

added null checks

mikter · 2020-03-23T07:55:44Z

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

+
+mesh_error_t WisunBorderRouter::stop()
+{
+    if (_mesh_if_id < 0) {


Is there really a need to return error on unknown usually pointless when you are shutting down

removed return parameter

mikter · 2020-03-23T07:59:13Z

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

+#if (MBED_CONF_MBED_MESH_API_WISUN_REGULATORY_DOMAIN != 255) || (MBED_CONF_MBED_MESH_API_WISUN_OPERATING_CLASS != 255) || (MBED_CONF_MBED_MESH_API_WISUN_OPERATING_MODE != 255)
+    status = set_network_regulatory_domain(MBED_CONF_MBED_MESH_API_WISUN_REGULATORY_DOMAIN,
+                                           MBED_CONF_MBED_MESH_API_WISUN_OPERATING_CLASS,
+                                           MBED_CONF_MBED_MESH_API_WISUN_OPERATING_MODE);


This does not look like you are able to call changes on these parameters before calling connect

It was previously collected in structure at creation and could be modified by user calling functions and then before interface up these were called

This configuration is made on network interface init. So after that and before connect() is called, it is possible to change the settings similarly as previously.

There is no need for settings cache on application side, since on the new configuration system, the settings can be programmed directly to Wi-SUN stack after the init.

artokin · 2020-03-24T07:38:56Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunBorderRouter.h

+    /**
+     * \brief Validate Wi-SUN RPL DIO trickle parameters.
+     *
+     * Function validates DIO trickle timer Imin, DIO trickle timer Imax and DIO trickle timer redundancy


Would you please describe when and why validate function should be used?

added brief description

artokin · 2020-03-24T07:43:19Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     *
+     * Function reads network name from mbed-mesh-api.
+     *
+     * \param network_name Network name as NUL terminated string. Must have space for 32 characters and null terminator (in total 33 characters).


Could be shorter like Must have space for 33 characters.

artokin · 2020-03-24T07:49:02Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     * When network size is changed, and if timing or RPL values should be other than defaults set by stack for the network size,
+     * they need to set again using above function calls.
+     *
+     * \param network_size Values defined in Wi-SUN management API. Automatic 0x00, small 0x01, medium 0x08, large 0x10 and certificate 0xff.


This is error prone if values are changed in the future.
Could we use NETWORK_SIZE_XX constants from ws_management_api.h? Or should WiSUN-Interface define its own constants and map them to NETWORK_SIZE before calling the nanostack api?

There is already today mismatch as Certificate 0 and automatic is 0xff.

artokin · 2020-03-24T07:50:15Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     *
+     * Function reads network size from mbed-mesh-api.
+     *
+     * \param network_size Values defined in Wi-SUN management API. Automatic 0x00, small 0x01, medium 0x08, large 0x10 and certificate 0xff.


Same as above, can we use NETWORK_SIZE constants?

artokin · 2020-03-24T07:50:35Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     *
+     * Function validates network size from mbed-mesh-api.
+     *
+     * \param network_size Values defined in Wi-SUN management API. Automatic 0x00, small 0x01, medium 0x08, large 0x10 and certificate 0xff.


Same as above

artokin · 2020-03-24T07:53:10Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     *
+     * Function overwrites parameters defined by Mbed OS configuration.
+     *
+     * \param channel_function Values defined in Wi-SUN management API. Fixed 0x00, TR51CF 0x01, DH1CF 0x02 and vendor defined 0x03.


Using hardcoded constants, can we use CHANNEL_FUNCTION_ somehow?

artokin · 2020-03-24T08:13:14Z

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

+        return MESH_ERROR_UNKNOWN;
+    }
+
+    int backbone_if_id = atoi(&backbone_if_name[3]);


Can get_interface_name be renamed and extended to return also if_id? Or maybe separate method for it?

Interface id is a nanostack specific feature, so I think we should not add high level NetworkInterface.h (or EMACInterface.h) methods for that, if we can avoid it.

We might need at some point to change for backhaul from NetworkInterface::get_default_instance() to more specific nanostack backhaul interface constructors, but a the moment the default instance is working as it is.

mikaleppanen · 2020-03-25T10:02:03Z

@amarjeet-arm @gkumaar @debdeep-arm @prashant-arm @umesh-arm Here is the configuration interface pull request to mbed-os

mikaleppanen · 2020-03-30T12:25:17Z

@mikter @artokin corrected based on review comments.

mikaleppanen · 2020-03-30T12:41:57Z

corrected coding style

mikter · 2020-03-31T07:10:53Z

features/nanostack/mbed-mesh-api/mbed-mesh-api/WisunInterface.h

+     * Medium network size: hundreds of devices
+     * Large network size: thousands of devices
+     * Automatic: when discovering the network, network size is learned from advertisements and timings adjusted accordingly
+     * Certificate: used on testing


I don't think we should limit the network size in this interface by enum it could be from 0 to 20 or something
Then just assign values for those points that we have inside stack
Otherwise changing this later on becomes really hard

Also Automatic is not valid option and should not be here
0 == 25 less
1 == 100
2== 200
10 == 1000
20 == 10000

Nanostack interface has these fixed values (and only those are supported):
#define NETWORK_SIZE_CERTIFICATE 0x00
#define NETWORK_SIZE_SMALL 0x01
#define NETWORK_SIZE_MEDIUM 0x08
#define NETWORK_SIZE_LARGE 0x10
#define NETWORK_SIZE_AUTOMATIC 0xFF
so will I add a conversion to mesh api so that 0 --> 6 means small, 7 - 12 medium, 13 to 18 large etc...

Changed network size to hundreds of devices, removed defines.

I don't like that automatic. It was causing more problems at nano stack side.

mikaleppanen · 2020-03-31T14:07:30Z

updated based on comments

mikter · 2020-04-01T05:27:06Z

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

+
+mesh_error_t WisunBorderRouter::start(NetworkInterface *mesh_if, OnboardNetworkStack::Interface *backbone_if)
+{
+    InterfaceNanostack *nano_mesh_if = reinterpret_cast<InterfaceNanostack *>(mesh_if);


Do you need null checks here?

juhhei01

Only thing what I would like to remove is that network size automatic. It is nice idea but not a good in practise.

0xc0170 · 2020-04-03T13:23:50Z

@mikaleppanen Can you review travis failures to compile events?

0xc0170 · 2020-04-14T14:34:42Z

Any update for this PR?

mergify · 2020-04-15T12:56:38Z

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

Pull request has been modified.

mikaleppanen · 2020-04-27T06:39:38Z

New version of nanostack is now on feature-wisun branch. Rebased commits.

mikaleppanen · 2020-04-28T12:42:07Z

@0xc0170 can this proceed?

0xc0170 · 2020-04-28T13:09:45Z

Set for CI, will schedule

0xc0170 · 2020-04-29T09:16:00Z

CI started

mbed-ci · 2020-04-29T10:20:21Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_wisun-mesh-test-lts

0xc0170 · 2020-04-29T11:44:03Z

wisun restarted, all fine now

0xc0170 · 2020-04-29T11:44:19Z

@artokin merge if its ready

mergify bot added release-version: feature branch needs: work labels Mar 19, 2020

ciarmcom requested review from artokin, JarkkoPaso, juhhei01, mikter and a team March 19, 2020 18:00

ciarmcom added the needs: review label Mar 19, 2020

mikter reviewed Mar 23, 2020

View reviewed changes

artokin reviewed Mar 24, 2020

View reviewed changes

adbridge removed the needs: review label Mar 26, 2020

mikaleppanen requested review from mikter and artokin March 30, 2020 12:23

mikter reviewed Mar 31, 2020

View reviewed changes

mikaleppanen requested a review from mikter March 31, 2020 14:07

mikter reviewed Apr 1, 2020

View reviewed changes

mikter previously approved these changes Apr 1, 2020

View reviewed changes

juhhei01 approved these changes Apr 1, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Apr 3, 2020

0xc0170 removed the needs: CI label Apr 8, 2020

0xc0170 added the needs: work label Apr 8, 2020

Added new configuration parameters to Wi-SUN interface

6219d75

Added Wi-SUN Border Router class

7709193

mikaleppanen requested review from juhhei01 and mikter April 27, 2020 07:31

0xc0170 added needs: CI and removed needs: work labels Apr 28, 2020

0xc0170 added ready for merge and removed needs: CI labels Apr 29, 2020

artokin merged commit 35d28d4 into ARMmbed:feature-wisun Apr 29, 2020

mikaleppanen mentioned this pull request Apr 30, 2020

Add new configuration parameters to Wi-SUN network interface and Border Router class #12898

Merged

0xc0170 removed the ready for merge label May 11, 2020

Added new configuration parameters to Wi-SUN network interface and Border Router class #12657

Added new configuration parameters to Wi-SUN network interface and Border Router class #12657

Uh oh!

Conversation

mikaleppanen commented Mar 19, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Mar 19, 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaleppanen commented Mar 25, 2020

Uh oh!

mikaleppanen commented Mar 30, 2020

Uh oh!

mikaleppanen commented Mar 30, 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!

mikaleppanen commented Mar 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juhhei01 left a comment

Choose a reason for hiding this comment

Uh oh!