From ede500f117dc115fee1dc041a6e3f054643d18c3 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Tue, 12 Jan 2021 23:03:31 -0500 Subject: [PATCH] Don't create multiple markers for the same issue Fixes https://gitlab.com/kicad/code/kicad/-/issues/7016 --- eeschema/connection_graph.cpp | 114 ++++++++++++++++++++++------------ eeschema/connection_graph.h | 32 +++++++++- 2 files changed, 105 insertions(+), 41 deletions(-) diff --git a/eeschema/connection_graph.cpp b/eeschema/connection_graph.cpp index 15ef5b02d9..31890e35a5 100644 --- a/eeschema/connection_graph.cpp +++ b/eeschema/connection_graph.cpp @@ -58,7 +58,7 @@ static const wxChar ConnProfileMask[] = wxT( "CONN_PROFILE" ); static const wxChar ConnTrace[] = wxT( "CONN" ); -bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCreateMarkers ) +bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) { PRIORITY highest_priority = PRIORITY::INVALID; std::vector candidates; @@ -188,7 +188,7 @@ bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCreateMarkers ) m_driver_connection = nullptr; } - if( aCreateMarkers && m_multiple_drivers ) + if( aCheckMultipleDrivers && m_multiple_drivers ) { // First check if all the candidates are actually the same bool same = true; @@ -207,30 +207,12 @@ bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCreateMarkers ) if( !same ) { - wxPoint pos = candidates[0]->Type() == SCH_PIN_T ? - static_cast( candidates[0] )->GetTransformedPosition() : - candidates[0]->GetPosition(); - - wxString msg = wxString::Format( _( "Both %s and %s are attached to the same " - "items; %s will be used in the netlist" ), - first, - GetNameForDriver( second_item ), - first ); - - std::shared_ptr ercItem = ERC_ITEM::Create( ERCE_DRIVER_CONFLICT ); - ercItem->SetItems( candidates[0], second_item ); - ercItem->SetErrorMessage( msg ); - - SCH_MARKER* marker = new SCH_MARKER( ercItem, pos ); - m_sheet.LastScreen()->Append( marker ); - - // If aCreateMarkers is true, then this is part of ERC check, so we - // should return false even if the driver was assigned - return false; + m_first_driver = m_driver; + m_second_driver = second_item; } } - return aCreateMarkers || ( m_driver != nullptr ); + return ( m_driver != nullptr ); } @@ -280,19 +262,14 @@ std::vector CONNECTION_SUBGRAPH::GetBusLabels() const } -const wxString& CONNECTION_SUBGRAPH::GetNameForDriver( SCH_ITEM* aItem ) +wxString CONNECTION_SUBGRAPH::driverName( SCH_ITEM* aItem ) const { - auto it = m_driver_name_cache.find( aItem ); - - if( it != m_driver_name_cache.end() ) - return it->second; - switch( aItem->Type() ) { case SCH_PIN_T: { SCH_PIN* pin = static_cast( aItem ); - m_driver_name_cache[aItem] = pin->GetDefaultNetName( m_sheet ); + return pin->GetDefaultNetName( m_sheet ); break; } @@ -301,8 +278,7 @@ const wxString& CONNECTION_SUBGRAPH::GetNameForDriver( SCH_ITEM* aItem ) case SCH_HIER_LABEL_T: case SCH_SHEET_PIN_T: { - m_driver_name_cache[aItem] = EscapeString( static_cast( aItem )->GetShownText(), - CTX_NETNAME ); + return EscapeString( static_cast( aItem )->GetShownText(), CTX_NETNAME ); break; } @@ -311,10 +287,34 @@ const wxString& CONNECTION_SUBGRAPH::GetNameForDriver( SCH_ITEM* aItem ) break; } + return wxEmptyString; +} + + +const wxString& CONNECTION_SUBGRAPH::GetNameForDriver( SCH_ITEM* aItem ) +{ + auto it = m_driver_name_cache.find( aItem ); + + if( it != m_driver_name_cache.end() ) + return it->second; + + m_driver_name_cache[aItem] = driverName( aItem ); + return m_driver_name_cache.at( aItem ); } +const wxString CONNECTION_SUBGRAPH::GetNameForDriver( SCH_ITEM* aItem ) const +{ + auto it = m_driver_name_cache.find( aItem ); + + if( it != m_driver_name_cache.end() ) + return it->second; + + return driverName( aItem ); +} + + void CONNECTION_SUBGRAPH::Absorb( CONNECTION_SUBGRAPH* aOther ) { wxASSERT( m_sheet == aOther->m_sheet ); @@ -861,7 +861,7 @@ void CONNECTION_GRAPH::buildConnectionGraph() } } - subgraph->ResolveDrivers(); + subgraph->ResolveDrivers( true ); subgraph->m_dirty = false; } @@ -2091,10 +2091,14 @@ int CONNECTION_GRAPH::RunERC() { int error_count = 0; - wxCHECK_MSG( m_schematic, true, "Null m_schematic in CONNECTION_GRAPH::ercCheckLabels" ); + wxCHECK_MSG( m_schematic, true, "Null m_schematic in CONNECTION_GRAPH::RunERC" ); ERC_SETTINGS& settings = m_schematic->ErcSettings(); + // We don't want to run many ERC checks more than once on a given screen even though it may + // represent multiple sheets with multiple subgraphs. We can tell these apart by drivers. + std::set seenDriverInstances; + for( auto&& subgraph : m_subgraphs ) { // Graph is supposed to be up-to-date before calling RunERC() @@ -2103,6 +2107,11 @@ int CONNECTION_GRAPH::RunERC() if( subgraph->m_absorbed ) continue; + if( seenDriverInstances.count( subgraph->m_driver ) ) + continue; + + seenDriverInstances.insert( subgraph->m_driver ); + /** * NOTE: * @@ -2113,14 +2122,13 @@ int CONNECTION_GRAPH::RunERC() * won't actually be connected to bus wires if they aren't in the right * format due to their TestDanglingEnds() implementation. */ - if( settings.IsTestEnabled( ERCE_DRIVER_CONFLICT ) ) { - if( !subgraph->ResolveDrivers( true ) ) + if( !ercCheckMultipleDrivers( subgraph ) ) error_count++; } - else - subgraph->ResolveDrivers( false ); + + subgraph->ResolveDrivers( false ); if( settings.IsTestEnabled( ERCE_BUS_TO_NET_CONFLICT ) ) { @@ -2168,6 +2176,36 @@ int CONNECTION_GRAPH::RunERC() } +bool CONNECTION_GRAPH::ercCheckMultipleDrivers( const CONNECTION_SUBGRAPH* aSubgraph ) +{ + if( !aSubgraph->m_second_driver ) + return true; + + 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(); + + wxString primaryName = aSubgraph->GetNameForDriver( primary ); + 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; +} + + bool CONNECTION_GRAPH::ercCheckBusToNetConflicts( const CONNECTION_SUBGRAPH* aSubgraph ) { auto sheet = aSubgraph->m_sheet; diff --git a/eeschema/connection_graph.h b/eeschema/connection_graph.h index a3b253e8c3..2ad76a3686 100644 --- a/eeschema/connection_graph.h +++ b/eeschema/connection_graph.h @@ -84,7 +84,9 @@ public: m_bus_entry( nullptr ), m_driver( nullptr ), m_driver_connection( nullptr ), - m_hier_parent( nullptr ) + m_hier_parent( nullptr ), + m_first_driver( nullptr ), + m_second_driver( nullptr ) {} ~CONNECTION_SUBGRAPH() = default; @@ -95,10 +97,10 @@ public: * If multiple possible drivers exist, picks one according to the priority. * If multiple "winners" exist, returns false and sets m_driver to nullptr. * - * @param aCreateMarkers controls whether ERC markers should be added for conflicts + * @param aCheckMultipleDrivers controls whether the second driver should be captured for ERC * @return true if m_driver was set, or false if a conflict occurred */ - bool ResolveDrivers( bool aCreateMarkers = false ); + bool ResolveDrivers( bool aCheckMultipleDrivers = false ); /** * Returns the fully-qualified net name for this subgraph (if one exists) @@ -111,6 +113,8 @@ public: /// Returns the candidate net name for a driver const wxString& GetNameForDriver( SCH_ITEM* aItem ); + const wxString GetNameForDriver( SCH_ITEM* aItem ) const; + /// Combines another subgraph on the same sheet into this one. void Absorb( CONNECTION_SUBGRAPH* aOther ); @@ -216,6 +220,19 @@ public: /// A cache of escaped netnames from schematic items 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; + +private: + + wxString driverName( SCH_ITEM* aItem ) const; }; /// Associates a net code with the final name of a net @@ -444,6 +461,15 @@ private: void recacheSubgraphName( CONNECTION_SUBGRAPH* aSubgraph, const wxString& aOldName ); + /** + * If the subgraph has multiple drivers of equal priority that are graphically connected, + * ResolveDrivers() will have stored the second driver for use by this function, which actually + * creates the markers + * @param aSubgraph is the subgraph to examine + * @return true for no errors, false for errors + */ + bool ercCheckMultipleDrivers( const CONNECTION_SUBGRAPH* aSubgraph ); + /** * Checks one subgraph for conflicting connections between net and bus labels *