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

Migrate Properties to QList and Some Refactoring #858

Merged
merged 17 commits into from
Aug 2, 2024

Conversation

dsm
Copy link
Collaborator

@dsm dsm commented Jul 22, 2024

Hi, This PR migrate Properties to QList #748 , getFirst and getLast method name in Q3PtrList to front and back so QList, std::vector etc. has front and back so this changes will reduce refactoring effort.

we should test to ensure stability of this PR.

@ra3xdh ra3xdh added this to the 24.4.0 milestone Jul 23, 2024
@ra3xdh
Copy link
Owner

ra3xdh commented Jul 23, 2024

I will merge this short after the upcoming release.

@dsm dsm changed the title rename getFirst and getLast to front,back. Migrate Properties to QList Jul 23, 2024
@dsm
Copy link
Collaborator Author

dsm commented Jul 23, 2024

@ra3xdh Hi, this PR became a Properties migration to QList.

@dsm dsm force-pushed the rename_q3ptrlist branch 2 times, most recently from 982c62d to aceb064 Compare July 23, 2024 10:30
@dsm
Copy link
Collaborator Author

dsm commented Jul 23, 2024

there is bugs somewhere I don't find. I tested bjt noise example in latest snapshot build no problem but this PR release gives this in ngspice console I manually stop in macos because normally it run 1-2 sec.

alter=0: no such command available in ngspice
alter=0.10001: no such command available in ngspice
alter=0.20002: no such command available in ngspice
alter=0.30003: no such command available in ngspice
alter=0.40004: no such command available in ngspice
alter=0.50005: no such command available in ngspice
alter=0.60006: no such command available in ngspice
alter=0.70007: no such command available in ngspice
alter=0.80008: no such command available in ngspice
alter=0.90009: no such command available in ngspice

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 23, 2024

I tested bjt noise example in latest snapshot build no problem but this PR release

I will look at this. I am planning to start to review the refactoring PRs in a few days after the release. Is only BJT-noise example failing, or all parameter sweep simulations affected?

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 23, 2024

there is bugs somewhere I don't find.

Probably something in Param_Sweep::getNgspiceBeforeSim need to be rewritten to align with refactored properties. Also you may check the Component::getProperty

@dsm
Copy link
Collaborator Author

dsm commented Jul 23, 2024

if I open BJT.sch and BJT_swp.sch app crashed normally don't crash. somewhere in the code broken

@dsm
Copy link
Collaborator Author

dsm commented Jul 23, 2024

Bugs in component:load method for BJT.sch

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 24, 2024

Some devices (at least Diode) use custom load procedure for backward compatibility. Something like this may be applied for BJT too. I will look into this in the next few days.

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

I found a fix for crash loading BTJ.sch for equations section call Props.insert method and QList sometimes moves List to another memory location and this happend p1 iterator null and not valid below code is the solution.

  if (Props.size() < (counts >> 1)) {
    int index = std::distance(Props.begin(), p1);
    Props.insert(index + 1, new Property("y", "1", true));
    p1 = Props.begin() + index;
  }

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

finally i fixed parameter sweep.

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

Hi, I found a new issue active_mixer.sch sim results different.

this PR:
image

release 24.3
image

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 24, 2024

I found a new issue active_mixer.sch sim results different.

It's need to compare netlists from old and new versions. It seems some device entry is wrong formatted.

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

How I do this?

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 24, 2024

Save the both netlists from before/after and use some software like kdiff. It will show what entry is different.

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

24.3

XTRAN4 0  0 _net6 _net2 _net3 Transformers_PositiveCouplingPS K=0.99 L1=1 L2=1 Rp=1 Rs=1
XTRAN5 0  _net8 _net7 IF 0 Transformers_PositiveCouplingPS K=0.99 L1=1 L2=1 Rp=1 Rs=1

this PR

XTRAN4 0  0 _net6 _net2 _net3 Transformers_PositiveCouplingPS p=0.99 K=1 L1=1 L2=1 Rp=1
XTRAN5 0  _net8 _net7 IF 0 Transformers_PositiveCouplingPS p=0.99 K=1 L1=1 L2=1 Rp=1

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 24, 2024

Yes, I see now that the last property of Transformer is not loaded. I guess all library devices may be affected.

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

p=0.99 is it correct I think not

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 24, 2024

Yes, the property name K is not loaded. It seems the property names are shifted by one position.

@dsm
Copy link
Collaborator Author

dsm commented Jul 24, 2024

latest commit fixed the issue.

image

@dsm
Copy link
Collaborator Author

