From 6313b58ad683293e5c1dd149c8e7db1b7b8a2ac1 Mon Sep 17 00:00:00 2001 From: John Beard Date: Fri, 17 May 2024 20:28:22 +0800 Subject: [PATCH] Break out junction analysis so it can be tested in isolation There are some corner cases with bus entries that could do with care and it's a bit tricky to keep all the cases in mind at once! --- eeschema/CMakeLists.txt | 3 +- eeschema/junction_helpers.cpp | 111 +++++++ eeschema/junction_helpers.h | 24 ++ eeschema/sch_screen.cpp | 145 ++------- qa/tests/eeschema/CMakeLists.txt | 2 +- qa/tests/eeschema/test_junction_helpers.cpp | 318 ++++++++++++++++++++ 6 files changed, 476 insertions(+), 127 deletions(-) create mode 100644 eeschema/junction_helpers.cpp create mode 100644 eeschema/junction_helpers.h create mode 100644 qa/tests/eeschema/test_junction_helpers.cpp diff --git a/eeschema/CMakeLists.txt b/eeschema/CMakeLists.txt index 4fa7fa3be9..654c01751e 100644 --- a/eeschema/CMakeLists.txt +++ b/eeschema/CMakeLists.txt @@ -356,11 +356,12 @@ set( EESCHEMA_SRCS files-io.cpp generate_alias_info.cpp gfx_import_utils.cpp - picksymbol.cpp + junction_helpers.cpp lib_symbol.cpp libarch.cpp menubar.cpp net_navigator.cpp + picksymbol.cpp pin_numbers.cpp pin_type.cpp project_sch.cpp diff --git a/eeschema/junction_helpers.cpp b/eeschema/junction_helpers.cpp new file mode 100644 index 0000000000..372b531531 --- /dev/null +++ b/eeschema/junction_helpers.cpp @@ -0,0 +1,111 @@ +#include "junction_helpers.h" + +#include + +using namespace JUNCTION_HELPERS; + +POINT_INFO JUNCTION_HELPERS::AnalyzePoint( const EE_RTREE& aItems, const VECTOR2I& aPosition, + bool aBreakCrossings ) +{ + enum layers + { + WIRES = 0, + BUSES + }; + + POINT_INFO info {}; + info.hasBusEntry = false; + info.hasExplicitJunctionDot = false; + info.isJunction = false; + + bool breakLines[2] = { false }; + std::unordered_set exitAngles[2]; + std::vector midPointLines[2]; + + // A pin at 90° still shouldn't match a line at 90° so just give pins unique numbers + int uniqueAngle = 10000; + + for( const SCH_ITEM* item : aItems.Overlapping( aPosition ) ) + { + if( item->GetEditFlags() & STRUCT_DELETED ) + continue; + + switch( item->Type() ) + { + case SCH_JUNCTION_T: + if( item->HitTest( aPosition, -1 ) ) + info.hasExplicitJunctionDot = true; + + break; + + case SCH_LINE_T: + { + const SCH_LINE* line = static_cast( item ); + int layer; + + if( line->GetStartPoint() == line->GetEndPoint() ) + break; + else if( line->GetLayer() == LAYER_WIRE ) + layer = WIRES; + else if( line->GetLayer() == LAYER_BUS ) + layer = BUSES; + else + break; + + if( line->IsConnected( aPosition ) ) + { + breakLines[layer] = true; + exitAngles[layer].insert( line->GetAngleFrom( aPosition ) ); + } + else if( line->HitTest( aPosition, -1 ) ) + { + if( aBreakCrossings ) + breakLines[layer] = true; + + // Defer any line midpoints until we know whether or not we're breaking them + midPointLines[layer].push_back( line ); + } + } + break; + + case SCH_BUS_WIRE_ENTRY_T: + if( item->IsConnected( aPosition ) ) + { + breakLines[BUSES] = true; + exitAngles[BUSES].insert( uniqueAngle++ ); + breakLines[WIRES] = true; + exitAngles[WIRES].insert( uniqueAngle++ ); + info.hasBusEntry = true; + } + + break; + + case SCH_SYMBOL_T: + case SCH_SHEET_T: + if( item->IsConnected( aPosition ) ) + { + breakLines[WIRES] = true; + exitAngles[WIRES].insert( uniqueAngle++ ); + } + + break; + + default: break; + } + } + + for( int layer : { WIRES, BUSES } ) + { + if( breakLines[layer] ) + { + for( const SCH_LINE* line : midPointLines[layer] ) + { + exitAngles[layer].insert( line->GetAngleFrom( aPosition ) ); + exitAngles[layer].insert( line->GetReverseAngleFrom( aPosition ) ); + } + } + } + + info.isJunction = exitAngles[WIRES].size() >= 3 || exitAngles[BUSES].size() >= 3; + return info; +} \ No newline at end of file diff --git a/eeschema/junction_helpers.h b/eeschema/junction_helpers.h new file mode 100644 index 0000000000..5256bd14bb --- /dev/null +++ b/eeschema/junction_helpers.h @@ -0,0 +1,24 @@ +#include +#include + +namespace JUNCTION_HELPERS +{ + +/** + * A selection of information about a point in the schematic that might + * be eligible for turning into a junction. +*/ +struct POINT_INFO +{ + bool isJunction; + bool hasExplicitJunctionDot; + bool hasBusEntry; +}; + +/** + * Check a tree of items for a confluence at a given point and work out what kind of junction + * it is, if any. +*/ +POINT_INFO AnalyzePoint( const EE_RTREE& aItem, const VECTOR2I& aPosition, bool aBreakCrossings ); + +} // namespace JUNCTION_HELPERS \ No newline at end of file diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp index bff21bfc84..8b2f90c46d 100644 --- a/eeschema/sch_screen.cpp +++ b/eeschema/sch_screen.cpp @@ -43,6 +43,7 @@ #include #include +#include #include #include #include @@ -476,33 +477,39 @@ std::set SCH_SCREEN::MarkConnections( SCH_LINE* aSegment, bool aSecon bool SCH_SCREEN::IsJunction( const VECTOR2I& aPosition ) const { - bool hasExplicitJunction; - bool hasBusEntry; - bool isJunction = doIsJunction( aPosition, false, &hasExplicitJunction, &hasBusEntry ); - - return isJunction; + const JUNCTION_HELPERS::POINT_INFO info = + JUNCTION_HELPERS::AnalyzePoint( Items(), aPosition, false ); + return info.isJunction; } bool SCH_SCREEN::IsExplicitJunction( const VECTOR2I& aPosition ) const { - bool hasExplicitJunction; - bool hasBusEntry; - bool isJunction = doIsJunction( aPosition, false, &hasExplicitJunction, &hasBusEntry ); + const JUNCTION_HELPERS::POINT_INFO info = + JUNCTION_HELPERS::AnalyzePoint( Items(), aPosition, false ); - return isJunction && !hasBusEntry; + return info.isJunction && !info.hasBusEntry; } bool SCH_SCREEN::IsExplicitJunctionNeeded( const VECTOR2I& aPosition ) const { - bool hasExplicitJunction; - bool hasBusEntry; - bool isJunction = doIsJunction( aPosition, false, &hasExplicitJunction, &hasBusEntry ); + const JUNCTION_HELPERS::POINT_INFO info = + JUNCTION_HELPERS::AnalyzePoint( Items(), aPosition, false ); - return isJunction && !hasBusEntry && !hasExplicitJunction; + return info.isJunction && !info.hasBusEntry && !info.hasExplicitJunctionDot; } + +bool SCH_SCREEN::IsExplicitJunctionAllowed( const VECTOR2I& aPosition ) const +{ + const JUNCTION_HELPERS::POINT_INFO info = + JUNCTION_HELPERS::AnalyzePoint( Items(), aPosition, true ); + + return info.isJunction && !info.hasBusEntry; +} + + SPIN_STYLE SCH_SCREEN::GetLabelOrientationForPoint( const VECTOR2I& aPosition, SPIN_STYLE aDefaultOrientation, const SCH_SHEET_PATH* aSheet ) const @@ -701,118 +708,6 @@ SPIN_STYLE SCH_SCREEN::GetLabelOrientationForPoint( const VECTOR2I& aPosit } -bool SCH_SCREEN::IsExplicitJunctionAllowed( const VECTOR2I& aPosition ) const -{ - bool hasExplicitJunction; - bool hasBusEntry; - bool isJunction = doIsJunction( aPosition, true, &hasExplicitJunction, &hasBusEntry ); - - return isJunction && !hasBusEntry; -} - - - -bool SCH_SCREEN::doIsJunction( const VECTOR2I& aPosition, bool aBreakCrossings, - bool* aHasExplicitJunctionDot, bool* aHasBusEntry ) const -{ - enum layers { WIRES = 0, BUSES }; - - *aHasExplicitJunctionDot = false; - *aHasBusEntry = false; - - bool breakLines[ 2 ] = { false }; - std::unordered_set exitAngles[ 2 ]; - std::vector midPointLines[ 2 ]; - - // A pin at 90° still shouldn't match a line at 90° so just give pins unique numbers - int uniqueAngle = 10000; - - for( const SCH_ITEM* item : Items().Overlapping( aPosition ) ) - { - if( item->GetEditFlags() & STRUCT_DELETED ) - continue; - - switch( item->Type() ) - { - case SCH_JUNCTION_T: - if( item->HitTest( aPosition, -1 ) ) - *aHasExplicitJunctionDot = true; - - break; - - case SCH_LINE_T: - { - const SCH_LINE* line = static_cast( item ); - int layer; - - if( line->GetStartPoint() == line->GetEndPoint() ) - break; - else if( line->GetLayer() == LAYER_WIRE ) - layer = WIRES; - else if( line->GetLayer() == LAYER_BUS ) - layer = BUSES; - else - break; - - if( line->IsConnected( aPosition ) ) - { - breakLines[ layer ] = true; - exitAngles[ layer ].insert( line->GetAngleFrom( aPosition ) ); - } - else if( line->HitTest( aPosition, -1 ) ) - { - if( aBreakCrossings ) - breakLines[ layer ] = true; - - // Defer any line midpoints until we know whether or not we're breaking them - midPointLines[ layer ].push_back( line ); - } - } - break; - - case SCH_BUS_WIRE_ENTRY_T: - if( item->IsConnected( aPosition ) ) - { - breakLines[ BUSES ] = true; - exitAngles[ BUSES ].insert( uniqueAngle++ ); - breakLines[ WIRES ] = true; - exitAngles[ WIRES ].insert( uniqueAngle++ ); - *aHasBusEntry = true; - } - - break; - - case SCH_SYMBOL_T: - case SCH_SHEET_T: - if( item->IsConnected( aPosition ) ) - { - breakLines[ WIRES ] = true; - exitAngles[ WIRES ].insert( uniqueAngle++ ); - } - - break; - - default: - break; - } - } - - for( int layer : { WIRES, BUSES } ) - { - if( breakLines[ layer ] ) - { - for( const SCH_LINE* line : midPointLines[ layer ] ) - { - exitAngles[ layer ].insert( line->GetAngleFrom( aPosition ) ); - exitAngles[ layer ].insert( line->GetReverseAngleFrom( aPosition ) ); - } - } - } - - return exitAngles[ WIRES ].size() >= 3 || exitAngles[ BUSES ].size() >= 3; -} - - bool SCH_SCREEN::IsTerminalPoint( const VECTOR2I& aPosition, int aLayer ) const { wxCHECK_MSG( aLayer == LAYER_NOTES || aLayer == LAYER_BUS || aLayer == LAYER_WIRE, false, diff --git a/qa/tests/eeschema/CMakeLists.txt b/qa/tests/eeschema/CMakeLists.txt index 48119501f5..b0cae74808 100644 --- a/qa/tests/eeschema/CMakeLists.txt +++ b/qa/tests/eeschema/CMakeLists.txt @@ -60,6 +60,7 @@ set( QA_EESCHEMA_SRCS erc/test_erc_rule_areas.cpp test_eagle_plugin.cpp + test_junction_helpers.cpp test_lib_part.cpp test_netlist_exporter_kicad.cpp test_ee_item.cpp @@ -116,4 +117,3 @@ target_compile_definitions( qa_eeschema ) kicad_add_boost_test( qa_eeschema qa_eeschema ) - diff --git a/qa/tests/eeschema/test_junction_helpers.cpp b/qa/tests/eeschema/test_junction_helpers.cpp new file mode 100644 index 0000000000..20087dfefb --- /dev/null +++ b/qa/tests/eeschema/test_junction_helpers.cpp @@ -0,0 +1,318 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright (C) 2024 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 + */ + +#include + +#include + +#include +#include +#include +#include + +using namespace JUNCTION_HELPERS; + +static constexpr int BE_SIZE = 25400; + +struct JUNCTION_HELPER_FIXTURE +{ + EE_RTREE items; +}; + +static SCH_LINE* make_wire( const VECTOR2I& aStart, const VECTOR2I& aEnd ) +{ + SCH_LINE* const line = new SCH_LINE{ aStart, LAYER_WIRE }; + line->SetEndPoint( aEnd ); + return line; +} + +static SCH_LINE* make_bus( const VECTOR2I& aStart, const VECTOR2I& aEnd ) +{ + SCH_LINE* const line = new SCH_LINE{ aStart, LAYER_BUS }; + line->SetEndPoint( aEnd ); + return line; +} + +BOOST_FIXTURE_TEST_SUITE( JunctionHelpers, JUNCTION_HELPER_FIXTURE ) + +/** + * Check that we can get the basic properties out as expected + */ +BOOST_AUTO_TEST_CASE( Empty ) +{ + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + + BOOST_CHECK( !info.isJunction ); +} + +BOOST_AUTO_TEST_CASE( SingleWireEnd ) +{ + /* + * not a junction + * V + * ----------- + */ + items.insert( make_wire( { 0, 0 }, { 0, 100 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + + BOOST_CHECK( !info.isJunction ); +} + +BOOST_AUTO_TEST_CASE( WireCorner ) +{ + /* + * | + * |_____ + * ^ + * not a junction + */ + items.insert( make_wire( { 0, 0 }, { 100, 0 } ) ); + items.insert( make_wire( { 0, 0 }, { 0, 100 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + + BOOST_CHECK( !info.isJunction ); +} + +BOOST_AUTO_TEST_CASE( WireTee ) +{ + /* + * | /-- junction in the middle + * | / + * -----O------ + */ + items.insert( make_wire( { 0, 0 }, { 100, 0 } ) ); + items.insert( make_wire( { 0, 0 }, { -100, 0 } ) ); + items.insert( make_wire( { 0, 0 }, { 0, 100 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + + BOOST_CHECK( info.isJunction ); +} + +BOOST_AUTO_TEST_CASE( BusEntryOnBus ) +{ + /* + * || <-- not a junction (it is a connection!) + * ||\ + * || \ + * || \ <-- also not a junction + */ + + items.insert( make_bus( { 0, 0 }, { 0, BE_SIZE } ) ); + + SCH_BUS_WIRE_ENTRY* const busEntry = new SCH_BUS_WIRE_ENTRY( { 0, 0 }, false ); + + // Make sure the BE is where we think it is + BOOST_REQUIRE_EQUAL( busEntry->GetPosition(), VECTOR2I( 0, 0 ) ); + BOOST_REQUIRE_EQUAL( busEntry->GetEnd(), VECTOR2I( BE_SIZE, BE_SIZE ) ); + + items.insert( busEntry ); + + // BE-bus point + const POINT_INFO info_start = AnalyzePoint( items, { 0, 0 }, false ); + BOOST_CHECK( !info_start.isJunction ); + BOOST_CHECK( info_start.hasBusEntry ); + + // Dangling end of the bus entry + const POINT_INFO info_end = AnalyzePoint( items, { BE_SIZE, BE_SIZE }, false ); + BOOST_CHECK( !info_end.isJunction ); + BOOST_CHECK( info_end.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( BusEntryToWire ) +{ + /* + * \ + * \ /--- <-- not a junction + * \V________________ + */ + SCH_BUS_WIRE_ENTRY* const busEntry = new SCH_BUS_WIRE_ENTRY( { 0, 0 }, false ); + items.insert( busEntry ); + items.insert( make_wire( { BE_SIZE, BE_SIZE }, { 2 * BE_SIZE, BE_SIZE } ) ); + + const POINT_INFO info = AnalyzePoint( items, { BE_SIZE, BE_SIZE }, false ); + BOOST_CHECK( !info.isJunction ); + BOOST_CHECK( info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( WireDirectToBus ) +{ + /* + * || /--- <-- not a junction + * ||______________ + */ + items.insert( make_bus( { 0, 0 }, { 0, BE_SIZE } ) ); + items.insert( make_wire( { 0, 0 }, { BE_SIZE, 0 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + BOOST_CHECK( !info.isJunction ); + BOOST_CHECK( !info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( WireCrossingBus ) +{ + /* || __ not a junction + * || / + * ______________________ + * || + * || + */ + items.insert( make_bus( { 0, 0 }, { 0, -BE_SIZE } ) ); + items.insert( make_bus( { 0, 0 }, { 0, BE_SIZE } ) ); + items.insert( make_wire( { 0, 0 }, { BE_SIZE, 0 } ) ); + items.insert( make_wire( { 0, 0 }, { -BE_SIZE, 0 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + // Two wires and two buses meet, which isn't a junction + BOOST_CHECK( !info.isJunction ); + BOOST_CHECK( !info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( WireToBusEntryRoot ) +{ + /* + * /--- <-- not a junction (also an ERC error!) + * ______________ + * ||\ + * || \ + * || \ + */ + items.insert( make_bus( { 0, 0 }, { 0, BE_SIZE } ) ); + items.insert( make_wire( { 0, 0 }, { BE_SIZE, 0 } ) ); + items.insert( new SCH_BUS_WIRE_ENTRY( { 0, 0 }, false ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + BOOST_CHECK( !info.isJunction ); + BOOST_CHECK( info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( WireCrossingBusEntryRoot ) +{ + /* || + * || /--- <-- it's a junction, but the client can choose not + * || to put a dot here + * ______________________ + * ||\ + * || \ + * || \ + */ + items.insert( make_bus( { 0, 0 }, { 0, -BE_SIZE } ) ); + items.insert( make_bus( { 0, 0 }, { 0, BE_SIZE } ) ); + items.insert( make_wire( { 0, 0 }, { BE_SIZE, 0 } ) ); + items.insert( make_wire( { 0, 0 }, { -BE_SIZE, 0 } ) ); + items.insert( new SCH_BUS_WIRE_ENTRY( { 0, 0 }, false ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + // Three buses meet here, so this is a junction + BOOST_CHECK( info.isJunction ); + BOOST_CHECK( info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( WireCornerToBusEntry ) +{ + /* + * || + * || + * || + * \ /--- junction here + * \ | + * O------------- + * | + * | + */ + items.insert( make_bus( { 0, 0 }, { 0, BE_SIZE } ) ); + items.insert( make_wire( { BE_SIZE, BE_SIZE }, { 2 * BE_SIZE, BE_SIZE } ) ); + items.insert( make_wire( { BE_SIZE, BE_SIZE }, { BE_SIZE, 2 * BE_SIZE } ) ); + items.insert( new SCH_BUS_WIRE_ENTRY( { 0, 0 }, false ) ); + + const POINT_INFO info = AnalyzePoint( items, { BE_SIZE, BE_SIZE }, false ); + BOOST_CHECK( info.isJunction ); + BOOST_CHECK( info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( SheetPinToOneWire ) +{ + /* + * ---+ ___not a junction + * |/ + * |=>|--------- + * | + * --+ + */ + + // The point of interest is at (0, 0) + + items.insert( make_wire( { 0, 0 }, { BE_SIZE, 0 } ) ); // right + + SCH_SHEET* const sheet = new SCH_SHEET( nullptr, { -10 * BE_SIZE, -10 * BE_SIZE }, + { 10 * BE_SIZE, 20 * BE_SIZE } ); + SCH_SHEET_PIN* const pin = new SCH_SHEET_PIN( sheet, { 0, 0 }, "Pin Name" ); + pin->SetSide( SHEET_SIDE::RIGHT ); + + sheet->AddPin( pin ); + items.insert( sheet ); + + // This test won't make sense if the pin isn't where we think it is! + BOOST_REQUIRE( sheet->IsConnected( { 0, 0 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + BOOST_CHECK( !info.isJunction ); + BOOST_CHECK( !info.hasBusEntry ); +} + +BOOST_AUTO_TEST_CASE( SheetPinToTwoWires ) +{ + /* + * A sheet pin counts as a wire, so this is a junction + * + * ---+ ___this is a junction + * |/ + * |=>O--------- + * |\ + * | \ + * | \ + * --+ + */ + // The point of interest is at (0, 0) + + items.insert( make_wire( { 0, 0 }, { BE_SIZE, 0 } ) ); // right + items.insert( make_wire( { 0, 0 }, { BE_SIZE, BE_SIZE } ) ); // right and down + + SCH_SHEET* const sheet = new SCH_SHEET( nullptr, { -10 * BE_SIZE, -10 * BE_SIZE }, + { 10 * BE_SIZE, 20 * BE_SIZE } ); + SCH_SHEET_PIN* const pin = new SCH_SHEET_PIN( sheet, { 0, 0 }, "Pin Name" ); + pin->SetSide( SHEET_SIDE::RIGHT ); + + sheet->AddPin( pin ); + items.insert( sheet ); + + BOOST_REQUIRE( sheet->IsConnected( { 0, 0 } ) ); + + const POINT_INFO info = AnalyzePoint( items, { 0, 0 }, false ); + BOOST_CHECK( info.isJunction ); + BOOST_CHECK( !info.hasBusEntry ); +} + +BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file