Review side effects of PR #682

guitorri commented 7 years ago

TODO:

[x] in qucs-core/scr/circuit.h remove unneeded virtual functions. The dc/trsolver can set the state for ohmmeter and the special cases can be handled inside the ohmmeter::initDC
- [x] fix virtual void initDC2 (void) { }//only used in ohmmeter.cpp
- [x] fix virtual void initDC2 (void) { }//only used in ohmmeter.cpp
[x] Remove netlist hacking to implement internal resistance, #720. Revert addition of internal resistance for vdc, vac, idc, iac, iprobe, vprobe The right way to do it would be to pass the Ri parameter and the component sets up the series or parallel resitor. The issue is that for a series resistor the vdc needs to allocate an internal node to add the resistor, even if Ri==0. So, instead of 2 nodes for each vdc, it will now need 3. I don't thing we should be doing that. If the user wants, just add the Ri manually.
[x] Remove netlist hacking for internal resistances of wattmeter, also #720. Same thing, pass Riv, Rvv to the wprobe and let it takes care of setting the resistors. Here again, instead of 4 nodes, 5 nodes will be required. If the user is looking for more information it might make sense to pay the price of an extra node.
[x] What about iprobe, vprobe? Shall we "upgrade" them to have internal resistance? No, decided to keep them ideal.

~~- [ ] Concerning AC Phasorial diagram, <Phasor ... > Unknown diagram. Implement now a similar fallback mechanis we have for unknown components, allow to skip unknown diagrams on load.~~ Similarly an unknown Paintings need such thing. A refactor on the parser is needed.

[x] update docs to reflect that phasordiagram modify the <Diagram ... > signature, append the selected frequency Eg. <Phasor 150 617 217 217 3 #c0c0c0 1 00 1 -687.584 500 687.584 1 -687.584 500 687.584 1 -1 0.5 1 315 0 225 "" "" "" "30 Hz;60 Hz;120 Hz;"> https://github.com/Qucs/qucs-help/pull/16

~~- [ ] Remove Hz from the Phasor list of frequencies, see above.~~

[x] phasor diagram display a list of frequency points, see #723 for initial attempt.

~~- [ ] get rid of these lines.if(Diag->Name == "Phasor" || Diag->Name == "Waveac") . The phasor code needs to go to phasor.cpp, waveac needs to go to waveac.cpp~~ Such things were already in use there, needs refactor, but not in this pass.

This was caught with latest merges:

