Skip to content
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

#339 rebased (New matching techniques for the matching tool) #666

Merged
merged 5 commits into from
Oct 5, 2017

Conversation

in3otd
Copy link
Contributor

@in3otd in3otd commented Feb 6, 2017

here is #339 rebased to develop. As explained there, I had to squash all the commits to understand how to fix the conflicts. I also removed the whitespace changes done on existing code and undone moving some of the #include to matchdialog.h; my understanding is that we should try to have only forward declarations in the heders file when possible and including the definitions only in the .cpp files.

Did not yet check much, please check if it behaves as expected.

I saw anyway an error: by using the default values in the Matching Tool (just open it and press Create) the circuit created by the previous version and this one differs:

Previous version:
image

This PR version (was the same before rebasing):
image

If you define an Equation Defined RF device with some S-parameters to match you will see that this PR version of the matching tool draws the Port 2 circuit (on the right side) in the reversed order...

Besides, would be nice to keep the components values with units prefixes instead of the engineering notation and having the component text for vertical components positioned correctly.

Feel free to push corrections to this branch 😁

@in3otd in3otd added this to the 0.0.20 milestone Feb 6, 2017
@andresmmera
Copy link
Contributor

@in3otd @guitorri I created a number of schematics for testing the new matching tool.
Should I put them at qucs-test?

@guitorri
Copy link
Member

There are a few LaTex errors like:

