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

Power combining (develop) #509

Merged
merged 14 commits into from
Oct 9, 2017
Merged

Conversation

andresmmera
Copy link
Contributor

Hello there,

This PR is aimed to replace PR #346 .

In addition to the existing features, it was added the LC equivalent of the Wilkinson power combiner (both single-stage and multistage).

On the other hand, it was added a panel on the right side of the window so as to display a preview of the current topology.
selection_054

selection_055

@andresmmera andresmmera added this to the 0.0.20 milestone Apr 25, 2016
@andresmmera andresmmera mentioned this pull request Apr 25, 2016
@ra3xdh ra3xdh mentioned this pull request Jan 29, 2017
11 tasks
@in3otd
Copy link
Contributor

in3otd commented Apr 22, 2017

nice work! I'm doing some testing, here are some random comments:

  • general:

    • do not disable window resizing, someone somewhere on some platform will need to resize it. I saw the comments in the related commit but, still, what's the advantage for the user in disabling resize? Can the aspect ratio of the SVG images be fixed ? (I seem to remember a similar problem with the Active Filter tool images)
    • disable "number of outputs" (visible but not active) for configurations when only 2 outputs are supported
    • S-parameter simulation start/stop frequencies are wrong when the frequency units selected are not GHz. Or also when entering numbers < 1 in the frequency box.
    • bonus point: change length units for the lines to have always numbers between 1 and 999 (engineering notation). I guess is safe to use misc::num2str() for everything that needs to be human readable.
  • if you select Bagley and then Gysel the Number of outputs shows 3, 5 and 7 but Gysel here supports 2 outputs only

  • some Topologies generate the equation for S11_dB, some don't; I think it's better to have it always (maybe also for all the Snn terms)

  • Wilkinson Multistage

    • Microstrip Implementation and Lumped Elements Implementation should probably be radio buttons even if the current implementation works fine
    • with Microstrips: the first microstrip has no W or L shown
  • Tree

    • line parameters are not shown (I guess it's a choice for space reasons, right?)
    • microstrip implementation has something wrong, seems centered to half the design frequency

@in3otd
Copy link
Contributor

in3otd commented Apr 22, 2017

also you should set the main window icon

setWindowIcon(QPixmap(":/images/bitmaps/big.qucs.xpm"));

see e.g. the QucsActiveFilter() constructor.

@andresmmera
Copy link
Contributor Author

👍
Ok, I'm gonna take a look.
Many thanks @in3otd !!

@andresmmera
Copy link
Contributor Author

I think I've almost fixed all the issues you pointed out. I'll probably push tomorrow.
Just a few comments:

do not disable window resizing, someone somewhere on some platform will need to resize it. I saw the comments in the related commit but, still, what's the advantage for the user in disabling resize? Can the aspect ratio of the SVG images be fixed ? (I seem to remember a similar problem with the Active Filter tool images)

Well, I did so because at some point I thought it would be good to hide the substrate panel when the corresponding QCheckbox was unchecked. Somehow, that forced me to fix the window size. Anyway, I ended doing the same stuff the Active Filter tool does (this)

(Tree wilkinson combiner) line parameters are not shown (I guess it's a choice for space reasons, right?)

Yes, of course. Otherwise, it becomes a mesh...

bonus point: change length units for the lines to have always numbers between 1 and 999 (engineering notation). I guess is safe to use misc::num2str() for everything that needs to be human readable.

Ok, done. The only inconvenient is some corner case. For example, at very high freqs the user may select 'mils' and the final result might be given in 'nm'... but I guess nobody is going to use this tool at 3 THz or so :-)

@guitorri guitorri self-assigned this Oct 6, 2017
@guitorri
Copy link
Member

guitorri commented Oct 6, 2017

The microstrip implementations do not have a corresponding .svg figure right?
Do you think is worth adding?

@guitorri
Copy link
Member

guitorri commented Oct 6, 2017

The Travelling wave does not create the S11_dB Equation. (no big deal)

@guitorri
Copy link
Member

guitorri commented Oct 6, 2017

For different number of outputs the figures are also not there. I guess we can leave it as is, doesn't make sense to create figures for all combinations. (Some day we will be able to use the schematic as a library so we can render the schematic templates and get a figure back...)

Let me know if you want to change something else.

@andresmmera
Copy link
Contributor Author

andresmmera commented Oct 6, 2017

The microstrip implementations do not have a corresponding .svg figure right?
Do you think is worth adding?

Honestly, I didn't really think about that. I just wanted to give a visual clue about the combiner structure. I can create the corresponding svg files for the microstrip implementation (hopefully) this weekend.

The Travelling wave does not create the S11_dB Equation.

My bad. Didn't notice that...

For different number of outputs the figures are also not there.

Yes, I did that because otherwise the schematic becomes a mesh when you select a big number of outputs. I know that the isolation between output ports (i.e., Sxy, x != y != 1, ) is an important figure, but I thought that it would be more convenient if the user adds the equation manually in case of he/she's interested on that.

Let me know if you want to change something else.

I'll add the S11 equation for the TW and tree combiners and then I'll add the figures for the microstrip.

@andresmmera
Copy link
Contributor Author

I've just pushed two commits in order to address the issues you mentioned yesterday

@andresmmera
Copy link
Contributor Author

I don't know why Travis fails... I rerun it just in case, but the error is still there.
It says:

 rm -f TAGS ID GTAGS GRTAGS GSYMS GPATH tags
rm -f cscope.out cscope.in.out cscope.po.out cscope.files
make[2]: Leaving directory `/home/travis/build/Qucs/qucs/qucs-0.0.19/_build'
rm -f config.status config.cache config.log configure.lineno config.status.lineno
rm -f Makefile
ERROR: files left in build directory after distclean:
./qucs/qucs/qucs_.cpp
make[1]: *** [distcleancheck] Error 1
make[1]: Leaving directory `/home/travis/build/Qucs/qucs/qucs-0.0.19/_build'
make: *** [distcheck] Error 1

Any guess?

@felix-salfelder
Copy link
Member

i don't know what triggers this. 6e3420e seems to fix it

@andresmmera
Copy link
Contributor Author

👍 Thanks

I'll wait until you merge that and then I'll rerun Travis again.

@felix-salfelder
Copy link
Member

#722

@andresmmera
Copy link
Contributor Author

Cool!
Thanks @felix-salfelder

andresmmera and others added 9 commits October 9, 2017 20:00
This is a tool for automatically create power combiners. It includes
some popular power combining structures as well as corporate combiners.

In addition to the old branch (-> master) features, it was added the LC
equivalent of a 2-way Wilkinson combiner (both single-stage and
multistage). Moreover, it was included a new panel for diplaying the
combiner topology.
* Removed unused #includes
* Added some comment
Travis CI cannot find "qucspowercombiningtool.h" header, so this commit
is another attempt to solve the problem
* The window resizing policy was set to 'fixed', however, its size is
scaled beforehand taking into account the screen dimensions so as to
ensure a nice appearance:

   QDesktopWidget dw;
   QRect screenSize = dw.availableGeometry(this);
   DefaultSize = QSize(screenSize.width() * 0.5, screenSize.height() *
0.6);//Hide substrate tab
   ExtendedSize = QSize(screenSize.width() * 0.8, screenSize.height() *
0.6);//Show substrate tab
   this->setFixedSize(DefaultSize);

* It was added a rounding function so as to avoid having irrelevant
decimals in the components parameters

* Moreover, it was added a combobox to let the user to choose the length
units (mm, um, mil,...)
In commits before, the output power ratio is given in n.u. In order to
present a more meaningful parameters, the output power ratio is now
expressed in dB
This commit fixes includes the following:

i) The window is again resizable. Basically, the svg image is fixed
according to what is done in qucs-activefilter

