Transmission line calculator overwrites input parameters

[Janis Skujenieks]

When using Calculator tool -> TransLine -> Coplanar wave guide with ground plane, sometimes electrical parameters that you input are overwritten with nan.

Example for Coplanar wave guide with ground plane:
Er=4.2
TanD=0.02
Rho=1.72e-8
H=1
T=0.035
mu Rel C=1
Frequency=2.5GHz
W=0.2
S=selected for calculation
L=0
Input params:
Z0=50
Ang_l=0

Press 'Synthesize' and Z0 and Ang_l and L gets overwritten to '-nan'
These parameters are input and its annoying to rewrite them. They shouldn't change.
I understand that these equations work only in specific range. I think correct way would be to present warning if input parameters are out of range.

Application: kicad
Version: (5.0.0-rc2-dev-426-ge57435c0f), debug build
Libraries:
    wxWidgets 3.0.3
    libcurl/7.58.0 OpenSSL/1.1.0g zlib/1.2.11 libidn2/2.0.4 libpsl/0.19.1 (+libidn2/2.0.4) nghttp2/1.30.0
Platform: Linux 4.15.6-1-ARCH x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
    Boost: 1.66.0
    Curl: 7.58.0
    Compiler: GCC 7.3.0 with C++ ABI 1011

Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_SPICE=ON

[José Jorge (mexchip)]

Would it be ok to show a dialog asking the user if he/she wants to keep or revert the changes to the input parameters?

[Janis Skujenieks (janis-skujenieks)]

I don't see any situation where I would want my input parameters changed. What would be the use for that? I think warning that there is no solution for given parameters would be enough.

[Seth Hillbrand (sethh)]

@José- Good question. I think the correct solution is Janis' suggestion: warn of no solution when the result is NaN and keep the user's previous input values.

[jean-pierre charras (jp-charras)]

I am guessing the fact "Synthesize" overwrite input parameters is only a (bad) side effect of the way internal calculations are made.

There is no good reason to overwrite input parameters.
When there is no solution, the output parameters show NaN. This is enough.

[José Jorge (mexchip)]

I'm attaching a WIP patch that does not update the input parameters described in the bug description.

As mentioned before, the coplanar calculations modify Z0 and ang_l, it is event stated in the code:

coplanar.cpp:
// compute inital coplanar parameters. This function modify Z0 and ang_l
// (set to NaN in some cases)

The patch avoids updating input parameters when Z0 is NaN. The same logic is applied to twisted pair calculations.

I would like some feedback, if the patch is ok and/or if it'd need to be applied to other calculations as well.

[Seth Hillbrand (sethh)]

Hi José-

Looks like a good start. Definitely an improvement. One thing that jumps out as missing in this is an indication that the calculation failed.

That said, I'm curious to know if Synthesize should ever overwrite Z0 when it succeeds. Is the calculation one that gives an approximation that requires a new Z0? If so, it might be equally frustrating if you are tuning to keep resetting Z0 to your desired value as it is to reset it from NaN.

[José Jorge (mexchip)]

I haven looked in detail, but it seems that several calculations use approximations and try to reduce the error by adjusting several values for input parameters, that's why at first I thought of letting the values change (current behavior), show a message box to the user stating that the change is indeed detected and let him/her keep the new values or go back to his/her original input.

[José Jorge (mexchip)]

I'm attaching a patch that takes a different approach, it adds a copy of every input parameter and compares the copy after the calculation, if different, shows a message to the user and let's him/her keep the new values or return to the original ones.
This patch could cover the following:
* showing a message to the user so he/she knows explicitly what happened.
* allows the user to see the new values,
* allows the user to keep the original values.

Regarding the example provided in the bug description, if nothing is entered for S (selectec for calculation, but leaving the input text empty), it seems Z0 and ang_l are not changed to nan (only L). I had to enter a value (e. g. 0.2) for S to get Z0 and ang_l changed to nan.

Could someone give this patch a try with the "Coplanar wave guide with ground plane" option and give some feedback?

[Seth Hillbrand (sethh)]

Hi José- Thank you for continuing to improve your patch! It will take a bit of time to review your patch and consider this approach. I should be able to look at it after the 5.0.1 release unless someone else is able to get to it first!

-S

[Seth Hillbrand (sethh)]

Sorry that this slipped through the cracks. Setting milestone so we don't forget it

[Seth Hillbrand (sethh)]

Hi José-

I tested the current patch but I always get the window informing me that "Some input parameters have been modified". It doesn't appear to be related to selecting values. I attach the parameters I was using

[Seth Hillbrand (sethh)]

@José- Is this something you could look at for 5.1.3?

[Wayne Stambaugh (stambaughw)]

@Jose, I took a look at your patch and you have a lot of coding policy violations[1]. The easiest way to fix you issue is use the clang-format hooks[2] build into our git repo. Also, we typically prefix class members with "m_" not "_". I'm not sure showing a warning dialog every calculation is a good idea. I would rather see a checkbox that allows the user to keep their current settings and a warning in the "Results" box if there is calculation error. Most users find constantly showing a warning dialog annoying.

[1]: http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html
[2]: http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#tools

[KiCad Janitor (kicad-janitor)]

KiCad bug tracker has moved to Gitlab. This report is now available here: https://gitlab.com/kicad/code/kicad/-/issues/1849