[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

@@ -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);

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
 };
 
 /**

src/core/api/commissioner_api.cpp

@@ -50,9 +50,6 @@ otError otCommissionerStart(otInstance *                 aInstance,
 
     Instance &instance = *static_cast<Instance *>(aInstance);
 
-#if OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE
-    SuccessOrExit(error = instance.Get<MeshCoP::BorderAgent>().Stop());
-#endif
     SuccessOrExit(error =
                       instance.Get<MeshCoP::Commissioner>().Start(aStateCallback, aJoinerCallback, aCallbackContext));
 exit:
@@ -65,9 +62,6 @@ otError otCommissionerStop(otInstance *aInstance)
     Instance &instance = *static_cast<Instance *>(aInstance);
 
     SuccessOrExit(error = instance.Get<MeshCoP::Commissioner>().Stop(/* aResign */ true));
-#if OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE
-    SuccessOrExit(error = instance.Get<MeshCoP::BorderAgent>().Start());
-#endif
 
 exit:
     return error;

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;
     }

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<MeshCoP::Commissioner>().IsDisabled());
+#endif
 
     if (Get<Mle::MleRouter>().IsAttached())
     {

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<Notifier>().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<Mle::MleRouter>().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<MeshCoP::BorderAgent>().Stop();
+    VerifyOrExit(error == OT_ERROR_NONE || error == OT_ERROR_ALREADY);
+#endif
 
     SuccessOrExit(error = Get<Coap::CoapSecure>().Start(SendRelayTransmit, this));
     Get<Coap::CoapSecure>().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<Coap::CoapSecure>().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<Coap::CoapSecure>().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;
 }
 

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.
      *