New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spice4qucs #275

Closed
wants to merge 411 commits into
base: develop
from

Conversation

Projects
None yet
6 participants
@ra3xdh
Contributor

ra3xdh commented Jun 12, 2015

According #226 spice4qucs was successfully rebased and ready for merge. At this stage all seems to be working. Please carefully check all again before merge.

Spice4qucs allows you to simulate Qucs schematic with Ngspice and Xyce. See #77 and #181 for more details.

@ra3xdh ra3xdh referenced this pull request Jun 12, 2015

Closed

Prepare spice4qucs for merge #226

13 of 16 tasks complete
@in3otd

This comment has been minimized.

Contributor

in3otd commented Jun 12, 2015

I played a little with spice4qucs and saw no real issues up to now, but I have a few observations:

  • The File Dialogs used in File->Open... and in Simulation->Simulate with spice->Settings->Select look quite different on my system( same for Simulation->Simulate with spice->Save netlist). Probably because the former is done using QFileDialog::getOpenFileName() and the latter is just a standard QFileDialog(), as the Qt docs says "[QFileDialog::getOpenFileName()] will use the native file dialog and not a QFileDialog.". I see no reason for the dialogs to be different, I think using the native one everywhere should be better.
  • There are no checks whether the ngpice or Xyce executables actually exists and can be run. SimProcess->start(cmd) is called and no error is reported if the simulator cannot be launched. Maybe AbstractSpiceKernel should have a method that checks for and run the simulator ?
  • Is there a reason why the results graphs are updated only when the simulation window is manually closed and the Data Display file is not automatically opened? This is a different behavior compared to when qucsator is run and from a user point of view I will actually expect to have the same behavior, regardless of the simulator used.
@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jun 13, 2015

@in3otd , Thank you for testing. See my three last commits for suggested improvements.

The File Dialogs used in File->Open... and in Simulation->Simulate with spice->Settings->Select look quite different on my system

Added unified dialogs using QFileDialog::get(Save|Open)FileName

There are no checks whether the ngpice or Xyce executables actually exists and can be run.

Added ngspice and Xyce start errors processing.

Is there a reason why the results graphs are updated only when the simulation window is manually closed and the Data Display file is not automatically opened?

Implemented diagrams update after each simulation and switching to DataDisplay after Simulation dialog is closed.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jun 19, 2015

Something strange happened with Travis and last commit. I didn't change anything in Qucs-core, but tests was failed for clang and TR_Puls3b_prj. GCC build passed successfully. @guitorri , can you look at it?

@ra3xdh ra3xdh closed this Jun 19, 2015

@ra3xdh ra3xdh reopened this Jun 19, 2015

@guitorri

This comment has been minimized.

Member

guitorri commented Jun 19, 2015

I wonder why GCC is passing... At the moment Travis checks out the master branch of qucs-test on every run. Qucs-test was updated to reflect the fixes on the tswitch by in3otd. If you merge master, cherry pick (c4d8a7b) or rebase the issues should go away.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jun 20, 2015

@guitorri , I have rebased my branch. All went good, but build job #756.3 for MacOS even was not started by unknown reason. Could you take a look at it?

@guitorri

This comment has been minimized.

Member

guitorri commented Jun 20, 2015

Sometimes the machines stall and time out. In this case the OSX instance took to long to download the Qt image... I restarted it. Can't you restart? I will see how can I give you access to restart the CI runs.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jun 20, 2015

I have no access to restart button at Travis. What I need to do?

@guitorri

This comment has been minimized.

Member

guitorri commented Jun 20, 2015

Did you try to sign in into Travis with your GitHub account?

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jun 20, 2015

Yes, I signed in with my GitHub account, but there is no access to restart builds.

@guitorri

This comment has been minimized.

Member

guitorri commented Jun 20, 2015

You should have full access to Travis.
http://docs.travis-ci.com/user/travis-pro/#Who-has-access-to-the-builds%3F

The restart is a small round button on the right, under the settings button.

Edit: that info was perhaps only for the pro part of Travis?! Not sure.

@felix-salfelder

This comment has been minimized.

Member

felix-salfelder commented Jul 16, 2015

skimming through the code..

for example in schematic_file.cpp, there are a lot of bool spice and bool xyce definitions that lead to overly bloated code that is hard to read, hard to maintain and hard to extend. so instead of

function something(bool spice, bool xyce){
    if(spice){
     // ..
    }else if(xyce){
     // ..
    }else{
     // ..
    }
}

