kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

Hi Sylwester,

I took a look at your repo and I have a few comments on your changes.  I
did not test it so there may be functional issues as well.

Please simplify your logic in
DIALOG_SIM_SETTINGS::TransferDataToWindow().  It's way more complicated
than it needs to be.

Get rid of DIALOG_SIM_SETTINGS::disableCtrlOnCheckboxEvent() and use the
appropriate wxUpdateUIEvent[1] handler to enable and/or disable any
controls.  Manually doing this will almost certainly lead to broken
control states.  We have done this poorly so many times in the past that
I am not allowing it in new code.

If you want to submit your changes for consideration, you should squash
you changes into a single patch and submit the output of `git
format-patch` to the mailing list.

Cheers,

Wayne

[1]:
https://docs.wxwidgets.org/3.0/classwx_update_u_i_event.html#aa25df58e7047f819f5dd0520eb2cc8ea

On 6/21/19 3:00 PM, Sylwester Kocjan wrote:
> Hi,
> 
> I prepared some changes in KiCad simulation dialog. They are about
> additional simulation parameters. According to tutorial, which I've
> found on website, I store them on GitHub repository in branch
> "Sim_InitialConditions_SK" here:
> 
> https://github.com/skocjan/kicad_initialconditions.git
> 
> Could you please have a look and do some review if possible? I'd be
> grateful for feedback if these changes are OK. In future I'd like to
> implement OP analysis using some controls I've added
> (https://bugs.launchpad.net/kicad/+bug/1821360).
> 
> Additionally I also prepared polish translation for new strings.
> 
> My aim was to make simulation with arbitrary initial conditions
> possible (additionally I added other options). Right now it is possible
> to enable checkbox UIC on TRAN tab, but there is no possibility to set
> IC to any arbitrary value in element properties. I'm afraid it will
> involve change in .sch format. Please take note that it is also
> possible to define initial conditions for entire nodes instead of
> capacitors or inductors (for example: ".ic v(11)=5 v(4)=-5 v(2)=2.2",
> see ngspice manual, chapter 15.2.2). It is also a challenge. 
> 
> So in current state we can use for initial conditions results of .OP
> analysis or default zero. 
> 
> From my point of view these are the topics, which should be taken into
> account during review:
> - in simulation code sometimes there is used '\n', sometimes "\r\n".
> Maybe it should be unified? Is there a common definition for newline in
> wxWidgets?
> - I added .option savecurrents: someone added TODO that it doesn't
> work. Maybe should we hide this control? 
> - something happened to colours on dialog windows, see attached image.
> I don't know what is it and how to fix it. seems to be unrelated to my
> changes, but maybe I'm wrong? (this is visible on Ubuntu 18.04)
> - some feedback regarding how to add IC field to capacitors, inductors
> or nodes will be very appreciated ;)
> 
> Limitations of my changes:
> - parsing SPICE .options is not implemented (in bool
> DIALOG_SIM_SETTINGS::parseCommand( const wxString& aCommand ))
> 
> Best regards,
> Sylwek
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>