94m############################################�[0m
�[94m#  Report schematic to netlist conversion  #�[0m
�[91m--> Found differences (!)�[0m
{'AC_SW_resonance_prj': ['- Vac:V1 _net0 gnd U="1 V" f="1 GHz" Phase="0" Theta="0"\n',
                         '+ Vac:V1 _net0_TEMP_V1 gnd U="1 V" f="1 GHz" Phase="0" Theta="0" Ri="0 Ohm"\n',
                         '?             ++++++++                                          +++++++++++\n',
                         '+ R:_R__net0_TEMP_V1 _net0_TEMP_V1 _net0 R="0 Ohm" Temp = "26.85" Tc1 = "0.0" Tc2 = "0.0" Tnom = "26.85"\n',
                         '+ \n',
                         '+ \n',
                         '+ \n',
                         '+ \n',
                         '+ \n'],

A few things:

Is it needed to output to internal resistance as a parameter of sources? A resistor is being added anyway.
If so, we need to update the gold netlist.txt to reflect the internal resistance of the sources.
Where are extra linebreaks coming from? here

guitorri commented 7 years ago

@felix-salfelder the *meters seem to be doing both, behaving as components (stamping the matrix), see d06558ce2e4ef, but also [hacking the netlist].(https://github.com/Qucs/qucs/blob/develop/qucs/qucs/components/component.cpp#L655-L657)

@gildias can you comment?

guitorri commented 7 years ago

A few more notes about PR #682

Qucs:

phasordiagram, AC Phasorial diagram, <Phasor ... >

break backward compatibility, Unknown diagram
Diagram properties dialog, frequency should be a dropbox with available frequencies, not a textbox.
phasordiagram modify the <Diagram ... > signature, append the selected frequency https://github.com/andresmmera/qucs/blame/ImprovedMarkers/qucs/qucs/diagrams/diagram.cpp#L1406

Qucsator:

Silence warning:

In file included from ../../../src/components/component.h:43:
../../../src/circuit.h:136:29: warning: control reaches end of non-void function [-Wreturn-type]
  virtual int getstate () { }//only used in ohmmeter.cpp

andresmmera commented 7 years ago

Diagram properties dialog, frequency should be a dropbox with available frequencies, not a textbox.

I'm working on a fix for this... [EDIT] See https://github.com/Qucs/qucs/pull/723

andresmmera commented 7 years ago

break backward compatibility, Unknown diagram

Can we really solve this? I mean, these diagrams are new features not included in previous releases. I don't see how to tackle this issue properly.

I guess the same happens with the recently added CAPQ, INDQ and CIRCLINE components (I didn't check that...)

felix-salfelder commented 7 years ago

Can we really solve this?

once we have plugins, it will be possible to backport new stuff to old versions.

currently that does neither seem realistic nor feasible. right now we should think of proper exceptions and error messages to report missing parts (an unavoidable scanario).

guitorri commented 7 years ago

Right now it is is impossible (or very hard) to expand the .sch syntax (add new stuff) without breaking backward compatibility. The current "parser" will just choke at any unknown thing. One way to handle it for now is add graceful degradation mechanism. When the user save the file with new stuff, inform that this version is not backward compatible. As a fallback, the Save As should enable to save for an earlier version. But since the .sch file format is not really versioned... tricky.

(off-topic, but) This was one of the main reasons I cautiously opposed from merging the Qucs-S stuff. It is very useful stuff, but I fear that merging it unaware of the consequences it will frustrate users and push the burden of fixing stuff back to us.

I guess is ok to break compatibility at some point (things have to evolve). For this reason people use major, minor and patch releases to make it clear what is going on. We can argue that so far we only had patch releases, right? :smile:

We should be fully aware of the consequences of breaking compatibility at all times, and work on our favor to help people move forward and minimize regressions and breakage on the user side. The main motivation for me on qucs-test was really this. To help us see the side effects, not to lock the things as is. We also need unit tests. I have something working and will push it shortly.

I am more concerned about the issue of internal resistance + additional resistance type of things (did not yet check if that is indeed a problem).

Enough of my gibberish.

As of now it seems that the above PR needs a bit more work. I am also looking into it.

guitorri commented 6 years ago

Updated the issue description and action list.

My question to all is the following: Shall we enable internal resistances for iprobe, vprobe and wprobe?

I am in favor of keeping them ideal. Users can add resistors as needed to make the components more realistic.

felix-salfelder commented 6 years ago

in the light of your updated to-do list ("nobody has implemented that anyway"), i suggest to stick to old/legacy probes.

newer code will have no problem with users bringing their own probes, if they like.

andresmmera commented 6 years ago

I'm also in favor of keeping them ideal. I admit I accepted and merged #682 but, honestly I think that the sources should be an ideal entity...

guitorri commented 6 years ago

Don't worry @andresmmera. I will push the fixes shortly.

guitorri commented 6 years ago

I will remove the ohmmeter.

The way it is implemented messes with the nasolver, trsolver, acsolver, dcsolver and the circuit baseclass just to get the information it needs. I don't think this is right approach. I am unsure about the uses of a dedicated ohmmeter in simulation. If the user need to know the impedance, open the branch he/she wants to inspect, add a source an probe in and that is it.

This was clearly targeted as an educational tool. The first failure mode is partially covered, if there are other sources actives the ohmmeter gives a NAN reading (which does not help much). The second mode can be confusing for beginners, if there are more than one ohmmeter on the same circuit it gives wrong results.

gildias commented 6 years ago

Hello,

With all the added modules on #682 being targeted towards engineering/electronics students, one of the main objectives was to simplifiy the usage of the simulator, as such, the ability to choose the desired frequencies for the phasor diagram is a great improvement, keeping this philosophy in mind. To clarify, in its current state, the Wave(Temporal) diagram does not display multiple frequencies at once.

The netlist was altered so a resistance was added in parallel to the necessary components, with the example of the ammeter, to mimic a "real" component. In the "real" components in which the internal resistance would result in a series with its other parameters, it was directly introduced to those components. Obviously this can also be achieved by adding another resistor to the circuit, but the addition of this function, even if a different approach is followed, will result in a more complete simulator.

The ohmmeter was implemented in a way that it could reach the same values as the voltmeter, as it needs that information to calculate the impedance in the branch, furthermore it should only cause interference whenever it is detected in the circuit. As you mentioned, if more than one ohmmeter exist in the same circuit, it will give a wrong result, this is probably because they act as separate current sources, and interfere with the values of one another, a possible way to solve this issue would be to return some warning when more than one ohmmeter is connected to the same circuit. Did not find a way to do this while still enabling multiple ohmmeters per simulation ( as multiple circuits are supported and functional ).

guitorri commented 6 years ago

Take a look at iprobe, vprobe and wprobe. The implementations for these probes are not leaking out of their respective .cpp files.
The ohmmeter is spreading implementation and logic into several simulation modes. Not to mention that it traverses the circuit at each time to find the CIR_OHMMETER. It does not seem right and it is going to be maintenance burden. It is almost like the ohmmeter requires a dedicated simulation mode of its own, to take care of disabling additional sources and checking for multiple definitions of ohmmeters on the same circuit.

Perhaps students should be taught to drive a unity voltage source and measure the current. One day they will be confronted with real circuits or with another simulator and they will need to know that much.

felix-salfelder commented 6 years ago

done. thanks

in3otd commented 6 years ago

It is almost like the ohmmeter requires a dedicated simulation mode of its own, to take care of disabling additional sources and checking for multiple definitions of ohmmeters on the same circuit.

I didn't try to use the ohmmeter but the above seems similar to what the S-parameters simulation does - computing the self and mutual (S-)parameters of the different ports (which then you can convert to Z-, Y- or other parameters).

Qucs / qucs

Review side effects of PR #682 #719