! LaTeX Error: File `SingleStubOpen.eps' not found.

See the LaTeX manual or LaTeX Companion for explanation.
Type  H <return>  for immediate help.
 ...                                              
                                                  
l.256 ...egraphics[width=50mm]{SingleStubOpen.eps}

I think it always nice to have the examples. These will not test the matching tool itself, but will serve as a reference.

@in3otd
Copy link
Contributor Author

in3otd commented Feb 15, 2017

BTW, it seems that qucs-doc cannot be built correctly out of tree: dvips is not able to find the .eps figures (but continues anyway)

@guitorri
Copy link
Member

About .eps, I believe that if you do not give the extension LaTex (pdflatex in our case) will take care of the conversion. We have other .eps files in use... it should work.

@guitorri
Copy link
Member

Try removing the extension as in:

./tutorial/textmode/content.tex:	\includegraphics[angle=0,width=0.4\linewidth]{rc_ac_circuit}
...
./tutorial/textmode/pics/Makefile.am:  rc_ac_circuit.eps \

@in3otd
Copy link
Contributor Author

in3otd commented Feb 16, 2017

omitting the extension is the Right Thing but when I build out of tree I get some error messages:

$ make 2>&1 > /dev/null | grep "Could not find figure file"

/usr/share/texmf/bin/dvips: Could not find figure file TLpiece.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file CYexample.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file MNAexample.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file NLexample.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file MNAnoise1.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file MNAnoise2.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file Unoise.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file Real_r.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file Real_c.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file Real_ec.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file QuarterWavelengthFilter.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file SingleStubOpen.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file SingleStubShort.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file DoubleStubOpen.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file DoubleStubShort.eps; continuing
/usr/share/texmf/bin/dvips: Could not find figure file MNAsingular.eps; continuing

not sure that building the docs out of tree ever worked, actually.

@in3otd
Copy link
Contributor Author

in3otd commented Feb 17, 2017

@andresmmera I see that if matching cannot be done (e.g. when getting the message It is not possible to match this load using the double stub method) pasting from the clipboard is activated anyway, so if the clipboard contained parts previously copied there they are proposed to be pasted on the schematic.
This is because slotEditPaste() is always called even if the matching cannot be done.

What's the difference between

calcMatchingCircuitLC()
calcMatchingCircuitSingleStub()
calcMatchingCircuitDoubleStub()
calcMatchingCircuitCascadedLambda4()
calcMatchingCircuitCascadedLCSections()
calcMatchingCircuitLambda8Lambda4()

I have the impression they all do the same thing, they just pass the parameters to the appropriate ladder code building routine and then they all call SchematicParser() and CreateSchematic().
I think that in calcMatchingCircuit() we could just call the appropriate ladder code building routine and than have a common routine that calls SchematicParser() and CreateSchematic(), similarly to what is done in calcBiMatch().
I'll take a look at this and other minor stuff.

@andresmmera
Copy link
Contributor

Indeed they do basically the same... Surely the code ended up like this after moving code back and forth a year ago or so... I'll try to put that code in calcMatchingCircuit, where it should be.

Thanks for the code review 👍

@in3otd
Copy link
Contributor Author

in3otd commented Feb 18, 2017

no problem, the code works, that's just to make it easier to read in the future 😁
If you have the time to reorganize the code that's fine, I was thinking to do that also for some other minor things, like

  • CreateSchematic() always return true and in any case its return value is not used by the various calcMatchingCircuit*, so you can simply make them all return void. I think that the string returned by the various ladder code building routines (calc*()) should be checked and consider an empty string as an error so do not enable pasting from clipboard.
  • calc2PortMatch() also partially builds the schematic, which is not so nice, this should all be done by SchematicParser(); it receives also the schcode so it can know what needs to be done, also for two-port matching circuits. schcode can use a flag telling if it's a two- or single-port matching, something like QUCS_TWO_PORT_MATCH | QUCS_S_PARAM_SIM. One could maybe use scoped enums for this.
    II think i this way you can avoid passing the components, wires and paintings string and just receive back a single string containing all the schematic directly from SchematicParser().
  • the widget in the matching dialog sometimes use too little or too much space, making things difficult to read. The problem here is that every OS/window manager renders widgets in a slightly different way; at least check that when enlarging/shrinking the dialog the widgets resize/move in a nice way, this is usually enough to make it look good on other systems.
    Especially when the substrate parameters are shown things get sometimes difficult to read.

@guitorri guitorri mentioned this pull request Feb 18, 2017
@andresmmera
Copy link
Contributor

andresmmera commented Feb 19, 2017

I have just made a few changes so as to tackle the issues pointed out by @in3otd

  • The calcMatchingCircuit* functions were removed.
  • SchematicParser function now treats the S-param simulation block, the ports and the labels as the rest of components. This avoids using the schcode flags and reduces the number of arguments.

Pending stuff:

  • Check GUI behaviour when resizing
  • Update docs

@andresmmera
Copy link
Contributor

I've just used the 'clang-format' command to give a well defined coding style to matchdialog.cpp/h
I'm not sure if you like that...

@andresmmera
Copy link
Contributor

I couldn't work much on Qucs this week but, finally, I've updated the docs...
I'll do a last overall revision tomorrow...
Anyway, I reckon that it's more or less finished and bug-free now.

@in3otd
Copy link
Contributor Author

in3otd commented Feb 26, 2017

yes, thanks, I did some testing last week and didn't see any issue.
Only that having the dialog window non-resizable might be annoying; the dialog looks mostly fine here, but I'm sure there will be some combination of OS/window manager/screen DPI/etc. which will require adjusting the window size for some users.

Building docs fails here now; AppVeyor complains about some taperedline stuff (?)

@andresmmera
Copy link
Contributor

Uhm... I'll take a look later

@andresmmera
Copy link
Contributor

I've just allowed resizing & push the commit. I've also checked that no weird effects happen after resizing (I mean, fields overlapping or stuff like that)
I'm gonna see what's going on with the commit history...

@andresmmera andresmmera force-pushed the MatchingTool_develop branch 2 times, most recently from aee709d to 88d9bd3 Compare February 26, 2017 12:00
@andresmmera
Copy link
Contributor

I've just removed the unwanted commits and corrected a typo (\Ohm instead of \Omega) in the docs.

@ra3xdh ra3xdh mentioned this pull request Aug 7, 2017
11 tasks
Matching methods implemented:
- Single stub and double stub (balanced and not balanced)
- multistage lambda/4
- cascaded LC sections (for purely real loads)
- lambda/8 with lambda/4 transformer (complex load to real source)
All the methods above were implemented for one-port and two ports matching.

The generated schematic can optionally include the S parameter
simulation block.

Added microstrip synthesis to the matching tool

It was a added a new layout on the right of the matching tool window so as to define the substrate
properties for microstrip synthesis. In this sense, now it is possible to generate microstrip
matching networks by converting the ideal transmission lines to microstrip lines.
This feature is much the same as the microstrip synthesis available on 'qucs-filter'. Indeed, I
have literally copied the functions 'calcMicrostrip()' and 'getMicrostrip()' from there.

Added documentation of the various matching methods
to '/technical/synthesis.tex'.

It was written a schematic generator so as to minimise code
redundancy. This avoids to rewrite the same code twice (at single-
port and at two-port matching functions). In other words, all schematic
generation is done at "SchematicParser()" and all those
"Create2Port_<matching method>" are no longer needed

New function for creating schematics

It was added a function for building a schematic from the components,
wires and paintings strings.

Add Precision parameter to num2str()
to allow specifying the number of significant digits.
Default is set at 6, as it was implicitly before.
@in3otd in3otd force-pushed the MatchingTool_develop branch from a7b0961 to d79b0ed Compare August 12, 2017 15:44
in3otd added 2 commits August 12, 2017 17:45
so that their values will be nicely formatted
since "1 m" is interpreted as "1 milli" when unit is "m" it
should be left out unless a prefix is present.
@in3otd in3otd force-pushed the MatchingTool_develop branch from d79b0ed to 1f987be Compare August 12, 2017 16:04
@in3otd
Copy link
Contributor Author

in3otd commented Aug 12, 2017

I have rebased the PR again, I tried without squashing all the commits but there were too many conflicts.

While testing I saw another issue, when the length of an element (stub, line, etc.) was in the meter range this was written as 1.23 m but this is interpreted by Qucs as 1 milli (meter). I have then modified again num2str() to handle this case.

It was observed that the spar block was missing when selecting double
stub matching
The impedance value at the load label was wrong when it was purely
inductive or resistive
@guitorri
Copy link
Member

guitorri commented Oct 4, 2017

This looks really good to me.
Is there anything else you want to add/modify?

@andresmmera
Copy link
Contributor

As for me, I'm not planning to do anything else here (at least in the short run :-) )

@guitorri guitorri merged commit bc9f2c5 into Qucs:develop Oct 5, 2017
@guitorri
Copy link
Member

guitorri commented Oct 5, 2017

Very well. Thank you. Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants