Skip to content
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

Mesh API: Get and validate functions for PHY mode and channel plan IDs #14251

Merged
merged 1 commit into from Mar 22, 2021
Merged

Mesh API: Get and validate functions for PHY mode and channel plan IDs #14251

merged 1 commit into from Mar 22, 2021

Conversation

JarkkoPaso
Copy link

@JarkkoPaso JarkkoPaso commented Feb 9, 2021 •
edited by artokin

Summary of changes

Implemented domain configuration functions in mesh API:

  • Functions will set, get and validate the parameters using corresponding Nanostack API.

Note! This change requires new Nanostack version: #14327

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 @mikaleppanen @artokin @juhhei01

@ciarmcom
Copy link
Member

ciarmcom commented Feb 9, 2021

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

mikter
mikter reviewed Feb 9, 2021
View reviewed changes
features/nanostack/mbed-mesh-api/source/WisunInterface.cpp Outdated

mesh_error_t WisunInterface::validate_network_phy_mode_and_channel_plan_id(uint8_t phy_mode_id, uint8_t channel_plan_id)
{
int status = ws_management_phy_mode_id_validate(get_interface_id(), phy_mode_id);
Copy link

Choose a reason for hiding this comment

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

Did we make separate validates for mode and plan id. this sounds problematic as there is probably some mode, plan combinations we don't support.
supported
mode1,plan1
mode2,plan1
mode2,plan2
but not supported
mode2,plan2
we can't set or validate this proberly do we need to change the underlying API?

Copy link
Author

Choose a reason for hiding this comment

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

I see your point. Yes, the validation on Nanostack API is separated. We can keep this PR on hold and I can change the Nanostack API.

juhhei01
juhhei01 reviewed Feb 11, 2021
View reviewed changes
features/nanostack/mbed-mesh-api/source/WisunInterface.cpp

mesh_error_t WisunInterface::validate_network_domain_configuration(uint8_t regulatory_domain, uint8_t phy_mode_id, uint8_t channel_plan_id)
{
int status = ws_management_domain_configuration_validate(get_interface_id(), regulatory_domain, phy_mode_id, channel_plan_id);
if (status != 0) {
return MESH_ERROR_UNKNOWN;
Copy link
Contributor

Choose a reason for hiding this comment

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

we not have a better error than unknot available?

Copy link
Author

Choose a reason for hiding this comment

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

Actually we have these error types in mesh API:
MESH_ERROR_NONE
MESH_ERROR_UNKNOWN
MESH_ERROR_MEMORY
MESH_ERROR_STATE
MESH_ERROR_PARAM

But Nanostack API documentation (Doxygen) doesn't tell the reason for error. It only says: "return <0 Fail" for every WS management function.

We should probably first check that error values are correctly set and documented in Nanostack API.

Copy link
Contributor

Choose a reason for hiding this comment

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

MESH_ERROR_PARAM may be good select for indicate proper information

mikter
mikter previously approved these changes Feb 12, 2021
View reviewed changes
artokin
artokin previously approved these changes Feb 16, 2021
View reviewed changes
0xc0170
0xc0170 previously approved these changes Feb 16, 2021
View reviewed changes
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @JarkkoPaso, please carry out any necessary work to get the changes merged. Thank you for your contributions.

@ciarmcom ciarmcom added the stale Stale Pull Request label Feb 21, 2021
@adbridge adbridge added needs: CI and removed needs: work stale Stale Pull Request labels Mar 10, 2021
@mergify mergify bot dismissed stale reviews from mikter, artokin, and 0xc0170 March 11, 2021 09:26

Pull request has been modified.

@mbed-ci
Copy link

mbed-ci commented Mar 16, 2021

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_example-test-lts

@mbed-ci
Copy link

mbed-ci commented Mar 17, 2021

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 37957e7 into ARMmbed:feature-wisun Mar 22, 2021
@artokin artokin mentioned this pull request Apr 20, 2021
Merged
@artokin artokin mentioned this pull request May 6, 2021
Merged
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

@artokin artokin artokin approved these changes

@0xc0170 0xc0170 0xc0170 left review comments

@mikaleppanen mikaleppanen Awaiting requested review from mikaleppanen

@juhhei01 juhhei01 Awaiting requested review from juhhei01

Assignees
No one assigned
Labels
release-version: feature branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@JarkkoPaso @ciarmcom @mbed-ci @0xc0170 @juhhei01 @mikter @artokin @adbridge