From f7cdc7af754f0830890a1c007b9079527208c9e6 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Sat, 2 Apr 2022 15:08:32 +0100 Subject: [PATCH] Apply a more sophisticated test for ignoring isInCoupledDiffPair. The basic problem is that the DRC engine does length testing and skew testing by collecting all the diff pair constituent parts and pairing them itself. Since each part is collected on its own, we need to ignore the 'B' unit when evaluating any conditional expressions. However, doing this in general means that when evaluating "OwnClearance()" we also ignore the 'B' unit and return the diff pair CLEARANCE_CONSTRAINT when we shouldn't. This implements a more discerning test which know what the current requested constraint is when evaluating expressions. See also https://forum.kicad.info/t/solved-custom-differencing-rule-not-working-drc/34034/6 Fixes https://gitlab.com/kicad/code/kicad/issues/11314 --- pcbnew/drc/drc_engine.cpp | 17 +++++++++---- pcbnew/drc/drc_rule_condition.cpp | 8 +++--- pcbnew/drc/drc_rule_condition.h | 6 ++--- .../drc/drc_test_provider_matched_length.cpp | 2 +- pcbnew/pcb_expr_evaluator.cpp | 25 ++++++++++++------- pcbnew/pcb_expr_evaluator.h | 18 ++++++------- qa/unittests/pcbnew/test_libeval_compiler.cpp | 5 ++-- 7 files changed, 46 insertions(+), 35 deletions(-) diff --git a/pcbnew/drc/drc_engine.cpp b/pcbnew/drc/drc_engine.cpp index 92365fcd61..af709fba2d 100644 --- a/pcbnew/drc/drc_engine.cpp +++ b/pcbnew/drc/drc_engine.cpp @@ -3,7 +3,7 @@ * * Copyright (C) 2004-2019 Jean-Pierre Charras, jp.charras at wanadoo.fr * Copyright (C) 2014 Dick Hollenbeck, dick@softplc.com - * Copyright (C) 2017-2021 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2017-2022 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 @@ -895,10 +895,15 @@ DRC_CONSTRAINT DRC_ENGINE::EvalRules( DRC_CONSTRAINT_T aConstraintType, const BO REPORT( wxString::Format( _( "Checking assertion \"%s\"." ), EscapeHTML( c->constraint.m_Test->GetExpression() ) ) ) - if( c->constraint.m_Test->EvaluateFor( a, b, aLayer, aReporter ) ) + if( c->constraint.m_Test->EvaluateFor( a, b, c->constraint.m_Type, aLayer, + aReporter ) ) + { REPORT( _( "Assertion passed." ) ) + } else + { REPORT( EscapeHTML( _( "--> Assertion failed. <--" ) ) ) + } }; auto processConstraint = @@ -1125,7 +1130,7 @@ DRC_CONSTRAINT DRC_ENGINE::EvalRules( DRC_CONSTRAINT_T aConstraintType, const BO EscapeHTML( c->condition->GetExpression() ) ) ) } - if( c->condition->EvaluateFor( a, b, aLayer, aReporter ) ) + if( c->condition->EvaluateFor( a, b, c->constraint.m_Type, aLayer, aReporter ) ) { if( aReporter ) { @@ -1321,7 +1326,8 @@ void DRC_ENGINE::ProcessAssertions( const BOARD_ITEM* a, REPORT( wxString::Format( _( "Checking rule assertion \"%s\"." ), EscapeHTML( c->constraint.m_Test->GetExpression() ) ) ) - if( c->constraint.m_Test->EvaluateFor( a, nullptr, a->GetLayer(), aReporter ) ) + if( c->constraint.m_Test->EvaluateFor( a, nullptr, c->constraint.m_Type, + a->GetLayer(), aReporter ) ) { REPORT( _( "Assertion passed." ) ) } @@ -1354,7 +1360,8 @@ void DRC_ENGINE::ProcessAssertions( const BOARD_ITEM* a, REPORT( wxString::Format( _( "Checking rule condition \"%s\"." ), EscapeHTML( c->condition->GetExpression() ) ) ) - if( c->condition->EvaluateFor( a, nullptr, a->GetLayer(), aReporter ) ) + if( c->condition->EvaluateFor( a, nullptr, c->constraint.m_Type, + a->GetLayer(), aReporter ) ) { REPORT( _( "Rule applied." ) ) testAssertion( c ); diff --git a/pcbnew/drc/drc_rule_condition.cpp b/pcbnew/drc/drc_rule_condition.cpp index 097f7b2076..947d16df4f 100644 --- a/pcbnew/drc/drc_rule_condition.cpp +++ b/pcbnew/drc/drc_rule_condition.cpp @@ -1,7 +1,7 @@ /* * This program source code file is part of KiCad, a free EDA CAD application. * - * Copyright (C) 2020 KiCad Developers, see change_log.txt for contributors. + * Copyright (C) 2020-2022 KiCad Developers, see change_log.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 @@ -41,7 +41,7 @@ DRC_RULE_CONDITION::~DRC_RULE_CONDITION() bool DRC_RULE_CONDITION::EvaluateFor( const BOARD_ITEM* aItemA, const BOARD_ITEM* aItemB, - PCB_LAYER_ID aLayer, REPORTER* aReporter ) + int aConstraint, PCB_LAYER_ID aLayer, REPORTER* aReporter ) { if( GetExpression().IsEmpty() ) return true; @@ -54,7 +54,7 @@ bool DRC_RULE_CONDITION::EvaluateFor( const BOARD_ITEM* aItemA, const BOARD_ITEM return false; } - PCB_EXPR_CONTEXT ctx( aLayer ); + PCB_EXPR_CONTEXT ctx( aConstraint, aLayer ); if( aReporter ) { @@ -109,7 +109,7 @@ bool DRC_RULE_CONDITION::Compile( REPORTER* aReporter, int aSourceLine, int aSou m_ucode = std::make_unique(); - PCB_EXPR_CONTEXT preflightContext( F_Cu ); + PCB_EXPR_CONTEXT preflightContext( 0, F_Cu ); bool ok = compiler.Compile( GetExpression().ToUTF8().data(), m_ucode.get(), &preflightContext ); return ok; diff --git a/pcbnew/drc/drc_rule_condition.h b/pcbnew/drc/drc_rule_condition.h index 0ea64f0a06..c7208ac543 100644 --- a/pcbnew/drc/drc_rule_condition.h +++ b/pcbnew/drc/drc_rule_condition.h @@ -1,7 +1,7 @@ /* * This program source code file is part of KiCad, a free EDA CAD application. * - * Copyright (C) 2020 KiCad Developers, see change_log.txt for contributors. + * Copyright (C) 2020-2022 KiCad Developers, see change_log.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 @@ -38,8 +38,8 @@ public: DRC_RULE_CONDITION( const wxString& aExpression = "" ); ~DRC_RULE_CONDITION(); - bool EvaluateFor( const BOARD_ITEM* aItemA, const BOARD_ITEM* aItemB, PCB_LAYER_ID aLayer, - REPORTER* aReporter = nullptr ); + bool EvaluateFor( const BOARD_ITEM* aItemA, const BOARD_ITEM* aItemB, int aConstraint, + PCB_LAYER_ID aLayer, REPORTER* aReporter = nullptr ); bool Compile( REPORTER* aReporter, int aSourceLine = 0, int aSourceOffset = 0 ); diff --git a/pcbnew/drc/drc_test_provider_matched_length.cpp b/pcbnew/drc/drc_test_provider_matched_length.cpp index 565fd80eb4..9aa3ebe0c8 100644 --- a/pcbnew/drc/drc_test_provider_matched_length.cpp +++ b/pcbnew/drc/drc_test_provider_matched_length.cpp @@ -333,7 +333,7 @@ bool DRC_TEST_PROVIDER_MATCHED_LENGTH::runInternal( bool aDelayReportMode ) } else { - ent.from = ent.to = _(""); + ent.from = ent.to = _( "" ); } m_report.Add( ent ); diff --git a/pcbnew/pcb_expr_evaluator.cpp b/pcbnew/pcb_expr_evaluator.cpp index 23dc60118d..772c7dbba2 100644 --- a/pcbnew/pcb_expr_evaluator.cpp +++ b/pcbnew/pcb_expr_evaluator.cpp @@ -784,20 +784,27 @@ static void isCoupledDiffPair( LIBEVAL::CONTEXT* aCtx, void* self ) aCtx->Push( result ); result->SetDeferredEval( - [a, b]() -> double + [a, b, context]() -> double { NETINFO_ITEM* netinfo = a ? a->GetNet() : nullptr; - wxString coupledNet; - wxString dummy; - if( netinfo - && DRC_ENGINE::MatchDpSuffix( netinfo->GetNetname(), coupledNet, dummy ) - && ( !b || b->GetNetname() == coupledNet ) ) + if( !netinfo ) + return 0.0; + + wxString coupledNet; + wxString dummy; + + if( !DRC_ENGINE::MatchDpSuffix( netinfo->GetNetname(), coupledNet, dummy ) ) + return 0.0; + + if( context->GetConstraint() == DRC_CONSTRAINT_T::LENGTH_CONSTRAINT + || context->GetConstraint() == DRC_CONSTRAINT_T::SKEW_CONSTRAINT ) { + // DRC engine evaluates these singly, so we won't have a B item return 1.0; } - return 0.0; + return b && b->GetNetname() == coupledNet; } ); } @@ -1227,12 +1234,12 @@ PCB_EXPR_EVALUATOR::~PCB_EXPR_EVALUATOR() bool PCB_EXPR_EVALUATOR::Evaluate( const wxString& aExpr ) { PCB_EXPR_UCODE ucode; - PCB_EXPR_CONTEXT preflightContext( F_Cu ); + PCB_EXPR_CONTEXT preflightContext( NULL_CONSTRAINT, F_Cu ); if( !m_compiler.Compile( aExpr.ToUTF8().data(), &ucode, &preflightContext ) ) return false; - PCB_EXPR_CONTEXT evaluationContext( F_Cu ); + PCB_EXPR_CONTEXT evaluationContext( NULL_CONSTRAINT, F_Cu ); LIBEVAL::VALUE* result = ucode.Run( &evaluationContext ); if( result->GetType() == LIBEVAL::VT_NUMERIC ) diff --git a/pcbnew/pcb_expr_evaluator.h b/pcbnew/pcb_expr_evaluator.h index 222738ee43..a3eb1fb611 100644 --- a/pcbnew/pcb_expr_evaluator.h +++ b/pcbnew/pcb_expr_evaluator.h @@ -1,7 +1,7 @@ /* * This program source code file is part of KiCad, a free EDA CAD application. * - * Copyright (C) 2019-2021 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2019-2022 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 @@ -52,7 +52,8 @@ public: class PCB_EXPR_CONTEXT : public LIBEVAL::CONTEXT { public: - PCB_EXPR_CONTEXT( PCB_LAYER_ID aLayer = UNDEFINED_LAYER ) : + PCB_EXPR_CONTEXT( int aConstraint, PCB_LAYER_ID aLayer ) : + m_constraint( aConstraint ), m_layer( aLayer ) { m_items[0] = nullptr; @@ -67,17 +68,12 @@ public: BOARD* GetBoard() const; - BOARD_ITEM* GetItem( int index ) const - { - return m_items[index]; - } - - PCB_LAYER_ID GetLayer() const - { - return m_layer; - } + int GetConstraint() const { return m_constraint; } + BOARD_ITEM* GetItem( int index ) const { return m_items[index]; } + PCB_LAYER_ID GetLayer() const { return m_layer; } private: + int m_constraint; BOARD_ITEM* m_items[2]; PCB_LAYER_ID m_layer; }; diff --git a/qa/unittests/pcbnew/test_libeval_compiler.cpp b/qa/unittests/pcbnew/test_libeval_compiler.cpp index ee501113d4..007a8fcd80 100644 --- a/qa/unittests/pcbnew/test_libeval_compiler.cpp +++ b/qa/unittests/pcbnew/test_libeval_compiler.cpp @@ -27,7 +27,7 @@ #include #include - +#include #include #include @@ -91,7 +91,8 @@ static bool testEvalExpr( const wxString& expr, LIBEVAL::VALUE expectedResult, { PCB_EXPR_COMPILER compiler; PCB_EXPR_UCODE ucode; - PCB_EXPR_CONTEXT context, preflightContext; + PCB_EXPR_CONTEXT context( NULL_CONSTRAINT, UNDEFINED_LAYER ); + PCB_EXPR_CONTEXT preflightContext( NULL_CONSTRAINT, UNDEFINED_LAYER ); bool ok = true; context.SetItems( itemA, itemB );