ii) The combobox for selecting the number of outputs is unabled for
those topologies that do not allow >2 branches

iii) Fixed the fstart/fstop in the S parameter block

iv) The length units are changed automatically in order to show numbers
\in [1, 999.99] only

v) The equations block now includes dB(S[n, n]) where n = 1:(Number of
outputs)

vi) It was added a groupbox with radiobuttons so as to let the user
choose the implementation (ideal TL, microstrip or CLC (only Wilkinson))

vii) Fixed microstrip implementation in the tree combiner (the impedance
of the output branches was wrong)
andresmmera and others added 5 commits October 9, 2017 20:00
if the selected power combiner type does not support the
lumped elements implementation
This commit also includes a slight rearrangement of code. Specifically,
the code related to the image update was concentrated in one function,
which is called by different radiobutton event handlers.
@guitorri
Copy link
Member

guitorri commented Oct 9, 2017

I rebased it locally. History is getting messy.
Can I force push to your branch? Please enable "Allow edit from maintainers" on the rigth.

@andresmmera
Copy link
Contributor Author

Feel free to do whatever you need.
The "Allow edit from maintainers" checkbox is already enabled

@guitorri
Copy link
Member

guitorri commented Oct 9, 2017

Thank you. I was giving the wrong branch name for update...

@guitorri guitorri merged commit 591835b into Qucs:develop Oct 9, 2017
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.

4 participants