From d16d2f5a5b1897b0fa3f4e797ff9b9a3a246c09a Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Sat, 17 May 2025 14:19:51 -0400 Subject: [PATCH] Fix a cadre of symbol pin alternate issues. Prevent the file formatter from writing and the file parser from loading symbol pin alternates that have an empty name or when the alternate name is the same as the default pin name. Apparently at some point they were able to be created and saved. Do not allow SCH_PIN::SetAlt() to set an alternate name when the name does not exist in the library symbol pin alternate list. This required fixing the associated QA test. When updating or changing schematic symbols from library, reset the pin alternate to the default when the alternate no longer exists in the symbol. Do not populate pin alternates context menu with duplicate default pin. Clear orphaned pin alternates to the default when saving symbols from symbol editor. --- eeschema/sch_edit_frame.cpp | 12 ++++++++++++ .../sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp | 5 ++++- .../sch_io_kicad_sexpr_lib_cache.cpp | 6 ++++++ eeschema/sch_pin.cpp | 19 +++++++++++++++++++ eeschema/sch_pin.h | 12 +++++++++++- eeschema/tools/sch_edit_tool.cpp | 6 +++++- qa/tests/eeschema/test_sch_pin.cpp | 7 +++++++ 7 files changed, 64 insertions(+), 3 deletions(-) diff --git a/eeschema/sch_edit_frame.cpp b/eeschema/sch_edit_frame.cpp index 3c41068182..e54fa2040e 100644 --- a/eeschema/sch_edit_frame.cpp +++ b/eeschema/sch_edit_frame.cpp @@ -2427,6 +2427,18 @@ void SCH_EDIT_FRAME::SaveSymbolToSchematic( const LIB_SYMBOL& aSymbol, GetCanvas()->GetView()->Update( unit ); } + // Clear any orphaned alternate pins. + for( SCH_PIN* pin : principalSymbol->GetPins() ) + { + wxString altName = pin->GetAlt(); + + if( altName.IsEmpty() ) + continue; + + if( pin->GetAlternates().count( altName ) == 0 ) + pin->SetAlt( wxEmptyString ); + } + if( !commit.Empty() ) commit.Push( _( "Save Symbol to Schematic" ) ); } diff --git a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp index 4c1dd29eb4..cebd70c6f8 100644 --- a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp +++ b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp @@ -805,7 +805,10 @@ void SCH_IO_KICAD_SEXPR::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSche for( const std::unique_ptr& pin : aSymbol->GetRawPins() ) { - if( pin->GetAlt().IsEmpty() ) + // There was a bug introduced somewhere in the original alternated pin code that would + // set the alternate pin to the default pin name which caused a number of library symbol + // comparison issues. Clearing the alternate pin resolves this issue. + if( pin->GetAlt().IsEmpty() || ( pin->GetAlt() == pin->GetBaseName() ) ) { m_out->Print( "(pin %s", m_out->Quotew( pin->GetNumber() ).c_str() ); KICAD_FORMAT::FormatUuid( m_out, pin->m_Uuid ); diff --git a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_lib_cache.cpp b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_lib_cache.cpp index c0764e8158..68d264f44e 100644 --- a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_lib_cache.cpp +++ b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_lib_cache.cpp @@ -488,6 +488,12 @@ void SCH_IO_KICAD_SEXPR_LIB_CACHE::savePin( SCH_PIN* aPin, OUTPUTFORMATTER& aFor for( const std::pair& alt : aPin->GetAlternates() ) { + // There was a bug somewhere in the alternate pin code that allowed pin alternates with no + // name to be saved in library symbols. This strips any invalid alternates just in case + // that code resurfaces. + if( alt.second.m_Name.IsEmpty() ) + continue; + aFormatter.Print( "(alternate %s %s %s)", aFormatter.Quotew( alt.second.m_Name ).c_str(), getPinElectricalTypeToken( alt.second.m_Type ), diff --git a/eeschema/sch_pin.cpp b/eeschema/sch_pin.cpp index 770572d2d0..3c84742753 100644 --- a/eeschema/sch_pin.cpp +++ b/eeschema/sch_pin.cpp @@ -426,6 +426,25 @@ void SCH_PIN::SetName( const wxString& aName ) } +void SCH_PIN::SetAlt( const wxString& aAlt ) +{ + wxString alt = aAlt; + + // Do not set the alternate pin definition to the default pin name. This breaks the library + // symbol comparison for the ERC and the library diff tool. It also incorrectly causes the + // schematic symbol pin alternate to be set. + if( aAlt.IsEmpty() || ( alt == GetBaseName() ) ) + { + m_alt = wxEmptyString; + return; + } + + wxCHECK2_MSG( m_libPin && m_libPin->GetAlternates().count( aAlt ), alt = wxEmptyString, + wxString::Format( wxS( "Pin '%s' does not have an alterate '%s'" ), m_number, aAlt ) ); + + m_alt = aAlt; +} + bool SCH_PIN::IsDangling() const { if( GetType() == ELECTRICAL_PINTYPE::PT_NC || GetType() == ELECTRICAL_PINTYPE::PT_NIC ) diff --git a/eeschema/sch_pin.h b/eeschema/sch_pin.h index 2f1af13e53..454a4d54ce 100644 --- a/eeschema/sch_pin.h +++ b/eeschema/sch_pin.h @@ -150,7 +150,17 @@ public: } wxString GetAlt() const { return m_alt; } - void SetAlt( const wxString& aAlt ) { m_alt = aAlt; } + + /** + * Set the name of the alternate pin. + * + * @note If the alternate pin is the same as the default pin name or does not exist in the + * list of pin alternates, it's set to an empty string which results in the alternate + * being set to the default pin. + * + * @param is the name of the pin alternate in #m_alternates. + */ + void SetAlt( const wxString& aAlt ); /** * Return the pin real orientation (PIN_UP, PIN_DOWN, PIN_RIGHT, PIN_LEFT), diff --git a/eeschema/tools/sch_edit_tool.cpp b/eeschema/tools/sch_edit_tool.cpp index 9fe1dd04eb..a42e352d4c 100644 --- a/eeschema/tools/sch_edit_tool.cpp +++ b/eeschema/tools/sch_edit_tool.cpp @@ -202,13 +202,17 @@ private: wxMenuItem* item = Append( ID_POPUP_SCH_ALT_PIN_FUNCTION, libPin->GetName(), wxEmptyString, wxITEM_CHECK ); - if( pin->GetAlt().IsEmpty() ) + if( pin->GetAlt().IsEmpty() || ( pin->GetAlt() == libPin->GetName() ) ) item->Check( true ); int ii = 1; for( const auto& [ name, definition ] : libPin->GetAlternates() ) { + // The default pin name is set above, avoid setting it again. + if( name == libPin->GetName() ) + continue; + item = Append( ID_POPUP_SCH_ALT_PIN_FUNCTION + ii, name, wxEmptyString, wxITEM_CHECK ); if( name == pin->GetAlt() ) diff --git a/qa/tests/eeschema/test_sch_pin.cpp b/qa/tests/eeschema/test_sch_pin.cpp index 3d2f640161..25c155d43f 100644 --- a/qa/tests/eeschema/test_sch_pin.cpp +++ b/qa/tests/eeschema/test_sch_pin.cpp @@ -26,6 +26,7 @@ // Code under test #include +#include #include #include @@ -116,6 +117,12 @@ BOOST_AUTO_TEST_CASE( Copy ) BOOST_CHECK_EQUAL( copied.GetNumber(), m_lib_pin->GetNumber() ); BOOST_CHECK_EQUAL( copied.GetAlt(), wxEmptyString ); + SCH_PIN::ALT alt; + alt.m_Name = wxS( "alt" ); + alt.m_Shape = GRAPHIC_PINSHAPE::INVERTED; + alt.m_Type = ELECTRICAL_PINTYPE::PT_OUTPUT; + copied.GetAlternates()[ wxS( "alt" ) ] = alt; + // Set some non-default values copied.SetAlt( "alt" );