From e0ee632d00135147827a72c505772b68f1844791 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Thu, 19 Apr 2018 10:02:25 -0700
Subject: [PATCH] [mle] adding SetRole() method to change device role (#2672)

This commit adds a new method `Mle::SetRole()` to update the device
role. This method is used in MLE class instead of direct assignment to
`mRole` variable. The `SetRole()` will ensure to set flag on
`Notifier` (when there is a role change) and also log the role change.
This change simplifies the code and addresses a (rare) issue where
notifier callback would not be invoked on transition from "detached"
to "disabled" role.
---
 src/core/thread/mle.cpp        | 44 ++++++++++++++++++----------------
 src/core/thread/mle.hpp        |  8 +++++++
 src/core/thread/mle_router.cpp | 18 ++++----------
 3 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp
index 80c4d59de..dada587b5 100644
--- a/src/core/thread/mle.cpp
+++ b/src/core/thread/mle.cpp
@@ -238,8 +238,6 @@ otError Mle::Start(bool aEnableReattach, bool aAnnounceAttach)
     VerifyOrExit(otPlatRadioGetPromiscuous(&netif.GetInstance()) == false, error = OT_ERROR_INVALID_STATE);
     VerifyOrExit(netif.IsUp(), error = OT_ERROR_INVALID_STATE);
 
-    mRole = OT_DEVICE_ROLE_DETACHED;
-    GetNotifier().SetFlags(OT_CHANGED_THREAD_ROLE);
     SetStateDetached();
 
     netif.GetKeyManager().Start();
@@ -275,21 +273,37 @@ otError Mle::Stop(bool aClearNetworkDatasets)
 {
     ThreadNetif &netif = GetNetif();
 
-    netif.GetKeyManager().Stop();
-    SetStateDetached();
-    netif.RemoveUnicastAddress(mMeshLocal16);
-
     if (aClearNetworkDatasets)
     {
         netif.GetActiveDataset().HandleDetach();
         netif.GetPendingDataset().HandleDetach();
     }
 
-    mRole = OT_DEVICE_ROLE_DISABLED;
+    VerifyOrExit(mRole != OT_DEVICE_ROLE_DISABLED);
 
+    netif.GetKeyManager().Stop();
+    SetStateDetached();
+    netif.RemoveUnicastAddress(mMeshLocal16);
+
+    SetRole(OT_DEVICE_ROLE_DISABLED);
+
+exit:
     return OT_ERROR_NONE;
 }
 
+void Mle::SetRole(otDeviceRole aRole)
+{
+    VerifyOrExit(aRole != mRole);
+
+    otLogInfoMle(GetInstance(), "Role %s -> %s", RoleToString(mRole), RoleToString(aRole));
+
+    mRole = aRole;
+    GetNotifier().SetFlags(OT_CHANGED_THREAD_ROLE);
+
+exit:
+    return;
+}
+
 otError Mle::Restore(void)
 {
     ThreadNetif &         netif    = GetNetif();
@@ -565,17 +579,12 @@ otError Mle::SetStateDetached(void)
 {
     ThreadNetif &netif = GetNetif();
 
-    if (mRole != OT_DEVICE_ROLE_DETACHED)
-    {
-        GetNotifier().SetFlags(OT_CHANGED_THREAD_ROLE);
-    }
-
     if (mRole == OT_DEVICE_ROLE_LEADER)
     {
         netif.RemoveUnicastAddress(mLeaderAloc);
     }
 
-    mRole               = OT_DEVICE_ROLE_DETACHED;
+    SetRole(OT_DEVICE_ROLE_DETACHED);
     mParentRequestState = kParentIdle;
     mParentRequestTimer.Stop();
     mChildUpdateRequestTimer.Stop();
@@ -585,7 +594,6 @@ otError Mle::SetStateDetached(void)
     netif.GetIp6().SetForwardingEnabled(false);
     netif.GetIp6().GetMpl().SetTimerExpirations(0);
 
-    otLogInfoMle(GetInstance(), "Role -> Detached");
     return OT_ERROR_NONE;
 }
 
@@ -593,18 +601,13 @@ otError Mle::SetStateChild(uint16_t aRloc16)
 {
     ThreadNetif &netif = GetNetif();
 
-    if (mRole != OT_DEVICE_ROLE_CHILD)
-    {
-        GetNotifier().SetFlags(OT_CHANGED_THREAD_ROLE);
-    }
-
     if (mRole == OT_DEVICE_ROLE_LEADER)
     {
         netif.RemoveUnicastAddress(mLeaderAloc);
     }
 
     SetRloc16(aRloc16);
-    mRole                = OT_DEVICE_ROLE_CHILD;
+    SetRole(OT_DEVICE_ROLE_CHILD);
     mParentRequestState  = kParentIdle;
     mReattachState       = kReattachStop;
     mChildUpdateAttempts = 0;
@@ -639,7 +642,6 @@ otError Mle::SetStateChild(uint16_t aRloc16)
     mPreviousParentRloc = mParent.GetRloc16();
 #endif
 
-    otLogInfoMle(GetInstance(), "Role -> Child");
     return OT_ERROR_NONE;
 }
 
diff --git a/src/core/thread/mle.hpp b/src/core/thread/mle.hpp
index 4a4cb03b2..a8ef01588 100644
--- a/src/core/thread/mle.hpp
+++ b/src/core/thread/mle.hpp
@@ -971,6 +971,14 @@ protected:
      */
     Message *NewMleMessage(void);
 
+    /**
+     * This method sets the device role.
+     *
+     * @param[in] aRole A device role.
+     *
+     */
+    void SetRole(otDeviceRole aRole);
+
     /**
      * This method appends an MLE header to a message.
      *
diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp
index b8039c4c9..207c2aba2 100644
--- a/src/core/thread/mle_router.cpp
+++ b/src/core/thread/mle_router.cpp
@@ -416,13 +416,8 @@ otError MleRouter::SetStateRouter(uint16_t aRloc16)
 {
     ThreadNetif &netif = GetNetif();
 
-    if (mRole != OT_DEVICE_ROLE_ROUTER)
-    {
-        GetNotifier().SetFlags(OT_CHANGED_THREAD_ROLE);
-    }
-
     SetRloc16(aRloc16);
-    mRole               = OT_DEVICE_ROLE_ROUTER;
+    SetRole(OT_DEVICE_ROLE_ROUTER);
     mParentRequestState = kParentIdle;
     mParentRequestTimer.Stop();
     mChildUpdateRequestTimer.Stop();
@@ -455,7 +450,6 @@ otError MleRouter::SetStateRouter(uint16_t aRloc16)
         }
     }
 
-    otLogInfoMle(GetInstance(), "Role -> Router");
     return OT_ERROR_NONE;
 }
 
@@ -463,13 +457,8 @@ otError MleRouter::SetStateLeader(uint16_t aRloc16)
 {
     ThreadNetif &netif = GetNetif();
 
-    if (mRole != OT_DEVICE_ROLE_LEADER)
-    {
-        GetNotifier().SetFlags(OT_CHANGED_THREAD_ROLE);
-    }
-
     SetRloc16(aRloc16);
-    mRole               = OT_DEVICE_ROLE_LEADER;
+    SetRole(OT_DEVICE_ROLE_LEADER);
     mParentRequestState = kParentIdle;
     mParentRequestTimer.Stop();
     mChildUpdateRequestTimer.Stop();
@@ -502,7 +491,8 @@ otError MleRouter::SetStateLeader(uint16_t aRloc16)
         }
     }
 
-    otLogInfoMle(GetInstance(), "Role -> Leader %d", mLeaderData.GetPartitionId());
+    otLogInfoMle(GetInstance(), "Leader partition id 0x%x", mLeaderData.GetPartitionId());
+
     return OT_ERROR_NONE;
 }