dsm commented Jul 25, 2024

@ra3xdh Hi, I recently push 2 commits. fixed build natively Qt 6.8 beta 2 with MSVC(VLAs not supported) and refactoring cmake file to switch c++20 and enable warning only for debug build.

Btw MSVC build not ok. I didn't test old version built with MSVC but this PR built with MSVC not working component drag or click crashed app or reload schematic also crashed app but no sense in debugger shows the crashed line maybe it's related Qt 6.8 beta 2, I'll switch 6.7.2 and looks any difference.

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 26, 2024

refactoring cmake file to switch c++20

Be careful with the latest C++ extensions. The Ubuntu-22.04 is not EOL and need to be supported till 2027. This distribution may not support some C++20 features. Need to check if this compiles and works using Ubuntu-22.04. But I have recently dropped support for Ubunutu-20.04 which is EOL next year.

Btw MSVC build not ok.

MSVC support may be dropped. If this will work with MSYS2+GCC/Clang this will be sufficient.

@dsm
Copy link
Collaborator Author

dsm commented Jul 26, 2024

ubuntu 22.04 has gcc-11.4 and it support c++20 and can be install gcc-12.3. Maybe I will test MSVC Clang(Clang-cl).

we can check qt version and it's greater than 6.7 then enable c++20 or c++17

@ra3xdh
Copy link
Owner

ra3xdh commented Jul 26, 2024

I have recently merged #845. @dsm, You may consider making the rebase of this PR.

@dsm
Copy link
Collaborator Author

dsm commented Jul 31, 2024

I remove latest 2 commit and rebase with current.

@ra3xdh
Copy link
Owner

ra3xdh commented Aug 1, 2024

I have finished with redesign of the ComponentDialog See dsm#1 The crashes seems to be gone now.

Redesign ComponentDialog::slotApplyInput
@dsm
Copy link
Collaborator Author

dsm commented Aug 1, 2024

it seems everythings works fine after your PR.

@ra3xdh
Copy link
Owner

ra3xdh commented Aug 1, 2024

I have found another crash while testing this PR. See #845 (comment) for context. The most probably it is related to #845 and not related to new Component::Properties.

@dsm
Copy link
Collaborator Author

dsm commented Aug 1, 2024

yes latest snapshot also has this issue and so it's not related to this PR.

@ra3xdh
Copy link
Owner

ra3xdh commented Aug 1, 2024

@dsm, Please apply the following patch on subcircuit.cpp Otherwise netlister adds File as the first parameter and simulation fails.

diff --git a/qucs/components/subcircuit.cpp b/qucs/components/subcircuit.cpp
index a8207c1e..61541112 100644
--- a/qucs/components/subcircuit.cpp
+++ b/qucs/components/subcircuit.cpp
@@ -230,8 +230,9 @@ QString Subcircuit::spice_netlist(bool)
         s += " "+nam;   // node names
     }
     s += " " + misc::properName(f);
-    for(Property *pp : Props) {
-        s += QString(" %1=%2").arg(pp->Name).arg(spicecompat::normalize_value(pp->Value));
+    for(int i = 1; i < Props.size(); i++) {
+      s += QString(" %1=%2").arg(Props.at(i)->Name)
+               .arg(spicecompat::normalize_value(Props.at(i)->Value));
     }
     s += "\n";
     return s;

@dsm
Copy link
Collaborator Author

dsm commented Aug 1, 2024

I think ,netlist method has the same issue

@ra3xdh
Copy link
Owner

ra3xdh commented Aug 1, 2024

Yes, Qucsator shows warning on File property, but the simulation works as expected. The Subcircuit::netlist also must be fixed.

@ra3xdh ra3xdh removed a link to an issue Aug 2, 2024
4 tasks
@ra3xdh ra3xdh merged commit 829182a into ra3xdh:current Aug 2, 2024
7 checks passed
@ra3xdh
Copy link
Owner

ra3xdh commented Aug 2, 2024

I didn't find any further problems with this PR and I am merging this. @dsm, Please stay in connection if something related will appear.

@ra3xdh ra3xdh linked an issue Aug 2, 2024 that may be closed by this pull request
4 tasks
@ra3xdh ra3xdh mentioned this pull request Aug 2, 2024
4 tasks
@ra3xdh ra3xdh modified the milestones: 24.4.0, 24.3.1 Sep 1, 2024
@dsm dsm deleted the rename_q3ptrlist branch September 5, 2024 14:32
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.

Get rid of Q3PtrList wrapper
2 participants