From bc37f184bbc1385ec70bb3e3f2a4bea3885e6691 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Fri, 2 Jan 2026 22:33:43 -0800 Subject: [PATCH] Prevent lockup when tuning differential pair length Fix freezes caused excessive branching. Replaced the recursive branching tracking with iterative joint mapping and a timeout fail-safe. Fixes https://gitlab.com/kicad/code/kicad/-/issues/22041 --- common/advanced_config.cpp | 5 + include/advanced_config.h | 12 ++ pcbnew/router/pns_dp_meander_placer.cpp | 12 ++ pcbnew/router/pns_topology.cpp | 178 +++++++++++++++--------- pcbnew/router/pns_topology.h | 2 +- 5 files changed, 139 insertions(+), 70 deletions(-) diff --git a/common/advanced_config.cpp b/common/advanced_config.cpp index 9d9583e0e7..ea1dc12fd7 100644 --- a/common/advanced_config.cpp +++ b/common/advanced_config.cpp @@ -137,6 +137,7 @@ static const wxChar SimulatorMultiRunCombinationLimit[] = wxT( "SimulatorMultiRu static const wxChar GitIconRefreshInterval[] = wxT( "GitIconRefreshInterval" ); static const wxChar MaxPastedTextLength[] = wxT( "MaxPastedTextLength" ); static const wxChar PNSProcessClusterTimeout[] = wxT( "PNSProcessClusterTimeout" ); +static const wxChar FollowBranchTimeout[] = wxT( "FollowBranchTimeoutMs" ); static const wxChar ImportSkipComponentBodies[] = wxT( "ImportSkipComponentBodies" ); static const wxChar ScreenDPI[] = wxT( "ScreenDPI" ); static const wxChar EnableVariantsUI[] = wxT( "EnableVariantsUI" ); @@ -324,6 +325,7 @@ ADVANCED_CFG::ADVANCED_CFG() m_MaxPastedTextLength = 100; m_PNSProcessClusterTimeout = 100; // Default: 100 ms + m_FollowBranchTimeout = 500; // Default: 500 ms m_ImportSkipComponentBodies = false; @@ -630,6 +632,9 @@ void ADVANCED_CFG::loadSettings( wxConfigBase& aCfg ) m_entries.push_back( std::make_unique( true, AC_KEYS::PNSProcessClusterTimeout, &m_PNSProcessClusterTimeout, 100, 10, 10000 ) ); + m_entries.push_back( std::make_unique( true, AC_KEYS::FollowBranchTimeout, + &m_FollowBranchTimeout, 500, 50, 5000 ) ); + m_entries.push_back( std::make_unique( true, AC_KEYS::ImportSkipComponentBodies, &m_ImportSkipComponentBodies, m_ImportSkipComponentBodies ) ); diff --git a/include/advanced_config.h b/include/advanced_config.h index bda2ef6409..6f6117e34f 100644 --- a/include/advanced_config.h +++ b/include/advanced_config.h @@ -851,6 +851,18 @@ public: */ int m_PNSProcessClusterTimeout; + /** + * Timeout for the PNS router's followBranch path search, in milliseconds. + * + * This limits how long the router will spend searching for the longest path + * through a complex track topology before returning the best path found so far. + * + * Setting name: "FollowBranchTimeoutMs" + * Valid values: 50 to 5000 + * Default value: 500 + */ + int m_FollowBranchTimeout; + /** * Skip importing component bodies when importing some format files, such as Altium. * diff --git a/pcbnew/router/pns_dp_meander_placer.cpp b/pcbnew/router/pns_dp_meander_placer.cpp index 9efbd6ad25..98e9fb6b7e 100644 --- a/pcbnew/router/pns_dp_meander_placer.cpp +++ b/pcbnew/router/pns_dp_meander_placer.cpp @@ -220,6 +220,18 @@ bool DP_MEANDER_PLACER::Move( const VECTOR2I& aP, ITEM* aEndItem ) m_originPair.CP().Split( m_currentStart, aP, preP, tunedP, postP ); m_originPair.CN().Split( m_currentStart, aP, preN, tunedN, postN ); + // Bail out early if the tuned sections are empty (issue #22041). This can happen when the + // split points are too close together or outside the line chain. + if( tunedP.PointCount() == 0 || tunedN.PointCount() == 0 ) + { + m_finalShapeP = m_originPair.CP(); + m_finalShapeN = m_originPair.CN(); + m_lastLength = origPathLength(); + m_lastStatus = TOO_SHORT; + + return false; + } + auto updateStatus = [&]() { diff --git a/pcbnew/router/pns_topology.cpp b/pcbnew/router/pns_topology.cpp index 7e61e61c28..1bdd46ce6a 100644 --- a/pcbnew/router/pns_topology.cpp +++ b/pcbnew/router/pns_topology.cpp @@ -21,6 +21,11 @@ #include +#include +#include + +#include + #include "pns_line.h" #include "pns_segment.h" #include "pns_arc.h" @@ -203,106 +208,141 @@ ITEM* TOPOLOGY::NearestUnconnectedItem( const JOINT* aStart, int* aAnchor, int a } -TOPOLOGY::PATH_RESULT TOPOLOGY::followBranch( const JOINT* aJoint, LINKED_ITEM* aPrev, +TOPOLOGY::PATH_RESULT TOPOLOGY::followBranch( const JOINT* aStartJoint, LINKED_ITEM* aPrev, std::set& aVisited, bool aFollowLockedSegments ) { - static int depth = 0; - depth++; - - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: joint at (%d,%d), prev=%p" ), - depth, aJoint->Pos().x, aJoint->Pos().y, aPrev ); + using clock = std::chrono::steady_clock; PATH_RESULT best; + best.m_end = aStartJoint; - ITEM* via = nullptr; - ITEM_SET links( aJoint->CLinks() ); + const int timeoutMs = ADVANCED_CFG::GetCfg().m_FollowBranchTimeout; + auto startTime = clock::now(); - for( ITEM* link : links ) + // State for iterative DFS: current joint, previous item, accumulated path items, + // accumulated length, and the set of visited joints for this path + struct STATE { - if( link->OfKind( ITEM::VIA_T ) && !aVisited.contains( link ) ) - via = link; - } + const JOINT* joint; + LINKED_ITEM* prev; + ITEM_SET pathItems; + int pathLength; + std::set visitedJoints; + ITEM* via; + }; - int branchCount = 0; - for( ITEM* link : links ) + std::stack stateStack; + + // Initialize with starting state + STATE initial; + initial.joint = aStartJoint; + initial.prev = aPrev; + initial.pathLength = 0; + initial.visitedJoints.insert( aStartJoint ); + initial.via = nullptr; + + stateStack.push( std::move( initial ) ); + + while( !stateStack.empty() ) { - if( link->OfKind( ITEM::SEGMENT_T | ITEM::ARC_T ) - && link != aPrev && !aVisited.contains( link ) ) + // Check timeout + auto elapsed = std::chrono::duration_cast( + clock::now() - startTime ).count(); + + if( elapsed > timeoutMs ) { - branchCount++; + wxLogTrace( wxT( "PNS_TUNE" ), + wxT( "followBranch: timeout after %lld ms, returning best path found" ), + elapsed ); + break; + } + + STATE current = std::move( stateStack.top() ); + stateStack.pop(); + + const JOINT* joint = current.joint; + ITEM_SET links( joint->CLinks() ); + + // Check for via at this joint + ITEM* via = nullptr; + + for( ITEM* link : links ) + { + if( link->OfKind( ITEM::VIA_T ) && !aVisited.contains( link ) ) + { + via = link; + break; + } + } + + // Find all unvisited branches from this joint + bool foundBranch = false; + + for( ITEM* link : links ) + { + if( !link->OfKind( ITEM::SEGMENT_T | ITEM::ARC_T ) ) + continue; + + if( link == current.prev ) + continue; + + if( aVisited.contains( link ) ) + continue; LINE l = m_world->AssembleLine( static_cast( link ), nullptr, false, aFollowLockedSegments ); - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: branch %d - assembled line with %d segments" ), - depth, branchCount, l.SegmentCount() ); - - if( l.CPoint( 0 ) != aJoint->Pos() ) - { - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: branch %d - reversing line" ), - depth, branchCount ); + if( l.CPoint( 0 ) != joint->Pos() ) l.Reverse(); - } - const JOINT* next = m_world->FindJoint( l.CLastPoint(), &l ); + const JOINT* nextJoint = m_world->FindJoint( l.CLastPoint(), &l ); - for( LINKED_ITEM* ll : l.Links() ) - aVisited.insert( ll ); + // Skip if we've already visited this joint in the current path + if( current.visitedJoints.count( nextJoint ) ) + continue; + foundBranch = true; + + // Build new state for this branch + STATE nextState; + nextState.joint = nextJoint; + nextState.prev = l.Links().back(); + nextState.pathItems = current.pathItems; + nextState.pathLength = current.pathLength + l.CLine().Length(); + nextState.visitedJoints = current.visitedJoints; + nextState.visitedJoints.insert( nextJoint ); + nextState.via = via; + + // Add via and line to path if( via ) - aVisited.insert( via ); + nextState.pathItems.Add( via ); - int lineLength = l.CLine().Length(); - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: branch %d - line length=%d, recursing..." ), - depth, branchCount, lineLength ); + nextState.pathItems.Add( l ); - PATH_RESULT sub = followBranch( next, l.Links().back(), aVisited, aFollowLockedSegments ); + stateStack.push( std::move( nextState ) ); + } - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: branch %d - sub-path returned with length=%d, %d items" ), - depth, branchCount, sub.m_length, sub.m_items.Size() ); - - ITEM_SET tmp; - if( via ) - tmp.Add( via ); - tmp.Add( l ); - for( ITEM* it : sub.m_items ) - tmp.Add( it ); - - int len = l.CLine().Length() + sub.m_length; - - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: branch %d - total length=%d (line=%d + sub=%d), best so far=%d" ), - depth, branchCount, len, lineLength, sub.m_length, best.m_length ); - - if( len > best.m_length ) + // If no branches found, this is a terminal joint - check if it's the best path + if( !foundBranch ) + { + if( current.pathLength > best.m_length ) { - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: branch %d - NEW BEST! Replacing best path" ), - depth, branchCount ); - best.m_length = len; - best.m_end = sub.m_end; - best.m_items = tmp; + best.m_length = current.pathLength; + best.m_end = joint; + best.m_items = current.pathItems; } - - for( LINKED_ITEM* ll : l.Links() ) - aVisited.erase( ll ); - - if( via ) - aVisited.erase( via ); } } - if( !best.m_end ) - { - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: no branches found, terminal joint" ), depth ); - best.m_end = aJoint; - } + wxLogTrace( wxT( "PNS_TUNE" ), + wxT( "followBranch: completed with best path length=%d, %d items" ), + best.m_length, best.m_items.Size() ); - wxLogTrace( wxT( "PNS_TUNE" ), wxT( "followBranch[depth=%d]: returning best path with length=%d, %d items" ), - depth, best.m_length, best.m_items.Size() ); - - depth--; return best; } + + ITEM_SET TOPOLOGY::followTrivialPath( LINE* aLine2, const JOINT** aTerminalJointA, const JOINT** aTerminalJointB, bool aFollowLockedSegments ) diff --git a/pcbnew/router/pns_topology.h b/pcbnew/router/pns_topology.h index 78b34f7b7a..cb07e6339a 100644 --- a/pcbnew/router/pns_topology.h +++ b/pcbnew/router/pns_topology.h @@ -113,7 +113,7 @@ private: PATH_RESULT() : m_end( nullptr ), m_length( 0 ) {} }; - PATH_RESULT followBranch( const JOINT* aJoint, LINKED_ITEM* aPrev, + PATH_RESULT followBranch( const JOINT* aStartJoint, LINKED_ITEM* aPrev, std::set& aVisited, bool aFollowLockedSegments ); ITEM_SET followTrivialPath( LINE* aLine, const JOINT** aTerminalJointA,