over an over again. could you please introduce classes

class spice: public simulator{
     //...
};
class xyce: public simulator{
     //...
};

and change your code to

function something(simulator* backend){
    backend->do_something(...);
}

in case the different backend require a different treatment? leaving all of this to somebody who might want to add a 4th simulator would be a bit unfair...

thanks

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 17, 2015

The last commits contains important changes suggested after testing of spice4qucs by @guitorri and @in3otd . Here is the list of the changes:

  • Simulate with external simulator dialog. This dialog is made more user-friendly. Added warning label in the top of dialog with hyperlink for help. Added exit button. Changed font to monospace. Working directory for temporary simulation data saving could set manually. Simulator log redirected to system log.txt . Now this dialog look like this:
    simdlg3
  • Added schematic checker. Checker check all components on SPICE-comaptibility. If component has undefined property SpiceModel it is treated as SPICE-incompatible (Qucs transmission lines, etc.). And such schematic cannot be simulated with SPICE. User error message is shown and further simulation is blocked:
There were SPICE-incompatible components. 
Simulator cannot proceed.Incompatible components are: Line1; Line2 ; X2
Ngspice error...
  • Implemented new datasets naming system to avoid dataset names conflicts. Now three datasets could be created during simulations scheamtic_name.dat --- default dataset from Qucsator; schematic_name.dat.ngspice --- for Ngspice; and schematic_name.dat.xyce --- for Xyce. Dataset selection list in DiagramDialog shows only the base name of dataset. User needs to select appropriate dataset from ChooseSimulator drop-down list. Only existing simulations are shown in this list. Also variable naming system is changed. Ngspice variables have ngspice/ prefix, Xyce variables have xyce/ prefix. Attached figure explains new variable naming system. All examples were updated. It's need to update documentation too.
    new_varname

Summarizing all above, I suggest to delay spice4qucs merging until 0.0.19 will be released, because a lot of new code was added at last moment. I think spice4qucs should be merged as soon as after 0.0.19 release.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 17, 2015

@felix-salfelder , All your suggested classes are already implemented. There is AbstractSpiceKernel class. Ngspice and Xyce classes inherit this class. Spice4qucs code is isolated from main Qucs code in these classes as far as possible. But not all could be implemented inside these classes. Schematic *sch pointer is passed in constructors of these classes. You need to inherit AbstractSpiceKernel and redefine some of its methods to implement a new simulator support.

Mentioned bool xyce is actually used only in two places:

  • Library component netlist obtaining
  • Subcircuit netlist obtaining.

Main circuit SPICE-netlist is generated in either Ngspice or Xyce classes. (See createNetlist() method). Before output netlists of all components it's need to fill the nodes table. It should be done with Schematic::prepareNetlist() . But Qucs will output library components netlists during netlist preparation, because subcircuits netlists builder is hidden deep inside Schematic::prepareNetlist(). So it's need to find a way to tell subcircuits netlist builder do not build Qucs netlist and build SPICE-netlist instead. Mentioned bool xyce and bool spice parameters serve only for this purpose.

As alternative it's possible to implement our own netlist preparation algorithm in Ngspice and Xyce classes. But it is not a good idea due to code duplication.

So, there are strong reasons to pass information about simulators in Schematic::prepareNetlist(). As you can see digital netlist builder uses the similar way (isVerilog property) I can define an enum simulator { ... } somewhere and use it instead of two bool parameters. It will increase code readability.

@felix-salfelder

This comment has been minimized.

Member

felix-salfelder commented Jul 17, 2015

All your suggested classes are already implemented.

no. the simulator classes are missing a common base class. currently abstractSpiceKernel inherits from QtObject.

But not all could be implemented inside these classes. Schematic *sch pointer is passed in constructors of these classes.

a pointer to the schematic should never be leaked to the simulator backend, rather (structurally) the netlister should look like

Schematic::netlist(stream, simulator)
{
     for (thing in netlist){
         simulator->netlist(stream, thing);
     }
}

this way, only the relevant parts are passed. for a full examle, consider the LANGUAGE class implemented in gnucap. the approach works to write spice, verilog, geda schematics, qucs, and some spectre dialect, should be good enogh for QUCS.

EDIT: i'm referring to the print_* routines of the LANGUAGE class

Spice4qucs code is isolated from main Qucs code in these classes as far as possible

no. the current implementation on the qucs side is filled with spice and xyce specific code. the bools i am pointing out is just one example, but a good starting point.

