kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

Hi,

Here's a patch for a very old bug in the microwave inductor (which is
a 5.1.0 milestone).

I haven't fixed the underlying issue here, but it does stop invalid
outputs being generated with a warning.

An example of inputs that generate invalid outputs (0.25mm trace width):

* Draw inductor 5mm long, ask for 6mm: this is too small
* Draw inductor 5mm long, as for 7mm: this can't be represented accurately

These aren't trivial to fix and only affect very "Short" inductors of
a few small turns. I have done the calculations for the first case (it
doesn't have a closed-form solution), but haven't started the second
bit, and there's a refactor and a good think needed. So, submitting
this to close out the 5.1.0 bug and at least prevent invalid outputs.

Cheers,

John

From 37ffeb90c01dbd1a9c81e92333f993527f7fe517 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Thu, 24 Jan 2019 17:02:12 +0000
Subject: [PATCH] Pcbnew: Disallow invalid mwwave inductor lengths

Some microwave inductor lengths cause invalid outputs
for the calculations, which causes jagged outputs.

* If the request length is such that four arcs and no straight
  segments is too long
* If the length is such that an N-turn coil is too short, but
  an N+1-turn coil is too long. This can happen when the coil
  count is small.

This patch doesn't fix the underlying geometric issue here - fixing
the first requires a numerical method, and fixing the second probably
needs an iterative approach. Both of these could benefit from
a refactor.

However, this patch does prevent the tools producing invalid outputs,
which can sometimes be quite subtle mistakes if the "jags" are small.

Fixes: lp:1792119
* https://bugs.launchpad.net/kicad/+bug/1792119
---
 pcbnew/microwave/microwave_inductor.cpp | 60 ++++++++++++++++++++-----
 1 file changed, 49 insertions(+), 11 deletions(-)

diff --git a/pcbnew/microwave/microwave_inductor.cpp b/pcbnew/microwave/microwave_inductor.cpp
index d5a8d81c2..2136a54b5 100644
--- a/pcbnew/microwave/microwave_inductor.cpp
+++ b/pcbnew/microwave/microwave_inductor.cpp
@@ -79,6 +79,15 @@ static void gen_arc( std::vector <wxPoint>& aBuffer,
 }
 
 
+enum class INDUCTOR_S_SHAPE_RESULT
+{
+    OK,        /// S-shape constructed
+    TOO_LONG,  /// Requested length too long
+    TOO_SHORT, /// Requested length too short
+    NO_REPR,   /// Requested length can't be represented
+};
+
+
 /**
  * Function BuildCornersList_S_Shape
  * Create a path like a S-shaped coil
@@ -88,10 +97,8 @@ static void gen_arc( std::vector <wxPoint>& aBuffer,
  * @param  aLength = full length of the path
  * @param  aWidth = segment width
  */
-static int BuildCornersList_S_Shape( std::vector <wxPoint>& aBuffer,
-                              const wxPoint& aStartPoint,
-                              const wxPoint& aEndPoint,
-                              int aLength, int aWidth )
+static INDUCTOR_S_SHAPE_RESULT BuildCornersList_S_Shape( std::vector<wxPoint>& aBuffer,
+        const wxPoint& aStartPoint, const wxPoint& aEndPoint, int aLength, int aWidth )
 {
 /* We must determine:
  * segm_count = number of segments perpendicular to the direction
@@ -171,7 +178,7 @@ static int BuildCornersList_S_Shape( std::vector <wxPoint>& aBuffer,
             if( radius < aWidth ) // Radius too small.
             {
                 // Unable to create line: Requested length value is too large for room
-                return 0;
+                return INDUCTOR_S_SHAPE_RESULT::TOO_LONG;
             }
         }
 
@@ -192,6 +199,30 @@ static int BuildCornersList_S_Shape( std::vector <wxPoint>& aBuffer,
     // reduce len of the segm_count segments + 2 half size segments (= 1 full size segment)
     segm_len -= delta_size / (segm_count + 1);
 
+    // at this point, it could still be that the requested length is too
+    // short (because 4 quarter-circles are too long)
+    // to fix this is a relatively complex numerical problem which probably
+    // needs a refactor in this area. For now, just reject these cases:
+    {
+        const int min_total_length = 2 * stubs_len + 2 * M_PI * ADJUST_SIZE * radius;
+        if( min_total_length > aLength )
+        {
+            // we can't express this inductor with 90-deg arcs of this radius
+            return INDUCTOR_S_SHAPE_RESULT::TOO_SHORT;
+        }
+    }
+
+    if( segm_len - 2 * radius < 0 )
+    {
+        // we can't represent this exact requested length with this number
+        // of segments (using the current algorithm). This stems from when
+        // you add a segment, you also add another half-circle, so there's a
+        // little bit of "dead" space.
+        // It's a bit ugly to just reject the input, as it might be possible
+        // to tweak the radius, but, again, that probably needs a refactor.
+        return INDUCTOR_S_SHAPE_RESULT::NO_REPR;
+    }
+
     // Generate first line (the first stub) and first arc (90 deg arc)
     pt = aStartPoint;
     aBuffer.push_back( pt );
@@ -257,7 +288,7 @@ static int BuildCornersList_S_Shape( std::vector <wxPoint>& aBuffer,
     // push last point (end point)
     aBuffer.push_back( aEndPoint );
 
-    return 1;
+    return INDUCTOR_S_SHAPE_RESULT::OK;
 }
 
 
@@ -298,7 +329,6 @@ MODULE* MWAVE::CreateMicrowaveInductor( INDUCTOR_PATTERN& inductorPattern,
      */
 
     D_PAD*   pad;
-    int      ll;
     wxString msg;
 
     auto pt = inductorPattern.m_End - inductorPattern.m_Start;
@@ -324,14 +354,22 @@ MODULE* MWAVE::CreateMicrowaveInductor( INDUCTOR_PATTERN& inductorPattern,
 
     // Calculate the elements.
     std::vector <wxPoint> buffer;
-    ll = BuildCornersList_S_Shape( buffer, inductorPattern.m_Start,
-                                   inductorPattern.m_End, inductorPattern.m_length,
-                                   inductorPattern.m_Width );
+    const INDUCTOR_S_SHAPE_RESULT res = BuildCornersList_S_Shape( buffer, inductorPattern.m_Start,
+            inductorPattern.m_End, inductorPattern.m_length, inductorPattern.m_Width );
 
-    if( !ll )
+    switch( res )
     {
+    case INDUCTOR_S_SHAPE_RESULT::TOO_LONG:
         aErrorMessage = _( "Requested length too large" );
         return nullptr;
+    case INDUCTOR_S_SHAPE_RESULT::TOO_SHORT:
+        aErrorMessage = _( "Requested length too small" );
+        return nullptr;
+    case INDUCTOR_S_SHAPE_RESULT::NO_REPR:
+        aErrorMessage = _( "Requested length can't be represented" );
+        return nullptr;
+    case INDUCTOR_S_SHAPE_RESULT::OK:
+        break;
     }
 
     // Generate footprint. the value is also used as footprint name.
-- 
2.20.1