Skip to content

[commissioner] synchronize the state with Border Agent#4788

Merged
jwhui merged 4 commits intoopenthread:masterfrom
LuDuda:pr/bugfix-commissioner
Apr 9, 2020
Merged

[commissioner] synchronize the state with Border Agent#4788
jwhui merged 4 commits intoopenthread:masterfrom
LuDuda:pr/bugfix-commissioner

Conversation

@LuDuda
Copy link
Copy Markdown
Member

@LuDuda LuDuda commented Apr 3, 2020

This PR fixes the bug found during the Commissioning procedure when i wasn't able to turn on native commissioner even though the actual state was Disabled.


In case both Commissioner and Border Agent are compiled in, there is a chance that Commissioner will change the state without user intervention e.g. from Active to Disabled (due to some network issue e.g. Keep Alives are lost) without calling the OpenThread API.

If that happens, it may not be possible to turn on/off the commissioner again. There are a couple of possible problems but just to give perspective:


This PR introduces a few improvements:

To make architectural more cleaner:

  • Move the Border Agent state handling from Mle to Border Agent module (by using notifier).
  • Remove connection between Border Agent and Commissioner in the commissioner_api.cpp.
  • Return OT_ERROR_ALREADY instead of OT_ERROR_INVALID_STATE when applicable in Commissioner API.

To fix the above problem:

  • Ensure Border Agent is stopped before starting the commissioner, and properly handle OT_ERROR_ALREADY error.
  • Inform the Border Agent module about the change of the commissioner state (by introducing new notifier entry).

@size-report
Copy link
Copy Markdown

size-report bot commented Apr 3, 2020

Size Report of OpenThread

Merging #4788 into master(8d5fc20).

name branch text data bss total
ot-cli-ftd master 391448 732 72716 464896
#4788 391656 732 72724 465112
+/- +208 0 +8 +216
ot-cli-mtd master 315176 732 64100 380008
#4788 315240 732 64108 380080
+/- +64 0 +8 +72
ot-ncp-ftd master 386680 732 72012 459424
#4788 386888 732 72020 459640
+/- +208 0 +8 +216
ot-ncp-mtd master 313592 732 63396 377720
#4788 313624 732 63404 377760
+/- +32 0 +8 +40
ot-rcp master 62720 636 7456 70812
#4788 62720 636 7456 70812
+/- 0 0 0 0
libopenthread-cli-ftd.a master 36969 0 3516 40485
#4788 36969 0 3516 40485
+/- 0 0 0 0
libopenthread-cli-mtd.a master 29474 0 3516 32990
#4788 29474 0 3516 32990
+/- 0 0 0 0
libopenthread-ftd.a master 190876 0 55018 245894
#4788 191114 0 55026 246140
+/- +238 0 +8 +246
libopenthread-mtd.a master 127370 0 46402 173772
#4788 127422 0 46410 173832
+/- +52 0 +8 +60
libopenthread-ncp-ftd.a master 47273 0 2388 49661
#4788 47273 0 2388 49661
+/- 0 0 0 0
libopenthread-ncp-mtd.a master 42693 0 2388 45081
#4788 42693 0 2388 45081
+/- 0 0 0 0
libopenthread-rcp.a master 26705 0 1532 28237
#4788 26705 0 1532 28237
+/- 0 0 0 0
libopenthread-radio.a master 10587 0 94 10681
#4788 10587 0 94 10681
+/- 0 0 0 0

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2020

Codecov Report

Merging #4788 into master will increase coverage by 0.00%.
The diff coverage is 70.83%.

@@           Coverage Diff           @@
##           master    #4788   +/-   ##
=======================================
  Coverage   75.33%   75.33%           
=======================================
  Files         324      324           
  Lines       40166    40177   +11     
=======================================
+ Hits        30258    30269   +11     
  Misses       9908     9908           