As alternative it's possible to implement our own netlist preparation algorithm in Ngspice and Xyce
classes. But it is not a good idea due to code duplication.

this is not what i suggest. the netlister should be freed from simulator specific code (sure!). the simulator (or language) specific code should be implemented seperately.

So, there are strong reasons to pass information about simulators in Schematic::prepareNetlist().

yes, but you should pass a pointer to the derived class of the simulator (or language dialect), not bools.

thanks

EDIT: theres now a functional example in #315. @ra3xdh please elaborate on what netlister code cannot be separated from the schematic code.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 19, 2015

Documentation is now up-to-date with the recent changes in variables naming system. See last commits on spice4qucs branch of the qucs-help.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 19, 2015

Allowed to use Xyce-Parallel comand-line patterns. This allows to construct user command to run user builds of Xyce-parallel (See https://groups.google.com/forum/#!topic/xyce-users/X2OYGeOAXEM for details). New "Simulator settings" look like this:
new-settings

@felix-salfelder

This comment has been minimized.

Member

felix-salfelder commented Jul 19, 2015

can we have one "setup simulator" dialog that works for all simulators (not one dialog that only tackles a subset of the options for two simulators)? this should be done in another PR, before adding more simulators.

thanks

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 19, 2015

This dialog serves for all external simulators. Later will be added unified simulation dialog (based on Simmessage). Also will be added Default simulator property for every schematic document.

But I think it should be done not at this point. It's not time for deep integration of external simulators. Maybe this should be done after 1-2 releases.

@felix-salfelder

This comment has been minimized.

Member

felix-salfelder commented Jul 19, 2015

It's not time for deep integration of external simulators.

all simulators currently are external (or what does "external" mean?) the dialogs should work for all of them, including the current (qucsator).

Maybe this should be done after 1-2 releases.

no. all simulators should be treated alike, from the beginning. QUCS will never recover from merging the proposed patchbomb, because every change/sanitation will then require a deep dive into all three simulator backends. nobody will want to do that (unless for money maybe).

really, we should do one thing after another, and merge small chunks in short intervals. i have started this. see #315 and #316. next, we should let the user choose and configure simulators somehow (EDIT: please consider #317).

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 20, 2015

@felix-salfelder I have considered your applications #315 and #316 Here will be my comments.

or what does "external" mean
External simulator doesn't use Qucs netlist notation and Qucs dataset format. It is all SPICE-compatible simulators. Verilog-A is not a simulator in a full sense. It is only netlister.

see #315 and #316.

I took a look at it. As far I can understand, you intend to reimplement Qucs netlist builder located in Schematic class from the scratch.

At first, I will not rewrite my code from the scratch. Almost 8 months of work will be lost. I have many positive responses for my patchset. It is operational. Only small bugs can remain. It was even tested in production. And I think it's bad idea to begin all from the scratch.

How many time will take implementation of you suggestions? #316 and #315 are only the first steps on a long way. Fullscale SPICE support in Qucs will be needed already this Autumn. There is no doubt, you are more skilled programmer than I am. But where was you last winter, when there was no "patchbomb"? I didn't close my code, spice4qucs development start was announced, and you could influence process at the beginning. Now time is over. And implementation of your concept will take a long time again. Maybe you are afraid that I will inject my patch and then disappear? No, I have no plans to disappear.

Now let's consider your code. Your solution is beautiful, but not all netlist builder could be implemented in such straightforward manner as for Qucsator and Verilog-A. Every simulator have differences. These differences ore often not obvious. Also, there is need to emulate at list the bases of Qucs equations system and postprocessors for SPICE-based simulators.

You intend to refactor Schematic::createNetlist(), remove bools usage, and implement all netlist builder via lang->printItem(pc, stream) Qucs netlist language is specially designed for such one-pass operation. This not concerns other netlister. Ngspice reuires recursive passes. Xyce requires even netlist set: one netlist per one simulation. Ngspice has ngnutmeg postprocessor. It's need to convert Qucs equations to ngnutmeg calls. Xyce has no postprocessor. It is only beginning.

Now bools are widely used inside Schematic. There are even implicit bools. I cannot say that it is good solution. For example it is : isVerilog, isAnalog , creatingLib and some others. But I think that it is not so bad. Code could be sufficiently easily understood. And there is no warranty that your solution will not introduce new bugs and will be the best.

I think we should use my existing rebase_spice4qucs branch as reference for refactoring instead of reimplementing all. I have tried to keep Schematic and QucsApp classes untouched as far as possible, because they will go through the refactoring by itself during Qt4-transfer procedure. And refactoring could be done with existing code. This refactoring could be done in convenient time. Attached block diagram illustrates base classes and its interaction (possible structure of multiple simulators, not all class methods are listed):
classes

@felix-salfelder

This comment has been minimized.

Member

felix-salfelder commented Jul 20, 2015

thanks for considering the code

I took a look at it. As far I can understand, you intend to reimplement Qucs netlist builder located in Schematic class from the scratch.

there's no reimplementation involved. I moved some lines of code from component to the language plugin for qucsator in order to be able to override the code in another plugin for the verilog netlister. the first step is merely a move, the second step is implementing something new.

At first, I will not rewrite my code from the scratch. Almost 8 months of work will be lost.

this does not seem to be necessary. i think it's OK to import the code to the components (where it does not belong) and call it from a plugin. cleanup later.

And I think it's bad idea to begin all from the scratch.

does reordering the commits and some sanitation qualify as "rewrite from scratch"?

Fullscale SPICE support in Qucs will be needed already this Autumn.

this is none of my business, i am trying to help sorting out a contribution that will arguably damage the project in the long run. the original author intentionally did NOT use SPICE behavioural modelling semantics for a very good reason. as I explained in another post, this is not even necessary to dotf

But where was you last winter, when there was no "patchbomb"?

I was writing my phd (and working on gnucap-adms etc.). i am not sorry, should I?
why do you "quote" the patchbomb? do you disagree?

Ngspice reuires recursive passes. Xyce requires even netlist set: one netlist per one simulation. Ngspice has ngnutmeg postprocessor. It's need to convert Qucs equations to ngnutmeg calls. Xyce has no postprocessor. It is only beginning.

this is exactly the reason why the simulator specific code should be separated from each other. calling ngnutmeg from there does not interfere the least.

And there is no warranty that your solution will not introduce new bugs and will be the
best.

in a first pass you could even use

if(lang->isSpice()){ // HACK, callback
    //
}

instead of if(isSpice) {}. this will hardly do anything different, and still get rid of the bools
(you need to set lang correctly, for sure).

I think we should use my existing rebase_spice4qucs branch as reference for refactoring instead of reimplementing all.

as I said, no reimplementation is required. what we need is some preparatory work that is independent of your branch (also watch out for my other comments) then rebase your branch on top of that.

there are easy changes that will reduce future headache a whole lot

  • use the Simulator base class as a base class for all simulators (your block diagram is already pretty close).
  • use the NetLang base class for the netlisting (even IF your implementations (temporarily) makes use of Schematic* it should be put where it belongs.

other things are misleading but don't really break things, for example you add QString spice_netlist(bool) to the components. how about filing a PR on this for getting started? i cannot sort out code that is not there, even if you worked on it for 8 month.

even with the functionality in the wrong place (which i think is a good compromise), writing the Xyce and Spice language classes is a matter of hours.

thank you

@in3otd

This comment has been minimized.

Contributor

in3otd commented Jul 20, 2015

...and in any case I think it will make sense to extend qucs-test to also test the netlisting for simulators other than qucsator.

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jul 21, 2015

@in3otd , Added command line option for Ngspice and Xyce netlist building and simulation execution without GUI. Now tests could be added.

@felix-salfelder

This comment has been minimized.

Member

felix-salfelder commented Jul 21, 2015

a command line option to create netlists has already been implemented in #316. here you can select the language by name, which is much better extensible.

thank you.

PS: we should work on merging this patchbomb, rather than increasing its size even more. by writing "we" i am implicitly offering help.

MikeBrinson and others added some commits Nov 13, 2015

@guitorri guitorri changed the base branch from master to develop Feb 18, 2017

@ra3xdh

This comment has been minimized.

Contributor

ra3xdh commented Jun 29, 2017

Closing this.

Recently we discussed the future of Qucs and Qucs-S with @felix-salfelder and decided to backport the codebase of Qucs-S to new modular architecutre (PRs #316, #458, and #589 ). The development and releases preparation of Qucs-S will be continued until the new architecture of Qucs will be fully operational.

@ra3xdh ra3xdh closed this Jun 29, 2017

@ra3xdh ra3xdh added the spice4qucs label Jun 29, 2017

@ra3xdh ra3xdh self-assigned this Jun 29, 2017

@Qucs Qucs locked and limited conversation to collaborators Jun 29, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.