From 6b43bb8fe331403ff93a1c884000246d58e55a2e Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Tue, 19 Sep 2023 15:52:26 -0700 Subject: [PATCH] Fix crash in incremental update and bus conn We store our connectivity dirty flag with the SCH_ITEM but we generate SCH_CONNECTION based on the SCH_ITEM and the SCH_SHEET_PATH. For this reason, we need to avoid clearing the connectivity dirty flag until we've finished processing all instances of the SCH_ITEM in the graph This also means that we need to allow getting the SCH_CONNECTION pointer even when the connectivity is still dirty (getting SCH_CONNECTION happens based on SCH_ITEM and SCH_SHEET_PATH, not just SCH_ITEM) (cherry picked from commit 7d12e1c4f56c0c40dc57f299d62fad88a605dd56) --- eeschema/connection_graph.cpp | 214 +++++++++++++++------------------- eeschema/connection_graph.h | 24 ++-- eeschema/sch_item.cpp | 10 +- 3 files changed, 105 insertions(+), 143 deletions(-) diff --git a/eeschema/connection_graph.cpp b/eeschema/connection_graph.cpp index f9ed9323e5..d7fe64150e 100644 --- a/eeschema/connection_graph.cpp +++ b/eeschema/connection_graph.cpp @@ -64,11 +64,79 @@ static const wxChar DanglingProfileMask[] = wxT( "CONN_PROFILE" ); static const wxChar ConnTrace[] = wxT( "CONN" ); +void CONNECTION_SUBGRAPH::RemoveItem( SCH_ITEM* aItem ) +{ + m_items.erase( aItem ); + m_drivers.erase( aItem ); + + if( aItem == m_driver ) + { + m_driver = nullptr; + m_driver_connection = nullptr; + } + + if( aItem->Type() == SCH_SHEET_PIN_T ) + m_hier_pins.erase( static_cast( aItem ) ); + + if( aItem->Type() == SCH_HIER_LABEL_T ) + m_hier_ports.erase( static_cast( aItem ) ); +} + + bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) { - PRIORITY highest_priority = PRIORITY::INVALID; - std::vector candidates; - std::vector strong_drivers; + auto candidate_cmp = [&]( SCH_ITEM* a, SCH_ITEM* b ) -> bool + { + // meet irreflexive requirements of std::sort + if( a == b ) + return false; + + SCH_CONNECTION* ac = a->Connection( &m_sheet ); + SCH_CONNECTION* bc = b->Connection( &m_sheet ); + + // Ensure we don't pick the subset over the superset + if( ac->IsBus() && bc->IsBus() ) + return bc->IsSubsetOf( ac ); + + // Ensure we don't pick a hidden power pin on a regular symbol over + // one on a power symbol + if( a->Type() == SCH_PIN_T && b->Type() == SCH_PIN_T ) + { + SCH_PIN* pa = static_cast( a ); + SCH_PIN* pb = static_cast( b ); + + bool aPower = pa->GetLibPin()->GetParent()->IsPower(); + bool bPower = pb->GetLibPin()->GetParent()->IsPower(); + + if( aPower && !bPower ) + return true; + else if( bPower && !aPower ) + return false; + } + + const wxString& a_name = GetNameForDriver( a ); + const wxString& b_name = GetNameForDriver( b ); + bool a_lowQualityName = a_name.Contains( "-Pad" ); + bool b_lowQualityName = b_name.Contains( "-Pad" ); + + if( a_lowQualityName && !b_lowQualityName ) + return false; + else if( b_lowQualityName && !a_lowQualityName ) + return true; + else + return a_name < b_name; + }; + + auto strong_cmp = [this]( SCH_ITEM* a, SCH_ITEM* b ) -> bool + { + const wxString& a_name = GetNameForDriver( a ); + const wxString& b_name = GetNameForDriver( b ); + return a_name < b_name; + }; + + PRIORITY highest_priority = PRIORITY::INVALID; + std::set candidates( candidate_cmp ); + std::set strong_drivers( strong_cmp ); m_driver = nullptr; @@ -88,17 +156,17 @@ bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) continue; if( item_priority >= PRIORITY::HIER_LABEL ) - strong_drivers.push_back( item ); + strong_drivers.insert( item ); if( item_priority > highest_priority ) { candidates.clear(); - candidates.push_back( item ); + candidates.insert( item ); highest_priority = item_priority; } else if( !candidates.empty() && ( item_priority == highest_priority ) ) { - candidates.push_back( item ); + candidates.insert( item ); } } @@ -128,56 +196,10 @@ bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) } } } - else - { - // For all other driver types, sort by quality of name - std::sort( candidates.begin(), candidates.end(), - [&]( SCH_ITEM* a, SCH_ITEM* b ) -> bool - { - // meet irreflexive requirements of std::sort - if( a == b ) - return false; - - SCH_CONNECTION* ac = a->Connection( &m_sheet ); - SCH_CONNECTION* bc = b->Connection( &m_sheet ); - - // Ensure we don't pick the subset over the superset - if( ac->IsBus() && bc->IsBus() ) - return bc->IsSubsetOf( ac ); - - // Ensure we don't pick a hidden power pin on a regular symbol over - // one on a power symbol - if( a->Type() == SCH_PIN_T && b->Type() == SCH_PIN_T ) - { - SCH_PIN* pa = static_cast( a ); - SCH_PIN* pb = static_cast( b ); - - bool aPower = pa->GetLibPin()->GetParent()->IsPower(); - bool bPower = pb->GetLibPin()->GetParent()->IsPower(); - - if( aPower && !bPower ) - return true; - else if( bPower && !aPower ) - return false; - } - - const wxString& a_name = GetNameForDriver( a ); - const wxString& b_name = GetNameForDriver( b ); - bool a_lowQualityName = a_name.Contains( "-Pad" ); - bool b_lowQualityName = b_name.Contains( "-Pad" ); - - if( a_lowQualityName && !b_lowQualityName ) - return false; - else if( b_lowQualityName && !a_lowQualityName ) - return true; - else - return a_name < b_name; - } ); - } } if( !m_driver ) - m_driver = candidates[0]; + m_driver = *candidates.begin(); } if( strong_drivers.size() > 1 ) @@ -185,7 +207,10 @@ bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) // Drop weak drivers if( m_strong_driver ) - m_drivers = strong_drivers; + { + m_drivers.clear(); + m_drivers.insert( strong_drivers.begin(), strong_drivers.end() ); + } // Cache driver connection if( m_driver ) @@ -200,30 +225,6 @@ bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) m_driver_connection = nullptr; } - if( aCheckMultipleDrivers && m_multiple_drivers ) - { - // First check if all the candidates are actually the same - bool same = true; - const wxString& first = GetNameForDriver( candidates[0] ); - SCH_ITEM* second_item = nullptr; - - for( unsigned i = 1; i < candidates.size(); i++ ) - { - if( GetNameForDriver( candidates[i] ) != first ) - { - second_item = candidates[i]; - same = false; - break; - } - } - - if( !same ) - { - m_first_driver = m_driver; - m_second_driver = second_item; - } - } - return ( m_driver != nullptr ); } @@ -242,11 +243,7 @@ void CONNECTION_SUBGRAPH::getAllConnectedItems( std::setm_absorbed_subgraphs.begin(), sg->m_absorbed_subgraphs.end() ); for( SCH_ITEM* item : sg->m_items ) - { - item->Connection( &m_sheet )->Reset(); - item->ConnectedItems( m_sheet ).clear(); aItems.emplace( m_sheet, item ); - } for( CONNECTION_SUBGRAPH* child_sg : sg->m_hier_children ) child_sg->getAllConnectedItems( aItems, aSubgraphs ); @@ -436,15 +433,15 @@ void CONNECTION_SUBGRAPH::Absorb( CONNECTION_SUBGRAPH* aOther ) void CONNECTION_SUBGRAPH::AddItem( SCH_ITEM* aItem ) { - m_items.push_back( aItem ); + m_items.insert( aItem ); if( aItem->Connection( &m_sheet )->IsDriver() ) - m_drivers.push_back( aItem ); + m_drivers.insert( aItem ); if( aItem->Type() == SCH_SHEET_PIN_T ) - m_hier_pins.push_back( static_cast( aItem ) ); + m_hier_pins.insert( static_cast( aItem ) ); else if( aItem->Type() == SCH_HIER_LABEL_T ) - m_hier_ports.push_back( static_cast( aItem ) ); + m_hier_ports.insert( static_cast( aItem ) ); } @@ -601,6 +598,7 @@ void CONNECTION_GRAPH::Recalculate( const SCH_SHEET_LIST& aSheetList, bool aUnco PROF_TIMER update_items( "updateItemConnectivity" ); m_sheetList = aSheetList; + std::set dirty_items; for( const SCH_SHEET_PATH& sheet : aSheetList ) { @@ -616,6 +614,7 @@ void CONNECTION_GRAPH::Recalculate( const SCH_SHEET_LIST& aSheetList, bool aUnco wxLogTrace( ConnTrace, wxT( "Adding item %s to connectivity graph update" ), item->GetTypeDesc() ); items.push_back( item ); + dirty_items.insert( item ); } // Ensure the hierarchy info stored in SCREENS is built and up to date // (multi-unit symbols) @@ -648,6 +647,9 @@ void CONNECTION_GRAPH::Recalculate( const SCH_SHEET_LIST& aSheetList, bool aUnco } } + for( SCH_ITEM* item : dirty_items ) + item->SetConnectivityDirty( false ); + if( wxLog::IsAllowedTraceMask( DanglingProfileMask ) ) update_items.Show(); @@ -957,8 +959,6 @@ void CONNECTION_GRAPH::updateItemConnectivity( const SCH_SHEET_PATH& aSheet, for( const VECTOR2I& point : points ) connection_map[ point ].push_back( item ); } - - item->SetConnectivityDirty( false ); } for( const auto& it : connection_map ) @@ -1663,10 +1663,16 @@ void CONNECTION_GRAPH::processSubGraphs() // by the chosen driver of the subgraph, so we need to create a dummy connection add_connections_to_check( subgraph ); + std::set checked_connections; + for( unsigned i = 0; i < connections_to_check.size(); i++ ) { auto member = connections_to_check[i]; + // Don't check the same connection twice + if( !checked_connections.insert( member.get() ).second ) + continue; + if( member->IsBus() ) { connections_to_check.insert( connections_to_check.end(), @@ -2867,38 +2873,7 @@ int CONNECTION_GRAPH::RunERC() bool CONNECTION_GRAPH::ercCheckMultipleDrivers( const CONNECTION_SUBGRAPH* aSubgraph ) { wxCHECK( aSubgraph, false ); - /* - * This was changed late in 6.0 to fix https://gitlab.com/kicad/code/kicad/-/issues/9367 - * so I'm going to leave the original code in for just a little while. If anyone comes - * across this in 7.0 development (or later), feel free to delete. - */ -#if 0 - if( aSubgraph->m_second_driver ) - { - SCH_ITEM* primary = aSubgraph->m_first_driver; - SCH_ITEM* secondary = aSubgraph->m_second_driver; - wxPoint pos = primary->Type() == SCH_PIN_T ? - static_cast( primary )->GetTransformedPosition() : - primary->GetPosition(); - - const wxString& primaryName = aSubgraph->GetNameForDriver( primary ); - const wxString& secondaryName = aSubgraph->GetNameForDriver( secondary ); - - wxString msg = wxString::Format( _( "Both %s and %s are attached to the same " - "items; %s will be used in the netlist" ), - primaryName, secondaryName, primaryName ); - - std::shared_ptr ercItem = ERC_ITEM::Create( ERCE_DRIVER_CONFLICT ); - ercItem->SetItems( primary, secondary ); - ercItem->SetErrorMessage( msg ); - - SCH_MARKER* marker = new SCH_MARKER( ercItem, pos ); - aSubgraph->m_sheet.LastScreen()->Append( marker ); - - return false; - } -#else if( aSubgraph->m_multiple_drivers ) { for( SCH_ITEM* driver : aSubgraph->m_drivers ) @@ -2933,7 +2908,6 @@ bool CONNECTION_GRAPH::ercCheckMultipleDrivers( const CONNECTION_SUBGRAPH* aSubg } } } -#endif return true; } diff --git a/eeschema/connection_graph.h b/eeschema/connection_graph.h index 7bd1ad5cdd..dc2880df62 100644 --- a/eeschema/connection_graph.h +++ b/eeschema/connection_graph.h @@ -86,9 +86,7 @@ public: m_bus_entry( nullptr ), m_driver( nullptr ), m_driver_connection( nullptr ), - m_hier_parent( nullptr ), - m_first_driver( nullptr ), - m_second_driver( nullptr ) + m_hier_parent( nullptr ) {} ~CONNECTION_SUBGRAPH() = default; @@ -130,7 +128,7 @@ public: void UpdateItemConnections(); /// Provides a read-only reference to the items in the subgraph - const std::vector& GetItems() const + const std::set& GetItems() const { return m_items; } @@ -162,6 +160,8 @@ public: return PRIORITY::NONE; } + void RemoveItem( SCH_ITEM* aItem ); + private: wxString driverName( SCH_ITEM* aItem ) const; @@ -206,9 +206,9 @@ public: /// Bus entry in graph, if any SCH_ITEM* m_bus_entry; - std::vector m_items; + std::set m_items; - std::vector m_drivers; + std::set m_drivers; SCH_ITEM* m_driver; @@ -237,10 +237,10 @@ public: std::unordered_set > m_bus_parents; // Cache for lookup of any hierarchical (sheet) pins on this subgraph (for referring down) - std::vector m_hier_pins; + std::set m_hier_pins; // Cache for lookup of any hierarchical ports on this subgraph (for referring up) - std::vector m_hier_ports; + std::set m_hier_ports; // If not null, this indicates the subgraph on a higher level sheet that is linked to this one CONNECTION_SUBGRAPH* m_hier_parent; @@ -251,14 +251,6 @@ public: /// A cache of escaped netnames from schematic items mutable std::unordered_map m_driver_name_cache; - /** - * Stores the primary driver for the multiple drivers ERC check. This is the chosen driver - * before subgraphs are absorbed (so m_driver may be different) - */ - SCH_ITEM* m_first_driver; - - /// Used for multiple drivers ERC message; stores the second possible driver (or nullptr) - SCH_ITEM* m_second_driver; }; struct NET_NAME_CODE_CACHE_KEY diff --git a/eeschema/sch_item.cpp b/eeschema/sch_item.cpp index 188bf022e6..0d0b09c38e 100644 --- a/eeschema/sch_item.cpp +++ b/eeschema/sch_item.cpp @@ -149,9 +149,6 @@ SCH_CONNECTION* SCH_ITEM::Connection( const SCH_SHEET_PATH* aSheet ) const if( !IsConnectable() ) return nullptr; - wxCHECK_MSG( !IsConnectivityDirty(), nullptr, - wxT( "Shouldn't be asking for connection if connectivity is dirty!" ) ); - if( !aSheet ) aSheet = &Schematic()->CurrentSheet(); @@ -218,10 +215,11 @@ void SCH_ITEM::AddConnectionTo( const SCH_SHEET_PATH& aSheet, SCH_ITEM* aItem ) SCH_CONNECTION* SCH_ITEM::InitializeConnection( const SCH_SHEET_PATH& aSheet, CONNECTION_GRAPH* aGraph ) { - SetConnectivityDirty( false ); - SCH_CONNECTION* connection = Connection( &aSheet ); + // N.B. Do not clear the dirty connectivity flag here because we may need + // to create a connection for a different sheet, and we don't want to + // skip the connection creation because the flag is cleared. if( connection ) { connection->Reset(); @@ -244,8 +242,6 @@ SCH_CONNECTION* SCH_ITEM::GetOrInitConnection( const SCH_SHEET_PATH& aSheet, if( !IsConnectable() ) return nullptr; - SetConnectivityDirty( false ); - SCH_CONNECTION* connection = Connection( &aSheet ); if( connection )