From e7eb94a693f291e22501c04ab8e271e788774eb9 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Mon, 9 Jan 2023 19:35:57 -0800
Subject: [PATCH] [router-table] simplify `GetNextHop()` (#8624)

---
 src/core/thread/router_table.cpp | 57 ++++++++++++++------------------
 src/core/thread/router_table.hpp | 11 +++---
 2 files changed, 29 insertions(+), 39 deletions(-)

diff --git a/src/core/thread/router_table.cpp b/src/core/thread/router_table.cpp
index 70a723e8f..91bf23a3e 100644
--- a/src/core/thread/router_table.cpp
+++ b/src/core/thread/router_table.cpp
@@ -440,12 +440,11 @@ exit:
     return cost;
 }
 
-uint16_t RouterTable::GetNextHop(uint16_t aDestination) const
+uint16_t RouterTable::GetNextHop(uint16_t aDestRloc16) const
 {
-    uint8_t       destinationId = Mle::RouterIdFromRloc16(aDestination);
-    uint8_t       routeCost;
+    uint16_t      nextHopRloc16 = Mle::kInvalidRloc16;
+    uint8_t       destRouterId  = Mle::RouterIdFromRloc16(aDestRloc16);
     uint8_t       linkCost;
-    uint16_t      rval = Mac::kShortAddrInvalid;
     const Router *router;
     const Router *nextHop;
 
@@ -454,49 +453,41 @@ uint16_t RouterTable::GetNextHop(uint16_t aDestination) const
         const Router &parent = Get<Mle::Mle>().GetParent();
 
         VerifyOrExit(parent.IsStateValid());
-        ExitNow(rval = parent.GetRloc16());
+        nextHopRloc16 = parent.GetRloc16();
+        ExitNow();
     }
 
-    // The frame is destined to a child
-    if (destinationId == Mle::RouterIdFromRloc16(Get<Mle::Mle>().GetRloc16()))
+    if (destRouterId == Mle::RouterIdFromRloc16(Get<Mle::Mle>().GetRloc16()))
     {
-        ExitNow(rval = aDestination);
+        // Destination is device itself or one of its
+        // children.
+        ExitNow(nextHopRloc16 = aDestRloc16);
     }
 
-    router = FindRouterById(destinationId);
+    router = FindRouterById(destRouterId);
     VerifyOrExit(router != nullptr);
 
-    linkCost  = GetLinkCost(destinationId);
-    routeCost = GetRouteCost(aDestination);
+    linkCost = GetLinkCost(*router);
 
-    if ((routeCost + GetLinkCost(router->GetNextHop())) < linkCost)
+    if (linkCost < Mle::kMaxRouteCost)
     {
-        nextHop = FindNextHopOf(*router);
-        VerifyOrExit(nextHop != nullptr && !nextHop->IsStateInvalid());
-
-        rval = Mle::Rloc16FromRouterId(router->GetNextHop());
+        nextHopRloc16 = Mle::Rloc16FromRouterId(destRouterId);
     }
-    else if (linkCost < Mle::kMaxRouteCost)
+
+    // Check if we have a forwarding route path towards the
+    // destination and whether direct link or forwarding path has a
+    // lower cost.
+
+    nextHop = FindNextHopOf(*router);
+    VerifyOrExit(nextHop != nullptr);
+
+    if (router->GetCost() + GetLinkCost(*nextHop) < linkCost)
     {
-        rval = Mle::Rloc16FromRouterId(destinationId);
+        nextHopRloc16 = nextHop->GetRloc16();
     }
 
 exit:
-    return rval;
-}
-
-uint8_t RouterTable::GetRouteCost(uint16_t aRloc16) const
-{
-    uint8_t       rval = Mle::kMaxRouteCost;
-    const Router *router;
-
-    router = FindRouterByRloc16(aRloc16);
-    VerifyOrExit(router != nullptr && FindNextHopOf(*router) != nullptr);
-
-    rval = router->GetCost();
-
-exit:
-    return rval;
+    return nextHopRloc16;
 }
 
 void RouterTable::UpdateRouterIdSet(uint8_t aRouterIdSequence, const Mle::RouterIdSet &aRouterIdSet)
diff --git a/src/core/thread/router_table.hpp b/src/core/thread/router_table.hpp
index 8797582f8..99e42e182 100644
--- a/src/core/thread/router_table.hpp
+++ b/src/core/thread/router_table.hpp
@@ -165,14 +165,14 @@ public:
     uint8_t GetPathCost(uint16_t aDestRloc16) const;
 
     /**
-     * This method returns the next hop towards an RLOC16 destination.
+     * This method determines the next hop towards an RLOC16 destination.
      *
-     * @param[in]  aDestination  The RLOC16 of the destination.
+     * @param[in]  aDestRloc16  The RLOC16 of the destination.
      *
-     * @returns A RLOC16 of the next hop if a route is known, kInvalidRloc16 otherwise.
+     * @returns A RLOC16 of the next hop if a route is known, `Mle::kInvalidRloc16` otherwise.
      *
      */
-    uint16_t GetNextHop(uint16_t aDestination) const;
+    uint16_t GetNextHop(uint16_t aDestRloc16) const;
 
     /**
      * This method finds the router for a given Router ID.
@@ -410,8 +410,7 @@ private:
         return AsNonConst(AsConst(this)->FindRouter(aMatcher));
     }
 
-    bool    UpdateLinkQualityOut(const Mle::RouteTlv &aRouteTlv, Router &aNeighbor, bool &aResetAdvInterval);
-    uint8_t GetRouteCost(uint16_t aRloc16) const;
+    bool UpdateLinkQualityOut(const Mle::RouteTlv &aRouteTlv, Router &aNeighbor, bool &aResetAdvInterval);
 
     class RouterIdMap
     {