From 58e771cbad1b435e2ad2ee1743484a2aa611d70f Mon Sep 17 00:00:00 2001 From: "Duda, Lukasz" Date: Fri, 3 Apr 2020 22:41:58 +0200 Subject: [PATCH] [commissioner] ensure synchronization with Border Agent (#4788) 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 --- include/openthread/commissioner.h | 9 +++++---- include/openthread/instance.h | 1 + src/core/api/commissioner_api.cpp | 6 ------ src/core/common/notifier.cpp | 4 ++++ src/core/meshcop/border_agent.cpp | 6 +++++- src/core/meshcop/commissioner.cpp | 23 ++++++++++++++++++----- src/core/meshcop/commissioner.hpp | 15 ++++++++++++--- 7 files changed, 45 insertions(+), 19 deletions(-) diff --git a/include/openthread/commissioner.h b/include/openthread/commissioner.h index 85743620f..00ed368d7 100644 --- a/include/openthread/commissioner.h +++ b/include/openthread/commissioner.h @@ -158,8 +158,9 @@ typedef void (*otCommissionerJoinerCallback)(otCommissionerJoinerEvent aEvent, * @param[in] aJoinerCallback A pointer to a function that is called with a joiner event occurs. * @param[in] aCallbackContext A pointer to application-specific context. * - * @retval OT_ERROR_NONE Successfully started the Commissioner role. - * @retval OT_ERROR_INVALID_STATE Commissioner is already started. + * @retval OT_ERROR_NONE Successfully started the Commissioner service. + * @retval OT_ERROR_ALREADY Commissioner is already started. + * @retval OT_ERROR_INVALID_STATE Device is not currently attached to a network. * */ otError otCommissionerStart(otInstance * aInstance, @@ -172,8 +173,8 @@ otError otCommissionerStart(otInstance * aInstance, * * @param[in] aInstance A pointer to an OpenThread instance. * - * @retval OT_ERROR_NONE Successfully stopped the Commissioner role. - * @retval OT_ERROR_INVALID_STATE Commissioner is already stopped. + * @retval OT_ERROR_NONE Successfully stopped the Commissioner service. + * @retval OT_ERROR_ALREADY Commissioner is already stopped. * */ otError otCommissionerStop(otInstance *aInstance); diff --git a/include/openthread/instance.h b/include/openthread/instance.h index 205b80a45..7fba573cc 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -150,6 +150,7 @@ enum OT_CHANGED_THREAD_BACKBONE_ROUTER_STATE = 1 << 25, ///< Backbone Router state changed OT_CHANGED_THREAD_BACKBONE_ROUTER_LOCAL = 1 << 26, ///< Local Backbone Router configuration changed OT_CHANGED_JOINER_STATE = 1 << 27, ///< Joiner state changed + OT_CHANGED_COMMISSIONER_STATE = 1 << 28, ///< Commissioner state changed }; /** diff --git a/src/core/api/commissioner_api.cpp b/src/core/api/commissioner_api.cpp index 7cbdde9c9..07c741de9 100644 --- a/src/core/api/commissioner_api.cpp +++ b/src/core/api/commissioner_api.cpp @@ -50,9 +50,6 @@ otError otCommissionerStart(otInstance * aInstance, Instance &instance = *static_cast(aInstance); -#if OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE - SuccessOrExit(error = instance.Get().Stop()); -#endif SuccessOrExit(error = instance.Get().Start(aStateCallback, aJoinerCallback, aCallbackContext)); exit: @@ -65,9 +62,6 @@ otError otCommissionerStop(otInstance *aInstance) Instance &instance = *static_cast(aInstance); SuccessOrExit(error = instance.Get().Stop(/* aResign */ true)); -#if OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE - SuccessOrExit(error = instance.Get().Start()); -#endif exit: return error; diff --git a/src/core/common/notifier.cpp b/src/core/common/notifier.cpp index 391d5348e..7c3a7891a 100644 --- a/src/core/common/notifier.cpp +++ b/src/core/common/notifier.cpp @@ -316,6 +316,10 @@ const char *Notifier::FlagToString(otChangedFlags aFlag) const retval = "NetifState"; break; + case OT_CHANGED_COMMISSIONER_STATE: + retval = "CommissionerState"; + break; + default: break; } diff --git a/src/core/meshcop/border_agent.cpp b/src/core/meshcop/border_agent.cpp index 8e79910a9..dfd739322 100644 --- a/src/core/meshcop/border_agent.cpp +++ b/src/core/meshcop/border_agent.cpp @@ -367,7 +367,11 @@ void BorderAgent::HandleStateChanged(Notifier::Callback &aCallback, otChangedFla void BorderAgent::HandleStateChanged(otChangedFlags aFlags) { - VerifyOrExit((aFlags & OT_CHANGED_THREAD_ROLE) != 0); + VerifyOrExit((aFlags & (OT_CHANGED_THREAD_ROLE | OT_CHANGED_COMMISSIONER_STATE)) != 0); + +#if OPENTHREAD_CONFIG_COMMISSIONER_ENABLE && OPENTHREAD_FTD + VerifyOrExit(Get().IsDisabled()); +#endif if (Get().IsAttached()) { diff --git a/src/core/meshcop/commissioner.cpp b/src/core/meshcop/commissioner.cpp index 8e5b4e7f5..35322a1d2 100644 --- a/src/core/meshcop/commissioner.cpp +++ b/src/core/meshcop/commissioner.cpp @@ -88,11 +88,12 @@ Commissioner::Commissioner(Instance &aInstance) void Commissioner::SetState(otCommissionerState aState) { - VerifyOrExit(mState != aState); + otCommissionerState oldState = mState; + OT_UNUSED_VARIABLE(oldState); - otLogInfoMeshCoP("Commissioner State: %s -> %s", StateToString(mState), StateToString(aState)); + SuccessOrExit(Get().Update(mState, aState, OT_CHANGED_COMMISSIONER_STATE)); - mState = aState; + otLogInfoMeshCoP("CommissionerState: %s -> %s", StateToString(oldState), StateToString(aState)); if (mStateCallback) { @@ -150,7 +151,12 @@ otError Commissioner::Start(otCommissionerStateCallback aStateCallback, otError error = OT_ERROR_NONE; VerifyOrExit(Get().IsAttached(), error = OT_ERROR_INVALID_STATE); - VerifyOrExit(mState == OT_COMMISSIONER_STATE_DISABLED, error = OT_ERROR_INVALID_STATE); + VerifyOrExit(mState == OT_COMMISSIONER_STATE_DISABLED, error = OT_ERROR_ALREADY); + +#if OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE + error = Get().Stop(); + VerifyOrExit(error == OT_ERROR_NONE || error == OT_ERROR_ALREADY); +#endif SuccessOrExit(error = Get().Start(SendRelayTransmit, this)); Get().SetConnectedCallback(&Commissioner::HandleCoapsConnected, this); @@ -166,8 +172,10 @@ otError Commissioner::Start(otCommissionerStateCallback aStateCallback, exit: if (error != OT_ERROR_NONE) { + otLogWarnMeshCoP("Failed to start commissioner: %s", otThreadErrorToString(error)); Get().Stop(); } + return error; } @@ -176,7 +184,7 @@ otError Commissioner::Stop(bool aResign) otError error = OT_ERROR_NONE; bool needResign = false; - VerifyOrExit(mState != OT_COMMISSIONER_STATE_DISABLED, error = OT_ERROR_INVALID_STATE); + VerifyOrExit(mState != OT_COMMISSIONER_STATE_DISABLED, error = OT_ERROR_ALREADY); Get().Stop(); @@ -202,6 +210,11 @@ otError Commissioner::Stop(bool aResign) } exit: + if (error != OT_ERROR_NONE) + { + otLogWarnMeshCoP("Failed to stop Commissioner: %s", otThreadErrorToString(error)); + } + return error; } diff --git a/src/core/meshcop/commissioner.hpp b/src/core/meshcop/commissioner.hpp index 67b10075c..ba8e26123 100644 --- a/src/core/meshcop/commissioner.hpp +++ b/src/core/meshcop/commissioner.hpp @@ -74,7 +74,8 @@ public: * @param[in] aCallbackContext A pointer to application-specific context. * * @retval OT_ERROR_NONE Successfully started the Commissioner service. - * @retval OT_ERROR_INVALID_STATE Commissioner is already started. + * @retval OT_ERROR_ALREADY Commissioner is already started. + * @retval OT_ERROR_INVALID_STATE Device is not currently attached to a network. * */ otError Start(otCommissionerStateCallback aStateCallback, @@ -86,8 +87,8 @@ public: * * @param[in] aResign Whether send LEAD_KA.req to resign as Commissioner * - * @retval OT_ERROR_NONE Successfully stopped the Commissioner service. - * @retval OT_ERROR_INVALID_STATE Commissioner is already stopped. + * @retval OT_ERROR_NONE Successfully stopped the Commissioner service. + * @retval OT_ERROR_ALREADY Commissioner is already stopped. * */ otError Stop(bool aResign); @@ -172,6 +173,14 @@ public: */ bool IsActive(void) const { return mState == OT_COMMISSIONER_STATE_ACTIVE; } + /** + * This method indicates whether or not the Commissioner role is disabled. + * + * @returns TRUE if the Commissioner role is disabled, FALSE otherwise. + * + */ + bool IsDisabled(void) const { return mState == OT_COMMISSIONER_STATE_DISABLED; } + /** * This function returns the Commissioner State. *