haskal
/
kicad
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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)
This commit is contained in:
Seth Hillbrand 2023-09-19 15:52:26 -07:00
parent d371bb06ae
commit 7d12e1c4f5
3 changed files with 104 additions and 146 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

214
eeschema/connection_graph.cpp
View File

@ -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<SCH_SHEET_PIN*>( aItem ) );
if( aItem->Type() == SCH_HIER_LABEL_T )
m_hier_ports.erase( static_cast<SCH_HIERLABEL*>( aItem ) );
}
bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers )
{
PRIORITY highest_priority = PRIORITY::INVALID;
std::vector<SCH_ITEM*> candidates;
std::vector<SCH_ITEM*> 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<SCH_PIN*>( a );
SCH_PIN* pb = static_cast<SCH_PIN*>( 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<SCH_ITEM*, decltype( candidate_cmp )> candidates( candidate_cmp );
std::set<SCH_ITEM*, decltype( strong_cmp )> 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<SCH_PIN*>( a );
SCH_PIN* pb = static_cast<SCH_PIN*>( 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::set<std::pair<SCH_SHEET_PAT
aSubgraphs.insert( sg->m_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 );
@ -441,15 +438,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<SCH_SHEET_PIN*>( aItem ) );
m_hier_pins.insert( static_cast<SCH_SHEET_PIN*>( aItem ) );
else if( aItem->Type() == SCH_HIER_LABEL_T )
m_hier_ports.push_back( static_cast<SCH_HIERLABEL*>( aItem ) );
m_hier_ports.insert( static_cast<SCH_HIERLABEL*>( aItem ) );
}
@ -606,6 +603,7 @@ void CONNECTION_GRAPH::Recalculate( const SCH_SHEET_LIST& aSheetList, bool aUnco
PROF_TIMER update_items( "updateItemConnectivity" );
m_sheetList = aSheetList;
std::set<SCH_ITEM*> dirty_items;
for( const SCH_SHEET_PATH& sheet : aSheetList )
{
@ -621,6 +619,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)
@ -653,6 +652,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();
@ -966,8 +968,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 )
@ -1679,10 +1679,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<SCH_CONNECTION*> 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(),
@ -2898,38 +2904,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<SCH_PIN*>( 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<ERC_ITEM> 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 )
@ -2964,7 +2939,6 @@ bool CONNECTION_GRAPH::ercCheckMultipleDrivers( const CONNECTION_SUBGRAPH* aSubg
}
}
}
#endif
return true;
}

23
eeschema/connection_graph.h
View File

@ -86,8 +86,6 @@ public:
m_bus_entry( nullptr ),
m_hier_parent( nullptr ),
m_driver( nullptr ),
m_first_driver( nullptr ),
m_second_driver( nullptr ),
m_no_connect( nullptr ),
m_driver_connection( nullptr )
{}
@ -133,7 +131,7 @@ public:
void UpdateItemConnections();
/// Provides a read-only reference to the items in the subgraph
const std::vector<SCH_ITEM*>& GetItems() const
const std::set<SCH_ITEM*>& GetItems() const
{
return m_items;
}
@ -196,6 +194,8 @@ public:
return m_sheet;
}
void RemoveItem( SCH_ITEM* aItem );
private:
wxString driverName( SCH_ITEM* aItem ) const;
@ -236,7 +236,7 @@ private:
/// Bus entry in graph, if any
SCH_ITEM* m_bus_entry;
std::vector<SCH_ITEM*> m_drivers;
std::set<SCH_ITEM*> m_drivers;
/**
* If a subgraph is a bus, this map contains links between the bus members and any
@ -258,10 +258,10 @@ private:
std::unordered_set<CONNECTION_SUBGRAPH*> > m_bus_parents;
// Cache for lookup of any hierarchical (sheet) pins on this subgraph (for referring down)
std::vector<SCH_SHEET_PIN*> m_hier_pins;
std::set<SCH_SHEET_PIN*> m_hier_pins;
// Cache for lookup of any hierarchical ports on this subgraph (for referring up)
std::vector<SCH_HIERLABEL*> m_hier_ports;
std::set<SCH_HIERLABEL*> 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;
@ -275,17 +275,8 @@ private:
/// Fully-resolved driver for the subgraph (might not exist in this subgraph)
SCH_ITEM* m_driver;
/**
* 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;
/// Contents of the subgraph
std::vector<SCH_ITEM*> m_items;
std::set<SCH_ITEM*> m_items;
/// No-connect item in graph, if any
SCH_ITEM* m_no_connect;

13
eeschema/sch_item.cpp
View File

@ -149,12 +149,6 @@ SCH_CONNECTION* SCH_ITEM::Connection( const SCH_SHEET_PATH* aSheet ) const
if( !IsConnectable() )
return nullptr;
if( IsConnectivityDirty() )
{
wxFAIL_MSG( wxT( "Shouldn't be asking for connection if connectivity is dirty!" ) );
return nullptr;
}
if( !aSheet )
aSheet = &Schematic()->CurrentSheet();
@ -221,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();
@ -247,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 )