[mle] Refactor of Router Role Upgrade and Downgrade conditions

Key changes:
- Router role upgrade attempts may no longer be started when beginning the
  child role, which may allow REEDs to advertise immediately after
  swapping parents when the network is already stable.
- Split ShouldDowngrade() into ShouldBeginDowngradeTimer for when the
  downgrade timer is started from advertisement conditions and
  ShouldRouterDowngrade() for checks that apply when the timer is started or when it is
  performed.
- Created ShouldUpgrade() for conditions to check when the timer is started and
  when the upgrade is attempted

Author: jthompson-lutron

src/core/thread/mle.hpp

@@ -2518,11 +2518,49 @@ class Mle : public InstanceLocator, private NonCopyable
     void     SetChildStateToValid(Child &aChild);
     bool     HasChildren(void);
     void     RemoveChildren(void);
-    bool     ShouldDowngrade(uint8_t aNeighborId, const RouteTlv &aRouteTlv) const;
-    bool     NeighborHasComparableConnectivity(const RouteTlv &aRouteTlv, uint8_t aNeighborId) const;
-    void     HandleAdvertiseTrickleTimer(void);
-    void     HandleTimeTick(void);
-    void     HandleRouterTableEvent(RouterTable::Events aEvents);
+
+    /**
+     * Determine if a router downgrade transition should occur or timer should be started.
+     *
+     * Note: Checks all downgrade conditions except having comparable connectivity to a neighbor router both when
+     * the timer is started and when the downgrade is performed.
+     *
+     * @retval TRUE   Downgrade conditions may apply for the Router role.
+     * @retval FALSE  The device should not downgrade or is not in the Router role.
+     *
+     * @sa ShouldBeginDowngradeTimer
+     */
+    bool ShouldRouterDowngrade(void) const;
+
+    /**
+     * Determine if a router downgrade transition timer should be started.
+     *
+     * Note: Checks all conditions known when handling advertisments.
+     *
+     * @retval TRUE   The unit should begin its downgrade timer.
+     * @retval FALSE  The device should not downgrade.
+     */
+    bool ShouldBeginDowngradeTimer(uint8_t aNeighborId, const RouteTlv &aRouteTlv) const;
+
+    /**
+     * Determine if a router upgrade transition should occur or timer should be started.
+     *
+     * Note: This checks all upgrade conditions both when the timer is started and when the upgrade is attempted but
+     * does not check `HasNeighborWithGoodLinkQuality()`, so that the runtime required by that check will only
+     * apply just before making the attempt to upgrade.
+     *
+     * @retval TRUE   Upgrade conditions may apply.
+     * @retval FALSE  The device should not upgrade.
+     *
+     * @sa HasNeighborWithGoodLinkQuality
+     */
+    bool ShouldUpgrade(void) const;
+
+    bool NeighborHasComparableConnectivity(uint8_t aNeighborId, const RouteTlv &aRouteTlv) const;
+
+    void HandleAdvertiseTrickleTimer(void);
+    void HandleTimeTick(void);
+    void HandleRouterTableEvent(RouterTable::Events aEvents);
 
     template <Uri kUri> void HandleTmf(Coap::Msg &aMsg);
 

src/core/thread/mle_ftd.cpp

@@ -131,7 +131,7 @@ void Mle::UpdateRouterRoleAllowed(UpdateRouterRoleAllowedReason aReason)
     // Take action based on the current role, the new `mRouterRoleAllowed`,
     // and the reason for the change.
 
-    if (IsChild() && mRouterRoleAllowed && (aReason == kReasonConfigParameterChanged))
+    if (IsChild() && mRouterRoleAllowed && (aReason == kReasonConfigParameterChanged) && ShouldUpgrade())
     {
         StartRouterRoleUpgradeTransition();
     }