Impacted Files Coverage Δ
src/core/api/commissioner_api.cpp 54.00% <ø> (+0.15%) ⬆️
src/core/common/notifier.cpp 82.75% <ø> (ø)
src/core/meshcop/border_agent.hpp 0.00% <ø> (ø)
src/core/meshcop/commissioner.hpp 75.00% <ø> (ø)
src/core/meshcop/joiner.cpp 83.45% <0.00%> (ø)
src/core/thread/mle.cpp 91.63% <ø> (-0.33%) ⬇️
src/core/meshcop/commissioner.cpp 73.77% <60.00%> (-0.37%) ⬇️
src/core/meshcop/border_agent.cpp 21.86% <100.00%> (+2.60%) ⬆️
src/posix/platform/uart.cpp 44.44% <0.00%> (-28.21%) ⬇️
src/posix/platform/netif.cpp 79.74% <0.00%> (-0.87%) ⬇️
... and 16 more

@wgtdkp wgtdkp requested review from simonlingoogle and wgtdkp April 4, 2020 03:13
@LuDuda LuDuda force-pushed the pr/bugfix-commissioner branch from ed47b54 to ed8209e Compare April 4, 2020 12:33
Copy link
Copy Markdown
Member

@wgtdkp wgtdkp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Copy Markdown
Contributor

@simonlingoogle simonlingoogle left a comment

Choose a reason for hiding this comment

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

LGTM

LuDuda added 4 commits April 8, 2020 23:00
)

This commit fixes situation when Commissioner can't be turned on or off,
due to incorrect state of the Border Agent.

In order to fix it:
 - a new COMMISSIONER state in the notifier module has been introduced
 - Border Agent API is not colled from the commissioner_api.cpp file
@LuDuda LuDuda force-pushed the pr/bugfix-commissioner branch from bf88636 to d3c1e47 Compare April 8, 2020 21:15
@jwhui jwhui requested a review from bukepo April 9, 2020 15:23
Copy link
Copy Markdown
Member

@bukepo bukepo left a comment

Choose a reason for hiding this comment

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

LGTM:+1:

Copy link
Copy Markdown
Member

@jwhui jwhui left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@jwhui jwhui merged commit 2f618bd into openthread:master Apr 9, 2020
jwhui pushed a commit that referenced this pull request Apr 9, 2020
This commit fixes situation when Commissioner can't be turned on or off,
due to incorrect state of the Border Agent.

In order to fix it:
 - a new COMMISSIONER state in the notifier module has been introduced
 - Border Agent API is not colled from the commissioner_api.cpp file
gjc13 pushed a commit to gjc13/openthread that referenced this pull request Aug 12, 2020
Issue(s): ot-sync-41

