From 1e5c664a9a229213c6cb892f422176595f3d5a4d Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Tue, 10 Jan 2023 19:05:54 -0800 Subject: [PATCH] [router-table] update cost on FED and enhance `GetPathCost()` (#8629) This commit adds new method `RouterTable::UpdateRoutesOnFed()` which tracks cost towards all routers on an FED child based the parent's cost towards them. This method is called whenever we receive an MLE Advertisement from the parent (replacing the code which tracked the cost only towards the leader) and also during attach process upon receiving a Child ID Response containing `RouteTlv` from the parent. This commit also updates `GetPathCost()` to handle more case: - Where device is detached or disabled, we return `kMaxRouteCost`. - If device is a child, we derive the path cost as the link cost to the parent plus the cost from parent towards the destination. - If device is a child and destination is another child of the same parent, we use link cost to parent plus an estimated cost towards the other child. This commit also adds a new helper method `GetPathCostToLeader()` which is then used in `MleRouter` to simplify the code, e.g., when deciding whether to respond to an MLE Parent Request to to check the current path cost to leader. --- src/core/thread/mle.cpp | 6 +- src/core/thread/mle_router.cpp | 94 ++--------- src/core/thread/router_table.cpp | 150 ++++++++++++------ src/core/thread/router_table.hpp | 29 +++- src/core/thread/topology.cpp | 21 +++ src/core/thread/topology.hpp | 27 ++-- .../toranj/cli/test-008-multicast-traffic.py | 1 + 7 files changed, 180 insertions(+), 148 deletions(-) diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index 5674bffb6..ce5413b4d 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -3373,9 +3373,13 @@ void Mle::HandleChildIdResponse(RxInfo &aRxInfo) #if OPENTHREAD_FTD if (IsFullThreadDevice()) { - switch (Get().ProcessRouteTlv(aRxInfo)) + RouteTlv routeTlv; + + switch (Get().ProcessRouteTlv(aRxInfo, routeTlv)) { case kErrorNone: + Get().UpdateRoutesOnFed(routeTlv, RouterIdFromRloc16(sourceAddress)); + break; case kErrorNotFound: break; default: diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index 9ddade460..c5fb11234 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -1276,8 +1276,6 @@ Error MleRouter::HandleAdvertisement(RxInfo &aRxInfo) if (IsFullThreadDevice()) { - Router *leader; - if ((mRouterSelectionJitterTimeout == 0) && (mRouterTable.GetActiveRouterCount() < mRouterUpgradeThreshold)) { @@ -1285,37 +1283,7 @@ Error MleRouter::HandleAdvertisement(RxInfo &aRxInfo) ExitNow(); } - leader = mRouterTable.GetLeader(); - - if (leader != nullptr) - { - for (uint8_t id = 0, routeCount = 0; id <= kMaxRouterId; id++) - { - if (!routeTlv.IsRouterIdSet(id)) - { - continue; - } - - if (id != GetLeaderId()) - { - routeCount++; - continue; - } - - if (routeTlv.GetRouteCost(routeCount) > 0) - { - leader->SetNextHop(id); - leader->SetCost(routeTlv.GetRouteCost(routeCount)); - } - else - { - leader->SetNextHop(kInvalidRouterId); - leader->SetCost(0); - } - - break; - } - } + mRouterTable.UpdateRoutesOnFed(routeTlv, routerId); } } else @@ -1397,7 +1365,6 @@ void MleRouter::HandleParentRequest(RxInfo &aRxInfo) uint16_t version; uint8_t scanMask; Challenge challenge; - Router *leader; Child *child; uint8_t modeBitmask; DeviceMode mode; @@ -1421,13 +1388,7 @@ void MleRouter::HandleParentRequest(RxInfo &aRxInfo) VerifyOrExit(mRouterTable.GetLeaderAge() < mNetworkIdTimeout, error = kErrorDrop); // 3. Its current routing path cost to the Leader is infinite. - leader = mRouterTable.GetLeader(); - OT_ASSERT(leader != nullptr); - - VerifyOrExit(IsLeader() || mRouterTable.GetLinkCost(GetLeaderId()) < kMaxRouteCost || - (IsChild() && leader->GetCost() + 1 < kMaxRouteCost) || - (leader->GetCost() + mRouterTable.GetLinkCost(leader->GetNextHop()) < kMaxRouteCost), - error = kErrorDrop); + VerifyOrExit(mRouterTable.GetPathCostToLeader() < kMaxRouteCost, error = kErrorDrop); // 4. It is a REED and there are already `kMaxRouters` active routers in // the network (because Leader would reject any further address solicit). @@ -3313,7 +3274,7 @@ void MleRouter::ResolveRoutingLoops(uint16_t aSourceMac, uint16_t aDestRloc16) VerifyOrExit(router != nullptr); // invalidate next hop - router->SetNextHop(kInvalidRouterId); + router->SetNextHopToInvalid(); ResetAdvertiseInterval(); exit: @@ -3490,7 +3451,7 @@ void MleRouter::HandleAddressSolicitResponse(Coap::Message *aMessage, VerifyOrExit(router != nullptr); router->SetExtAddress(Get().GetExtAddress()); - router->SetCost(0); + router->SetNextHopToInvalid(); router = mRouterTable.FindRouterById(mParent.GetRouterId()); VerifyOrExit(router != nullptr); @@ -3499,8 +3460,7 @@ void MleRouter::HandleAddressSolicitResponse(Coap::Message *aMessage, router->SetFrom(mParent); router->SetState(Neighbor::kStateValid); - router->SetNextHop(kInvalidRouterId); - router->SetCost(0); + router->SetNextHopToInvalid(); leader = mRouterTable.GetLeader(); OT_ASSERT(leader != nullptr); @@ -3508,8 +3468,7 @@ void MleRouter::HandleAddressSolicitResponse(Coap::Message *aMessage, if (leader != router) { // Keep route path to the Leader reported by the parent before it is updated. - leader->SetCost(mParent.GetLeaderCost()); - leader->SetNextHop(RouterIdFromRloc16(mParent.GetRloc16())); + leader->SetNextHopAndCost(RouterIdFromRloc16(mParent.GetRloc16()), mParent.GetLeaderCost()); } if (isRouterIdAllocated) @@ -3707,9 +3666,7 @@ exit: void MleRouter::FillConnectivityTlv(ConnectivityTlv &aTlv) { - Router *leader; - uint8_t cost; - int8_t parentPriority = kParentPriorityMedium; + int8_t parentPriority = kParentPriorityMedium; if (mParentPriority != kParentPriorityUnspecified) { @@ -3732,22 +3689,12 @@ void MleRouter::FillConnectivityTlv(ConnectivityTlv &aTlv) aTlv.SetParentPriority(parentPriority); - // compute leader cost and link qualities aTlv.SetLinkQuality1(0); aTlv.SetLinkQuality2(0); aTlv.SetLinkQuality3(0); - leader = mRouterTable.GetLeader(); - cost = (leader != nullptr) ? leader->GetCost() : static_cast(kMaxRouteCost); - - switch (mRole) + if (IsChild()) { - case kRoleDisabled: - case kRoleDetached: - cost = static_cast(kMaxRouteCost); - break; - - case kRoleChild: switch (mParent.GetLinkQualityIn()) { case kLinkQuality0: @@ -3765,30 +3712,8 @@ void MleRouter::FillConnectivityTlv(ConnectivityTlv &aTlv) aTlv.SetLinkQuality3(aTlv.GetLinkQuality3() + 1); break; } - - cost += CostForLinkQuality(mParent.GetLinkQualityIn()); - break; - - case kRoleRouter: - if (leader != nullptr) - { - cost += mRouterTable.GetLinkCost(leader->GetNextHop()); - - if (!IsRouterIdValid(leader->GetNextHop()) || mRouterTable.GetLinkCost(GetLeaderId()) < cost) - { - cost = mRouterTable.GetLinkCost(GetLeaderId()); - } - } - - break; - - case kRoleLeader: - cost = 0; - break; } - aTlv.SetActiveRouters(mRouterTable.GetActiveRouterCount()); - for (const Router &router : Get()) { if (router.GetRloc16() == GetRloc16()) @@ -3822,7 +3747,8 @@ void MleRouter::FillConnectivityTlv(ConnectivityTlv &aTlv) } } - aTlv.SetLeaderCost((cost < kMaxRouteCost) ? cost : static_cast(kMaxRouteCost)); + aTlv.SetActiveRouters(mRouterTable.GetActiveRouterCount()); + aTlv.SetLeaderCost(Min(mRouterTable.GetPathCostToLeader(), kMaxRouteCost)); aTlv.SetIdSequence(mRouterTable.GetRouterIdSequence()); aTlv.SetSedBufferSize(OPENTHREAD_CONFIG_DEFAULT_SED_BUFFER_SIZE); aTlv.SetSedDatagramCount(OPENTHREAD_CONFIG_DEFAULT_SED_DATAGRAM_COUNT); diff --git a/src/core/thread/router_table.cpp b/src/core/thread/router_table.cpp index da4539c3a..611004be2 100644 --- a/src/core/thread/router_table.cpp +++ b/src/core/thread/router_table.cpp @@ -88,7 +88,7 @@ Router *RouterTable::AddRouter(uint8_t aRouterId) router->Clear(); router->SetRloc16(Mle::Rloc16FromRouterId(aRouterId)); - router->SetNextHop(Mle::kInvalidRouterId); + router->SetNextHopToInvalid(); mRouterIdMap.SetIndex(aRouterId, mRouters.IndexOf(*router)); @@ -197,8 +197,7 @@ Error RouterTable::Release(uint8_t aRouterId) { if (otherRouter.GetNextHop() == aRouterId) { - otherRouter.SetNextHop(Mle::kInvalidRouterId); - otherRouter.SetCost(0); + otherRouter.SetNextHopToInvalid(); } } @@ -228,8 +227,7 @@ void RouterTable::RemoveRouterLink(Router &aRouter) { if (router.GetNextHop() == aRouter.GetRouterId()) { - router.SetNextHop(Mle::kInvalidRouterId); - router.SetCost(0); + router.SetNextHopToInvalid(); if (GetLinkCost(router) >= Mle::kMaxRouteCost) { @@ -324,7 +322,7 @@ exit: return error; } -Router *RouterTable::GetLeader(void) { return FindRouterById(Get().GetLeaderId()); } +const Router *RouterTable::GetLeader(void) const { return FindRouterById(Get().GetLeaderId()); } uint32_t RouterTable::GetLeaderAge(void) const { @@ -381,6 +379,8 @@ uint8_t RouterTable::GetPathCost(uint16_t aDestRloc16) const const Router *router; const Router *nextHop; + VerifyOrExit(Get().IsAttached()); + if (aDestRloc16 == Get().GetRloc16()) { // Destination is this device, return cost as zero. @@ -388,51 +388,61 @@ uint8_t RouterTable::GetPathCost(uint16_t aDestRloc16) const ExitNow(cost = 0); } - if (Get().IsChild()) - { - // If device is a child, then check if destination is our parent - // and determine cost based on the link quality to parent. - // Otherwise we cannot determine the cost. - - VerifyOrExit(aDestRloc16 == Get().GetParent().GetRloc16()); - cost = CostForLinkQuality(Get().GetParent().GetLinkQualityIn()); - ExitNow(); - } - destRouterId = Mle::RouterIdFromRloc16(aDestRloc16); - if (destRouterId == Mle::RouterIdFromRloc16(Get().GetRloc16())) + router = FindRouterById(destRouterId); + nextHop = (router != nullptr) ? FindNextHopOf(*router) : nullptr; + + if (Get().IsChild()) { - // `aDestRloc16` is a one of device's children. We know the - // device is not a child from `IsChild()` check above and - // that the destination is not device itself (from first `if` - // check above). + const Router &parent = Get().GetParent(); - const Child *child = Get().FindChild(aDestRloc16, Child::kInStateValid); + // If destination is our parent or another child of our + // parent, we use the link cost to our parent. Otherwise we + // check if we have a next hop towards the destination and + // add its cost to the link cost to parent. - VerifyOrExit(child != nullptr); - ExitNow(cost = CostForLinkQuality(child->GetLinkQualityIn())); + VerifyOrExit((destRouterId == parent.GetRouterId()) || (nextHop != nullptr)); + + cost = CostForLinkQuality(parent.GetLinkQualityIn()); + + if (destRouterId != parent.GetRouterId()) + { + cost += router->GetCost(); + } + + // The case where destination itself is a child is handled at + // the end (after `else` block). } - - router = FindRouterById(destRouterId); - - VerifyOrExit(router != nullptr); - - cost = GetLinkCost(*router); - - nextHop = FindNextHopOf(*router); - - if (nextHop != nullptr) + else // Role is router or leader { - // Determine whether direct link or forwarding hop link - // has a lower cost. - cost = Min(cost, static_cast(router->GetCost() + GetLinkCost(*nextHop))); + if (destRouterId == Mle::RouterIdFromRloc16(Get().GetRloc16())) + { + // Destination is a one of our children. + + const Child *child = Get().FindChild(aDestRloc16, Child::kInStateValid); + + VerifyOrExit(child != nullptr); + ExitNow(cost = CostForLinkQuality(child->GetLinkQualityIn())); + } + + VerifyOrExit(router != nullptr); + + cost = GetLinkCost(*router); + + if (nextHop != nullptr) + { + // Determine whether direct link or forwarding hop link + // has a lower cost. + cost = Min(cost, static_cast(router->GetCost() + GetLinkCost(*nextHop))); + } } if (!Mle::IsActiveRouter(aDestRloc16)) { // Destination is a child. we assume best link quality // between destination and its parent router. + cost += kCostForLinkQuality3; } @@ -440,6 +450,11 @@ exit: return cost; } +uint8_t RouterTable::GetPathCostToLeader(void) const +{ + return GetPathCost(Mle::Rloc16FromRouterId(Get().GetLeaderId())); +} + uint16_t RouterTable::GetNextHop(uint16_t aDestRloc16) const { uint16_t nextHopRloc16 = Mle::kInvalidRloc16; @@ -512,7 +527,7 @@ void RouterTable::UpdateRouterIdSet(uint8_t aRouterIdSequence, const Mle::Router Router *router = FindRouterById(routerId); OT_ASSERT(router != nullptr); - router->SetNextHop(Mle::kInvalidRouterId); + router->SetNextHopToInvalid(); RemoveRouterLink(*router); RemoveRouter(*router); } @@ -592,15 +607,8 @@ void RouterTable::UpdateRoutes(const Mle::RouteTlv &aRouteTlv, uint8_t aRouterId resetAdvInterval = true; } - if (router->GetNextHop() != aRouterId) + if (router->SetNextHopAndCost(aRouterId, cost)) { - router->SetNextHop(aRouterId); - changed = true; - } - - if (router->GetCost() != cost) - { - router->SetCost(cost); changed = true; } } @@ -611,8 +619,7 @@ void RouterTable::UpdateRoutes(const Mle::RouteTlv &aRouteTlv, uint8_t aRouterId resetAdvInterval = true; } - router->SetNextHop(Mle::kInvalidRouterId); - router->SetCost(0); + router->SetNextHopToInvalid(); router->SetLastHeard(TimerMilli::GetNow()); changed = true; } @@ -624,8 +631,7 @@ void RouterTable::UpdateRoutes(const Mle::RouteTlv &aRouteTlv, uint8_t aRouterId if (newCost < curCost) { - router->SetNextHop(aRouterId); - router->SetCost(cost); + router->SetNextHopAndCost(aRouterId, cost); changed = true; } } @@ -685,6 +691,44 @@ exit: return changed; } +void RouterTable::UpdateRoutesOnFed(const Mle::RouteTlv &aRouteTlv, uint8_t aParentId) +{ + bool changed = false; + + for (uint8_t routerId = 0, index = 0; routerId <= Mle::kMaxRouterId; + index += aRouteTlv.IsRouterIdSet(routerId) ? 1 : 0, routerId++) + { + Router *router; + uint8_t cost; + uint8_t nextHopId; + + if (!aRouteTlv.IsRouterIdSet(routerId) || (routerId == aParentId)) + { + continue; + } + + router = FindRouterById(routerId); + + if (router == nullptr) + { + continue; + } + + cost = aRouteTlv.GetRouteCost(index); + nextHopId = (cost == 0) ? Mle::kInvalidRouterId : aParentId; + + if (router->SetNextHopAndCost(nextHopId, cost)) + { + changed = true; + } + } + + if (changed) + { + LogRouteTable(); + } +} + void RouterTable::FillRouteTlv(Mle::RouteTlv &aRouteTlv, const Neighbor *aNeighbor) const { uint8_t routerIdSequence = mRouterIdSequence; @@ -856,6 +900,10 @@ void RouterTable::LogRouteTable(void) const { string.Append(" - me"); } + else if (Get().IsChild() && (router.GetRloc16() == Get().GetParent().GetRloc16())) + { + string.Append(" - parent"); + } else { if (router.IsStateValid()) diff --git a/src/core/thread/router_table.hpp b/src/core/thread/router_table.hpp index 99e42e182..e601a8cb3 100644 --- a/src/core/thread/router_table.hpp +++ b/src/core/thread/router_table.hpp @@ -124,7 +124,15 @@ public: * @returns A pointer to the Leader in the Thread network. * */ - Router *GetLeader(void); + Router *GetLeader(void) { return AsNonConst(AsConst(this)->GetLeader()); } + + /** + * This method returns the leader in the Thread network. + * + * @returns A pointer to the Leader in the Thread network. + * + */ + const Router *GetLeader(void) const; /** * This method returns the leader's age in seconds, i.e., seconds since the last Router ID Sequence update. @@ -164,6 +172,14 @@ public: */ uint8_t GetPathCost(uint16_t aDestRloc16) const; + /** + * This method returns the mesh path cost to leader. + * + * @returns The path cost to leader. + * + */ + uint8_t GetPathCostToLeader(void) const; + /** * This method determines the next hop towards an RLOC16 destination. * @@ -325,6 +341,17 @@ public: */ void UpdateRoutes(const Mle::RouteTlv &aRouteTlv, uint8_t aRouterId); + /** + * This method updates the routes on an FED based on a received `RouteTlv` from the parent. + * + * This method MUST be called when device is an FED child and @p aRouteTlv is received from its current parent. + * + * @param[in] aRouteTlv The received `RouteTlv` from parent. + * @param[in] aParentId The Router ID of parent. + * + */ + void UpdateRoutesOnFed(const Mle::RouteTlv &aRouteTlv, uint8_t aParentId); + /** * This method gets the allocated Router ID set. * diff --git a/src/core/thread/topology.cpp b/src/core/thread/topology.cpp index 3869a7158..15023af5f 100644 --- a/src/core/thread/topology.cpp +++ b/src/core/thread/topology.cpp @@ -567,4 +567,25 @@ void Parent::Clear(void) Init(instance); } +bool Router::SetNextHopAndCost(uint8_t aNextHop, uint8_t aCost) +{ + bool changed = false; + + if (mNextHop != aNextHop) + { + mNextHop = aNextHop; + changed = true; + } + + if (mCost != aCost) + { + mCost = aCost; + changed = true; + } + + return changed; +} + +bool Router::SetNextHopToInvalid(void) { return SetNextHopAndCost(Mle::kInvalidRouterId, 0); } + } // namespace ot diff --git a/src/core/thread/topology.hpp b/src/core/thread/topology.hpp index 4cd5cca44..52be960a3 100644 --- a/src/core/thread/topology.hpp +++ b/src/core/thread/topology.hpp @@ -1401,14 +1401,6 @@ public: */ uint8_t GetNextHop(void) const { return mNextHop; } - /** - * This method sets the router ID of the next hop to this router. - * - * @param[in] aRouterId The router ID of the next hop to this router. - * - */ - void SetNextHop(uint8_t aRouterId) { mNextHop = aRouterId; } - /** * This method gets the link quality out value for this router. * @@ -1442,12 +1434,25 @@ public: uint8_t GetCost(void) const { return mCost; } /** - * This method sets the router cost to this router. + * This method sets the next hop and cost to this router. * - * @param[in] aCost The router cost to this router. + * @param[in] aNextHop The Router ID of the next hop to this router. + * @param[in] aCost The cost to this router. + * + * @retval TRUE If there was a change, i.e., @p aNextHop or @p aCost were different from their previous values. + * @retval FALSE If no change to next hop and cost values (new values are the same as before). * */ - void SetCost(uint8_t aCost) { mCost = aCost; } + bool SetNextHopAndCost(uint8_t aNextHop, uint8_t aCost); + + /** + * This method sets the next hop to this router as invalid and clears the cost. + * + * @retval TRUE If there was a change (next hop was valid before). + * @retval FALSE No change to next hop (next hop was invalid before). + * + */ + bool SetNextHopToInvalid(void); private: uint8_t mNextHop; ///< The next hop towards this router diff --git a/tests/toranj/cli/test-008-multicast-traffic.py b/tests/toranj/cli/test-008-multicast-traffic.py index 01c8cd2d6..7623a9c5a 100755 --- a/tests/toranj/cli/test-008-multicast-traffic.py +++ b/tests/toranj/cli/test-008-multicast-traffic.py @@ -97,6 +97,7 @@ r2.join(r1) r3.join(r2) r4.join(r3) fed.join(r1, cli.JOIN_TYPE_REED) + sed.join(r3, cli.JOIN_TYPE_SLEEPY_END_DEVICE) sed.set_pollperiod(600)