From 511d213462fce5a3c935e6ca98ea9ddaee5eb936 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Fri, 2 Jan 2026 12:11:48 -0800 Subject: [PATCH] Handle bus member rename deterministically Look to see if, in the same bus, the bus member was renamed. if so, take the new name unless one of two things: 1) If the current name already matches a different bus member 2) If a different bus updated the neighbor (this preserves the bus name) Fixes https://gitlab.com/kicad/code/kicad/-/issues/18299 --- eeschema/connection_graph.cpp | 22 +- .../eeschema/issue18299/issue18299.kicad_pro | 421 ++++++++++++++++++ .../eeschema/issue18299/issue18299.kicad_sch | 171 +++++++ qa/data/eeschema/issue18299/test.kicad_sch | 146 ++++++ qa/tests/eeschema/CMakeLists.txt | 1 + .../test_issue18299_bus_member_rename.cpp | 133 ++++++ 6 files changed, 892 insertions(+), 2 deletions(-) create mode 100644 qa/data/eeschema/issue18299/issue18299.kicad_pro create mode 100644 qa/data/eeschema/issue18299/issue18299.kicad_sch create mode 100644 qa/data/eeschema/issue18299/test.kicad_sch create mode 100644 qa/tests/eeschema/test_issue18299_bus_member_rename.cpp diff --git a/eeschema/connection_graph.cpp b/eeschema/connection_graph.cpp index a9503f6666..fabd1b4b49 100644 --- a/eeschema/connection_graph.cpp +++ b/eeschema/connection_graph.cpp @@ -2854,9 +2854,27 @@ void CONNECTION_GRAPH::propagateToNeighbors( CONNECTION_SUBGRAPH* aSubgraph, boo if( neighbor_name == member->Name() ) continue; - // Was this neighbor already updated from a different sheet? Don't rename it again + // Was this neighbor already updated from a different sheet? Don't rename it again, + // unless this same parent bus updated it and the bus member name has since changed + // (which can happen when a bus member is renamed via stale member update, issue #18299). if( neighbor_conn->Sheet() != neighbor->m_sheet ) - continue; + { + // If the neighbor's connection sheet doesn't match this parent bus's sheet, + // it was updated by a different bus entirely. Don't override. + if( neighbor_conn->Sheet() != parent->Sheet() ) + continue; + + // If the neighbor's connection sheet matches this parent bus's sheet but + // the names differ, check if the neighbor's current name still matches + // a member of this bus. If it does, the neighbor was updated by a different + // member of this same bus and we should preserve that (determinism). + // If it doesn't match any member, the bus member was renamed and we should update. + SCH_CONNECTION temp( nullptr, neighbor->m_sheet ); + temp.ConfigureFromLabel( neighbor_name ); + + if( matchBusMember( parent, &temp ) ) + continue; + } // Safety check against infinite recursion wxCHECK2_MSG( neighbor_conn->IsNet(), continue, diff --git a/qa/data/eeschema/issue18299/issue18299.kicad_pro b/qa/data/eeschema/issue18299/issue18299.kicad_pro new file mode 100644 index 0000000000..4e84adbbd7 --- /dev/null +++ b/qa/data/eeschema/issue18299/issue18299.kicad_pro @@ -0,0 +1,421 @@ +{ + "board": { + "3dviewports": [], + "ipc2581": { + "dist": "", + "distpn": "", + "internal_id": "", + "mfg": "", + "mpn": "" + }, + "layer_presets": [], + "viewports": [] + }, + "boards": [], + "cvpcb": { + "equivalence_files": [] + }, + "erc": { + "erc_exclusions": [], + "meta": { + "version": 0 + }, + "pin_map": [ + [ + 0, + 0, + 0, + 0, + 0, + 0, + 1, + 0, + 0, + 0, + 0, + 2 + ], + [ + 0, + 2, + 0, + 1, + 0, + 0, + 1, + 0, + 2, + 2, + 2, + 2 + ], + [ + 0, + 0, + 0, + 0, + 0, + 0, + 1, + 0, + 1, + 0, + 1, + 2 + ], + [ + 0, + 1, + 0, + 0, + 0, + 0, + 1, + 1, + 2, + 1, + 1, + 2 + ], + [ + 0, + 0, + 0, + 0, + 0, + 0, + 1, + 0, + 0, + 0, + 0, + 2 + ], + [ + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 0, + 2 + ], + [ + 1, + 1, + 1, + 1, + 1, + 0, + 1, + 1, + 1, + 1, + 1, + 2 + ], + [ + 0, + 0, + 0, + 1, + 0, + 0, + 1, + 0, + 0, + 0, + 0, + 2 + ], + [ + 0, + 2, + 1, + 2, + 0, + 0, + 1, + 0, + 2, + 2, + 2, + 2 + ], + [ + 0, + 2, + 0, + 1, + 0, + 0, + 1, + 0, + 2, + 0, + 0, + 2 + ], + [ + 0, + 2, + 1, + 1, + 0, + 0, + 1, + 0, + 2, + 0, + 0, + 2 + ], + [ + 2, + 2, + 2, + 2, + 2, + 2, + 2, + 2, + 2, + 2, + 2, + 2 + ] + ], + "rule_severities": { + "bus_definition_conflict": "error", + "bus_entry_needed": "error", + "bus_to_bus_conflict": "error", + "bus_to_net_conflict": "error", + "conflicting_netclasses": "error", + "different_unit_footprint": "error", + "different_unit_net": "error", + "duplicate_reference": "error", + "duplicate_sheet_names": "error", + "endpoint_off_grid": "warning", + "extra_units": "error", + "global_label_dangling": "warning", + "hier_label_mismatch": "error", + "label_dangling": "error", + "lib_symbol_issues": "warning", + "missing_bidi_pin": "warning", + "missing_input_pin": "warning", + "missing_power_pin": "error", + "missing_unit": "warning", + "multiple_net_names": "warning", + "net_not_bus_member": "warning", + "no_connect_connected": "warning", + "no_connect_dangling": "warning", + "pin_not_connected": "error", + "pin_not_driven": "error", + "pin_to_pin": "error", + "power_pin_not_driven": "error", + "similar_labels": "error", + "simulation_model_issue": "ignore", + "unannotated": "error", + "unit_value_mismatch": "error", + "unresolved_variable": "error", + "wire_dangling": "error" + } + }, + "libraries": { + "pinned_footprint_libs": [], + "pinned_symbol_libs": [] + }, + "meta": { + "filename": "issue18229.kicad_pro", + "version": 1 + }, + "net_settings": { + "classes": [ + { + "bus_width": 12, + "clearance": 0.2, + "diff_pair_gap": 0.25, + "diff_pair_via_gap": 0.25, + "diff_pair_width": 0.2, + "line_style": 0, + "microvia_diameter": 0.3, + "microvia_drill": 0.1, + "name": "Default", + "pcb_color": "rgba(0, 0, 0, 0.000)", + "schematic_color": "rgba(0, 0, 0, 0.000)", + "track_width": 0.25, + "via_diameter": 0.8, + "via_drill": 0.4, + "wire_width": 6 + }, + { + "bus_width": 12, + "clearance": 0.2, + "diff_pair_gap": 0.25, + "diff_pair_via_gap": 0.25, + "diff_pair_width": 0.2, + "line_style": 0, + "microvia_diameter": 0.3, + "microvia_drill": 0.1, + "name": "test", + "pcb_color": "rgba(0, 0, 0, 0.000)", + "schematic_color": "rgb(255, 0, 4)", + "track_width": 0.25, + "via_diameter": 0.8, + "via_drill": 0.4, + "wire_width": 6 + } + ], + "meta": { + "version": 3 + }, + "net_colors": null, + "netclass_assignments": null, + "netclass_patterns": [] + }, + "pcbnew": { + "last_paths": { + "gencad": "", + "idf": "", + "netlist": "", + "plot": "", + "pos_files": "", + "specctra_dsn": "", + "step": "", + "svg": "", + "vrml": "" + }, + "page_layout_descr_file": "" + }, + "schematic": { + "annotate_start_num": 0, + "bom_export_filename": "", + "bom_fmt_presets": [], + "bom_fmt_settings": { + "field_delimiter": ",", + "keep_line_breaks": false, + "keep_tabs": false, + "name": "CSV", + "ref_delimiter": ",", + "ref_range_delimiter": "", + "string_delimiter": "\"" + }, + "bom_presets": [], + "bom_settings": { + "exclude_dnp": false, + "fields_ordered": [ + { + "group_by": false, + "label": "Reference", + "name": "Reference", + "show": true + }, + { + "group_by": true, + "label": "Value", + "name": "Value", + "show": true + }, + { + "group_by": false, + "label": "Datasheet", + "name": "Datasheet", + "show": true + }, + { + "group_by": false, + "label": "Footprint", + "name": "Footprint", + "show": true + }, + { + "group_by": false, + "label": "Qty", + "name": "${QUANTITY}", + "show": true + }, + { + "group_by": true, + "label": "DNP", + "name": "${DNP}", + "show": true + } + ], + "filter_string": "", + "group_symbols": true, + "name": "Grouped By Value", + "sort_asc": true, + "sort_field": "Reference" + }, + "connection_grid_size": 50.0, + "drawing": { + "dashed_lines_dash_length_ratio": 12.0, + "dashed_lines_gap_length_ratio": 3.0, + "default_bus_thickness": 12.0, + "default_line_thickness": 6.0, + "default_text_size": 50.0, + "default_wire_thickness": 6.0, + "field_names": [], + "intersheets_ref_own_page": false, + "intersheets_ref_prefix": "", + "intersheets_ref_short": false, + "intersheets_ref_show": true, + "intersheets_ref_suffix": "", + "junction_size_choice": 3, + "label_size_ratio": 0.375, + "operating_point_overlay_i_precision": 3, + "operating_point_overlay_i_range": "~A", + "operating_point_overlay_v_precision": 3, + "operating_point_overlay_v_range": "~V", + "overbar_offset_ratio": 1.23, + "pin_symbol_size": 25.0, + "text_offset_ratio": 0.15 + }, + "legacy_lib_dir": "", + "legacy_lib_list": [], + "meta": { + "version": 1 + }, + "net_format_name": "Allegro (Telesis format)", + "ngspice": { + "fix_include_paths": true, + "fix_passive_vals": true, + "meta": { + "version": 0 + }, + "model_mode": 0, + "workbook_filename": "" + }, + "page_layout_descr_file": "", + "plot_directory": "./", + "spice_adjust_passive_values": true, + "spice_current_sheet_as_root": false, + "spice_external_command": "spice \"%I\"", + "spice_model_current_sheet_as_root": true, + "spice_save_all_currents": false, + "spice_save_all_dissipations": false, + "spice_save_all_voltages": false, + "subpart_first_id": 65, + "subpart_id_separator": 0 + }, + "sheets": [ + [ + "a762d6aa-3004-4a46-aea9-50946c64404d", + "Root" + ], + [ + "5cfd53fb-ab69-498e-a59a-b57a7797f287", + "test2" + ], + [ + "1a6e1223-e16a-4737-b06a-fab3da607b78", + "test1" + ] + ], + "text_variables": {} +} diff --git a/qa/data/eeschema/issue18299/issue18299.kicad_sch b/qa/data/eeschema/issue18299/issue18299.kicad_sch new file mode 100644 index 0000000000..a8af15f77c --- /dev/null +++ b/qa/data/eeschema/issue18299/issue18299.kicad_sch @@ -0,0 +1,171 @@ +(kicad_sch + (version 20231120) + (generator "eeschema") + (generator_version "8.0") + (uuid "a762d6aa-3004-4a46-aea9-50946c64404d") + (paper "A5") + (lib_symbols) + (bus + (pts + (xy 92.71 43.18) (xy 123.19 43.18) + ) + (stroke + (width 0) + (type default) + ) + (uuid "7979a7e0-d4fe-424f-982b-b8968ef91175") + ) + (wire + (pts + (xy 109.22 50.8) (xy 92.71 50.8) + ) + (stroke + (width 0) + (type default) + ) + (uuid "97af57b7-50ad-4f14-8f07-b5e22b4380ab") + ) + (label "bbb" + (at 109.22 50.8 180) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify right bottom) + ) + (uuid "4887ffdb-bc78-41c9-b7b2-8db2e3f486c4") + ) + (label "c{a}" + (at 106.68 43.18 0) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + (uuid "6cbf6b51-71df-45bc-8250-65ecea8211d2") + ) + (sheet + (at 43.18 31.75) + (size 49.53 52.07) + (fields_autoplaced yes) + (stroke + (width 0.1524) + (type solid) + ) + (fill + (color 0 0 0 0.0000) + ) + (uuid "1a6e1223-e16a-4737-b06a-fab3da607b78") + (property "Sheetname" "test1" + (at 43.18 31.0384 0) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + ) + (property "Sheetfile" "test.kicad_sch" + (at 43.18 84.4046 0) + (effects + (font + (size 1.27 1.27) + ) + (justify left top) + ) + ) + (pin "b" input + (at 92.71 50.8 0) + (effects + (font + (size 1.27 1.27) + ) + (justify right) + ) + (uuid "d7ace383-22e7-47e9-93a1-b94e33325c5f") + ) + (pin "{a}" input + (at 92.71 43.18 0) + (effects + (font + (size 1.27 1.27) + ) + (justify right) + ) + (uuid "82f1eb83-196e-406d-9d71-4caeb5475321") + ) + (instances + (project "issue18229" + (path "/a762d6aa-3004-4a46-aea9-50946c64404d" + (page "3") + ) + ) + ) + ) + (sheet + (at 123.19 31.75) + (size 49.53 52.07) + (fields_autoplaced yes) + (stroke + (width 0.1524) + (type solid) + ) + (fill + (color 0 0 0 0.0000) + ) + (uuid "5cfd53fb-ab69-498e-a59a-b57a7797f287") + (property "Sheetname" "test2" + (at 123.19 31.0384 0) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + ) + (property "Sheetfile" "test.kicad_sch" + (at 123.19 84.4046 0) + (effects + (font + (size 1.27 1.27) + ) + (justify left top) + ) + ) + (pin "b" input + (at 123.19 50.8 180) + (effects + (font + (size 1.27 1.27) + ) + (justify left) + ) + (uuid "77077e58-f680-4057-8553-24f15f9df2c9") + ) + (pin "{a}" input + (at 123.19 43.18 180) + (effects + (font + (size 1.27 1.27) + ) + (justify left) + ) + (uuid "f5f10559-aeeb-4550-996a-34afa89dafe9") + ) + (instances + (project "issue18229" + (path "/a762d6aa-3004-4a46-aea9-50946c64404d" + (page "2") + ) + ) + ) + ) + (sheet_instances + (path "/" + (page "1") + ) + ) +) diff --git a/qa/data/eeschema/issue18299/test.kicad_sch b/qa/data/eeschema/issue18299/test.kicad_sch new file mode 100644 index 0000000000..6006a064a4 --- /dev/null +++ b/qa/data/eeschema/issue18299/test.kicad_sch @@ -0,0 +1,146 @@ +(kicad_sch + (version 20231120) + (generator "eeschema") + (generator_version "8.0") + (uuid "290ea578-b262-42be-95fe-3ab10a1a235b") + (paper "A5") + (lib_symbols) + (bus_alias "a" + (members "a" "b" "c") + ) + (bus_entry + (at 104.14 59.69) + (size 2.54 2.54) + (stroke + (width 0) + (type default) + ) + (uuid "6a31f00f-51b5-4761-a53c-08dc0bc6e2cf") + ) + (bus_entry + (at 100.33 59.69) + (size 2.54 2.54) + (stroke + (width 0) + (type default) + ) + (uuid "9007874c-b9eb-4610-a467-3cc76f84cfd0") + ) + (wire + (pts + (xy 102.87 62.23) (xy 102.87 64.77) + ) + (stroke + (width 0) + (type default) + ) + (uuid "2572d2e1-3b53-4ef1-9872-6d508406a9c7") + ) + (bus + (pts + (xy 100.33 59.69) (xy 104.14 59.69) + ) + (stroke + (width 0) + (type default) + ) + (uuid "8710ff94-211b-4ccb-afff-db1dec396d5e") + ) + (wire + (pts + (xy 106.68 62.23) (xy 114.3 62.23) + ) + (stroke + (width 0) + (type default) + ) + (uuid "9bb99e82-aa01-4a17-bbec-34149751e36b") + ) + (wire + (pts + (xy 102.87 64.77) (xy 114.3 64.77) + ) + (stroke + (width 0) + (type default) + ) + (uuid "9c7fb3b8-ac07-415b-8b97-6fd4181be86a") + ) + (bus + (pts + (xy 88.9 59.69) (xy 100.33 59.69) + ) + (stroke + (width 0) + (type default) + ) + (uuid "a6133a5e-5399-40b5-85c1-73b59b0614fe") + ) + (bus + (pts + (xy 104.14 59.69) (xy 107.95 59.69) + ) + (stroke + (width 0) + (type default) + ) + (uuid "c561c3c1-166c-41d6-bb6b-b689770b2904") + ) + (label "{a}" + (at 91.44 59.69 0) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + (uuid "a499ed48-ec37-4bf9-9446-01b65fbb727f") + ) + (label "a" + (at 110.49 62.23 0) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + (uuid "cae16f4a-dab7-43d0-8d1f-ae9b099d0b1f") + ) + (label "b" + (at 102.87 64.77 0) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + (uuid "efb544a5-4e91-4400-a43d-68a4d1a5e2f3") + ) + (hierarchical_label "b" + (shape input) + (at 114.3 64.77 0) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify left) + ) + (uuid "40446e11-1653-4147-8cd1-799b05933432") + ) + (hierarchical_label "{a}" + (shape input) + (at 88.9 59.69 180) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify right) + ) + (uuid "73832671-3211-48b3-b1bc-2ed4154a1c27") + ) +) diff --git a/qa/tests/eeschema/CMakeLists.txt b/qa/tests/eeschema/CMakeLists.txt index 00da9dac04..bc222761aa 100644 --- a/qa/tests/eeschema/CMakeLists.txt +++ b/qa/tests/eeschema/CMakeLists.txt @@ -69,6 +69,7 @@ set( QA_EESCHEMA_SRCS test_annotation_units_integration.cpp test_connectivity_algo.cpp test_issue18119_bus_hierarchy.cpp + test_issue18299_bus_member_rename.cpp test_nc_pin_connectivity.cpp test_crossing_label.cpp test_eagle_plugin.cpp diff --git a/qa/tests/eeschema/test_issue18299_bus_member_rename.cpp b/qa/tests/eeschema/test_issue18299_bus_member_rename.cpp new file mode 100644 index 0000000000..0159caee8a --- /dev/null +++ b/qa/tests/eeschema/test_issue18299_bus_member_rename.cpp @@ -0,0 +1,133 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright The KiCad Developers, see AUTHORS.TXT for contributors. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 3 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, you may find one here: + * https://www.gnu.org/licenses/gpl-3.0.en.html + * or you may search the http://www.gnu.org website for the version 32 license, + * or you may write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ + +/** + * @file test_issue18299_bus_member_rename.cpp + * + * Test for issue #18299: Bus member falls off bus when also individually renamed at a sheet pin. + * + * The test schematic has: + * - Main sheet with two sub-sheet instances (test1, test2) of the same schematic (test.kicad_sch) + * - Bus {a} connects test1 to test2 via sheet pins + * - Bus alias "a" has members: a, b, c + * - Net "b" is a member of bus {a} and is broken out via bus entry in the sub-sheet + * - Net "b" is ALSO individually connected to a sheet pin on test1 (renamed to "bbb" on parent) + * + * Expected: Net "b" inside test1 should be connected to net "b" inside test2 through the bus, + * even though test1's "b" is also individually renamed at a sheet pin. + * + * The bug: When a bus member net is also individually renamed at a sheet pin, the bus member + * connection through the bus is lost. + */ + +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +struct ISSUE18299_FIXTURE +{ + ISSUE18299_FIXTURE() + { } + + SETTINGS_MANAGER m_settingsManager; + std::unique_ptr m_schematic; +}; + + +/** + * Test that bus member connections are maintained through hierarchy even when the member + * is also individually renamed at a sheet pin. + * + * Expected connectivity: + * - Net "b" in test1 sub-sheet should be connected to net "b" in test2 sub-sheet via the bus + * - The individual sheet pin rename ("b" -> "bbb") should not break the bus member connection + */ +BOOST_FIXTURE_TEST_CASE( Issue18299BusMemberRename, ISSUE18299_FIXTURE ) +{ + LOCALE_IO dummy; + + KI_TEST::LoadSchematic( m_settingsManager, "issue18299/issue18299", m_schematic ); + + SCH_SHEET_LIST sheets = m_schematic->BuildSheetListSortedByPageNumbers(); + CONNECTION_GRAPH* graph = m_schematic->ConnectionGraph(); + + // Find the "b" label net codes in each sub-sheet instance + // We're looking for local labels named "b" in the sub-sheets + std::map labelNetCodes; // "sheetname:labelname" -> net code + + for( const auto& [key, subgraphs] : graph->GetNetMap() ) + { + for( CONNECTION_SUBGRAPH* subgraph : subgraphs ) + { + const SCH_SHEET_PATH& sheetPath = subgraph->GetSheet(); + + for( SCH_ITEM* item : subgraph->GetItems() ) + { + if( item->Type() == SCH_LABEL_T ) + { + SCH_LABEL* label = static_cast( item ); + wxString labelText = label->GetText(); + + // Get the sheet instance name from the path + wxString sheetName; + + if( sheetPath.size() > 1 ) + { + // Get the last sheet in the path (the containing sheet) + const SCH_SHEET* lastSheet = sheetPath.Last(); + + if( lastSheet ) + sheetName = lastSheet->GetName(); + } + + if( !sheetName.IsEmpty() && labelText == "b" ) + { + wxString mapKey = sheetName + ":" + labelText; + labelNetCodes[mapKey] = key.Netcode; + } + } + } + } + } + + // Verify we found "b" labels in both sub-sheet instances + BOOST_REQUIRE_MESSAGE( labelNetCodes.count( "test1:b" ), + "Label 'b' should be found in test1 sub-sheet" ); + BOOST_REQUIRE_MESSAGE( labelNetCodes.count( "test2:b" ), + "Label 'b' should be found in test2 sub-sheet" ); + + // The main test: "b" in test1 and "b" in test2 should be on the same net + // because they are both members of bus {a} which connects the two sheets. + // This tests that the individual sheet pin rename ("b" -> "bbb") does not + // break the bus member connection through the bus. + BOOST_CHECK_MESSAGE( labelNetCodes["test1:b"] == labelNetCodes["test2:b"], + "Label 'b' in test1 should be on the same net as label 'b' in test2 " + "(connected through bus {a})" ); +}