@@ -343,11 +343,12 @@ void Mle::HandleChildStart(void)
 {
     mAddressSolicitRejected = false;
 
-    if (IsRouterRoleAllowed())
+    if (ShouldUpgrade())
     {
         // As a REED, re-start the timer to check for a role upgrade when re-attaching as a Child
         StartRouterRoleUpgradeTransition();
     }
+    // If the REED shouldn't upgrade, then it may begin sending advertisements on the next time tick.
 
     StopLeader();
     Get<TimeTicker>().RegisterReceiver(TimeTicker::kMle);
@@ -1310,7 +1311,7 @@ Error Mle::HandleAdvertisementOnFtd(RxInfo &aRxInfo, uint16_t aSourceAddress, co
                 ExitNow(error = kErrorDetached);
             }
 
-            if (mRouterTable.GetActiveRouterCount() < mRouterUpgradeThreshold)
+            if (ShouldUpgrade())
             {
                 if (!IsRouterRoleTransitioning())
                 {
@@ -1348,7 +1349,7 @@ Error Mle::HandleAdvertisementOnFtd(RxInfo &aRxInfo, uint16_t aSourceAddress, co
     //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
     // Update routers as a router or leader.
 
-    if (ShouldDowngrade(routerId, routeTlv))
+    if (ShouldBeginDowngradeTimer(routerId, routeTlv))
     {
         // Downgrade conditions have been met after an advertisement from another active router
         StartRouterRoleDowngradeTransition();
@@ -1623,7 +1624,7 @@ void Mle::HandleTimeTick(void)
         else if (IsRouterRoleUpgradePending())
         {
             // An upgrade should be performed, if the conditions to upgrade still apply
-            if (mRouterTable.GetActiveRouterCount() < mRouterUpgradeThreshold && HasNeighborWithGoodLinkQuality())
+            if (ShouldUpgrade() && HasNeighborWithGoodLinkQuality())
             {
                 IgnoreError(BecomeRouter(kReasonTooFewRouters));
                 // Any attempt to become a router will also clear the current role transition
@@ -1649,7 +1650,7 @@ void Mle::HandleTimeTick(void)
         if (mRouterRoleTransition.IsDowngradePending())
         {
             bool roleAllowed           = IsRouterRoleAllowed();
-            bool shouldRouterDowngrade = IsRouter() && mRouterTable.GetActiveRouterCount() > mRouterDowngradeThreshold;
+            bool shouldRouterDowngrade = ShouldRouterDowngrade();
             if (!roleAllowed || shouldRouterDowngrade)
             {
                 if (IsRouter())
@@ -3830,71 +3831,90 @@ void Mle::FillConnectivityTlvValue(ConnectivityTlvValue &aTlvValue) const
     aTlvValue.InitFrom(connectivity);
 }
 
-bool Mle::ShouldDowngrade(uint8_t aNeighborId, const RouteTlv &aRouteTlv) const
+bool Mle::ShouldBeginDowngradeTimer(uint8_t aNeighborId, const RouteTlv &aRouteTlv) const
 {
     // Determine whether all conditions are satisfied for the router
     // to downgrade after receiving info for a neighboring router
     // with Router ID `aNeighborId` along with its `aRouteTlv`.
 
-    bool    shouldDowngrade   = false;
-    uint8_t activeRouterCount = mRouterTable.GetActiveRouterCount();
-    uint8_t count;
+    bool shouldDowngrade = false;
 
-    VerifyOrExit(IsRouter());
     VerifyOrExit(mRouterTable.IsAllocated(aNeighborId));
-    VerifyOrExit(!mBlockDowngrade);
 
     // Return false, to let an existing downgrade transition continue
     VerifyOrExit(!mRouterRoleTransition.IsDowngrading());
 
+    VerifyOrExit(ShouldRouterDowngrade());
+
+    // Check that the neighbor has as good or better-quality links to the same routers.
+    VerifyOrExit(NeighborHasComparableConnectivity(aNeighborId, aRouteTlv));
+
+#if OPENTHREAD_CONFIG_BORDER_ROUTER_ENABLE && OPENTHREAD_CONFIG_BORDER_ROUTER_REQUEST_ROUTER_ROLE
+    // Check if we are eligible to be router due to being a BR.
+    VerifyOrExit(!Get<NetworkData::Notifier>().IsEligibleForRouterRoleUpgradeAsBorderRouter());
+#endif
+
+    shouldDowngrade = true;
+
+exit:
+    return shouldDowngrade;
+}
+
+bool Mle::ShouldRouterDowngrade(void) const
+{
+    uint8_t count;
+    uint8_t excessRouters;
+    bool    shouldDowngrade   = false;
+    uint8_t activeRouterCount = mRouterTable.GetActiveRouterCount();
+
+    // Only kRoleRouter may downgrade.
+    // kRoleLeader may not downgrade due to these conditions, and other roles are not applicable.
+    VerifyOrExit(IsRouter());
+
+    VerifyOrExit(!mBlockDowngrade);
     VerifyOrExit(activeRouterCount > mRouterDowngradeThreshold);
 
+    excessRouters = (activeRouterCount > mRouterDowngradeThreshold) ? activeRouterCount - mRouterDowngradeThreshold : 0;
+    VerifyOrExit(mChildTable.GetNumChildren(Child::kInStateValid) < excessRouters * 3);
+
     // Check that we have at least `kMinDowngradeNeighbors`
     // neighboring routers with two-way link quality of 2 or better.
-
     count = 0;
-
     for (const Router &router : mRouterTable)
     {
         if (!router.IsStateValid() || (router.GetTwoWayLinkQuality() < kLinkQuality2))
         {
             continue;
         }
-
         count++;
-
         if (count >= kMinDowngradeNeighbors)
         {
             break;
         }
     }
-
     VerifyOrExit(count >= kMinDowngradeNeighbors);
 
-    // Check that we have fewer children than three times the number
-    // of excess routers (defined as the difference between number of
-    // active routers and `mRouterDowngradeThreshold`).
-
-    count = activeRouterCount - mRouterDowngradeThreshold;
-    VerifyOrExit(mChildTable.GetNumChildren(Child::kInStateValid) < count * 3);
-
-    // Check that the neighbor has as good or better-quality links to
-    // same routers.
+    shouldDowngrade = true;
+exit:
+    return shouldDowngrade;
+}
 
-    VerifyOrExit(NeighborHasComparableConnectivity(aRouteTlv, aNeighborId));
+bool Mle::ShouldUpgrade(void) const
+{
+    bool shouldUpgrade = false;
 
-#if OPENTHREAD_CONFIG_BORDER_ROUTER_ENABLE && OPENTHREAD_CONFIG_BORDER_ROUTER_REQUEST_ROUTER_ROLE
-    // Check if we are eligible to be router due to being a BR.
-    VerifyOrExit(!Get<NetworkData::Notifier>().IsEligibleForRouterRoleUpgradeAsBorderRouter());
-#endif
+    VerifyOrExit(IsRouterRoleAllowed());
 
-    shouldDowngrade = true;
+    if (mRouterTable.GetActiveRouterCount() < mRouterUpgradeThreshold)
+    {
+        shouldUpgrade = true;
+    }
 
 exit:
-    return shouldDowngrade;
+    return shouldUpgrade;
 }
 
-bool Mle::NeighborHasComparableConnectivity(const RouteTlv &aRouteTlv, uint8_t aNeighborId) const
+bool Mle::NeighborHasComparableConnectivity(uint8_t aNeighborId, const RouteTlv &aRouteTlv) const
 {
     // Check whether the neighboring router with Router ID `aNeighborId`
     // (along with its `aRouteTlv`) has as good or better-quality links