* origin/github/master:
  [ip6] disable security for link-local packets from unsecure ports (openthread#4782)
  [network-data] simplify ServiceTlv (openthread#4806)
  [joiner] align error logs with other modules (openthread#4788)
  [border-agent] remove the OT_CHANGED_BORDER_AGENT_STATE enumeration (openthread#4788)
  [commissioner] ensure synchronization with Border Agent (openthread#4788)
  [border-agent] use Notifier to update the state (openthread#4788)
  [cmake] add support for cc2652 (openthread#4691)
  [icmp] do not generate errors in response to errors (openthread#4800)
  [icmp] pass full error-causing message when sending error (openthread#4800)
  [mesh-forwarder] move frame-to-message conversion to separate method (openthread#4800)
  [mesh-forwarder] move ICMPv6 Dst Unreach call to mesh-forwarder (openthread#4800)
  [network-data] adopt const for methods, parameters, and local vars (openthread#4802)
  [mbedtls] leverage cmake provided by mbedtls (openthread#4803)
  [network-data-notifier] fix flag for child removed notification (openthread#4805)
  [harness-automation] fix case selection issue when using Chrome 80 (openthread#4648)
  [mle] add DeviceRole type, and IsChild, IsRouter, etc method (openthread#4794)
  [android] export CFLAGS and include directories (openthread#4797)
  [network-data] use GetLocator() when registering network data (openthread#4798)
  [network-data] implement network data notifier (openthread#4783)
  [github-actions] include fuzz checks (openthread#4792)
  [toranj] update test-039 to stagger initial address queries (openthread#4791)
  [coap] avoid div-by-0 error invalidating coap params (openthread#4787)
  [nrf528xx] add defines needed for compilation with IAR in nrf_security (openthread#4790)
  [address-resolver] ensure prev pointer stays valid on entry removal (openthread#4789)
  [network-data] simplify code, user helper methods (openthread#4780)
  [mesh-forwarder] fix bug in evicting from address resolver queue (openthread#4786)
  [jn5189] remove unused files
  [nrf528xx] update nrf security (openthread#4773)
  [network-data] add IncrementVersionAndStableVersion() (openthread#4781)
  [meshcop] generate PSKc from passphrase (openthread#4766)
  [core] use the common `RouterIdSet` class (openthread#4756)
  [network-data] periodically check for stale child ids (openthread#4779)
  [mesh-forwarder] include address resolving queue in message eviction (openthread#4776)
  [efr32] add nvm3 support (openthread#4521)
  [cmake] add support for cc1352 (openthread#4690)
  [toranj] add new test-case for cache table snoop (openthread#4600)
  [mac] remove disable CSMA-CA on the last transmit (openthread#4765)
  [toranj] update test-602-channel-select to add extra wait (openthread#4777)
  [gp712] align with the new otPlatFlash API (openthread#4774)
  [network-data] fix FindTlv() to return NULL when TLV sequence is malformed (openthread#4771)
  [simulation] enhance simulation for OTNS (openthread#4268)
  [github-actions] add `apt update` to each build (openthread#4772)
  [toranj] remove unused age property when parsing cache table (openthread#4599)
  [spinel/ncp] update address cache table property to include additional info (openthread#4599)
  [address-resolver] update public OT API for getting cache table (openthread#4599)
  [address-resolver] update how the cache table changes are logged (openthread#4599)
  [address-resolver] use linked-list and enable snooped entry timeout (openthread#4599)
  [linked-list] change PopAfter() parameter to be a pointer (openthread#4599)
  [docs] add lgtm.com status badge (openthread#4762)
  [ip6-address] add `SetMulticastNetworkPrefix()` for Prefix-Based Multicast Address (openthread#4757)
  [posix] fix mld process invalid arguments (openthread#4764)
  [cli] add `childip max` command (openthread#4759)
  [network-diagnostic] replace anonymous structs in netdiag.h (openthread#4760)
  [posix] add support for multicast group join (openthread#4687)
  [toranj] change start.sh to remove variable check to retry failed test (openthread#4761)
  [network-data] adding helper methods, simplify code (openthread#4743)
  [mesh-forwarder] do not remove router on link failures (openthread#4753)
  [meshcop] rename leader.cpp to meshcop_leader.cpp (openthread#4754)
  [bbr] (Un)Subscribe AllNetworkBBRs Multicast address (openthread#4755)
  [core] remove useless code for the old mLinkLocal16 (removed in openthread#913) (openthread#4750)
  [script] add build option to check-posix-pty (openthread#4748)

Change-Id: I3a7df8f13a9bb3db060864cbef8d238ce4db7c07
sjlongland pushed a commit to vrtsystems/openthread that referenced this pull request Dec 14, 2022
sjlongland pushed a commit to vrtsystems/openthread that referenced this pull request Dec 14, 2022
)

This commit fixes situation when Commissioner can't be turned on or off,
due to incorrect state of the Border Agent.

In order to fix it:
 - a new COMMISSIONER state in the notifier module has been introduced
 - Border Agent API is not colled from the commissioner_api.cpp file
sjlongland pushed a commit to vrtsystems/openthread that referenced this pull request Dec 14, 2022
sjlongland pushed a commit to vrtsystems/openthread that referenced this pull request Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants