Skip to content

Nanostack: Update Wi-SUN configuration #12349

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 10, 2020
Merged

Nanostack: Update Wi-SUN configuration #12349

merged 1 commit into from
Feb 10, 2020

Conversation

artokin
Copy link
Contributor

@artokin artokin commented Jan 31, 2020

Summary of changes

-Add more details to Wi-SUN configuration help texts
-Add min/max value checks for some Wi-SUN configuration values
-Define default values for some Wi-SUN parameters instead of referring to Nanostack internal default values.

Impact of changes

Migration actions required

Documentation

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)
[] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-mesh

@artokin artokin requested a review from korjaa January 31, 2020 13:05
@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested review from a team January 31, 2020 14:00
michalpasztamobica
michalpasztamobica reviewed Jan 31, 2020
View reviewed changes
features/nanostack/mbed-mesh-api/mbed_lib.json Outdated
},
"wisun-bc-interval": {
"help": "Broadcast interval. Duration between broadcast dwell intervals. Range: 0-16777216 milliseconds",
"help": "Broadcast interval. Duration between broadcast dwell intervals. Range: 0-16777216 milliseconds, keep as 0 to learn the value from network.",
"value_max": 16777216,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an expert here, just making sure this is correct...
16777216 is 0x100 0000. Should this by any chance be 16777215 (0xFF FFFF)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalpasztamobica , good catch. Actually the broadcast interval length can be 4 octets but the resolution might be limited. Setting the length as 32-bit for now.

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 4, 2020
mikter
mikter reviewed Feb 4, 2020
View reviewed changes
features/nanostack/mbed-mesh-api/mbed_lib.json Outdated
},
"wisun-bc-interval": {
"help": "Broadcast interval. Duration between broadcast dwell intervals. Range: 0-16777216 milliseconds",
"help": "Broadcast interval. Duration between broadcast dwell intervals. 32-bit, keep as 0 to learn the value from network.",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BC interval and dwell is always learned by nodes and always set to value for border router. So when this value is set valid value should be used if this configuration is in future used by border router sw.

dwell:250, interval:1050 are the defaults if I remember correctly

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 4, 2020

I run Ci meanwhile

@mbed-ci
Copy link

mbed-ci commented Feb 4, 2020

Test run: SUCCESS

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

mikter
mikter previously approved these changes Feb 5, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Feb 5, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 5, 2020

CI started

@mergify mergify bot added needs: work and removed needs: CI labels Feb 5, 2020
@mbed-ci
Copy link

mbed-ci commented Feb 5, 2020

Test run: FAILED

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

Failed test jobs:

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

Update Wi-SUN configuration
5836b7e 
-Improve help texts of Wi-SUN configuration values
-Add min/max value checks to Wi-SUN configuration values
-Define default values for some parameters, instead of referring to
 Nanostack internal default values.
@artokin artokin force-pushed the wisun_configuration_update branch from ffebccd to 5836b7e Compare February 5, 2020 13:02
@mergify mergify bot dismissed mikter’s stale review February 5, 2020 13:03

Pull request has been modified.

@artokin
Copy link
Contributor Author

artokin commented Feb 5, 2020

Test failure was caused by the change of bc_interval/bc_dwell_interval default value. Pushed following changes to solve the failing test:

  • Changed bc_interval/bc_dwell_interval values back to the default values
  • Squashed all commits together
  • Rebased on top of master branch

Would you please re-review?

@artokin artokin requested a review from mikter February 5, 2020 13:08
@mergify mergify bot added needs: CI and removed needs: work labels Feb 5, 2020
korjaa
korjaa reviewed Feb 6, 2020
View reviewed changes
features/nanostack/mbed-mesh-api/mbed_lib.json
"help": "Unicast dwell interval. Range: 15-255 milliseconds.",
"value_min": 15,
"value_max": 255,
"value": 255
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11/10 points and a big 👍

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 7, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2020

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 563c116 into ARMmbed:master Feb 10, 2020
@mergify mergify bot removed the ready for merge label Feb 10, 2020
@artokin artokin deleted the wisun_configuration_update branch February 10, 2020 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikter mikter mikter approved these changes

@korjaa korjaa korjaa approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

@mikaleppanen mikaleppanen Awaiting requested review from mikaleppanen

@juhhei01 juhhei01 Awaiting requested review from juhhei01

@JarkkoPaso JarkkoPaso Awaiting requested review from JarkkoPaso

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@artokin @ciarmcom @0xc0170 @mbed-ci @juhhei01 @JarkkoPaso @mikter @korjaa @michalpasztamobica