-
Notifications
You must be signed in to change notification settings - Fork 176
ImprovedOpAmpLimited #4643
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
ImprovedOpAmpLimited #4643
Conversation
|
I'll look into it. |
|
@HansOlsson this was by accident. |
|
Sorry @maltelenz I forgot to mention:
@GallLeo please generate new reference results for |
|
After merging this PR, I'll prepare another PR containing a remake of Modelica.Electrical.Examples.ChuaCircuit using realistic parameters (mH and pF insteaf of H and F), showing the path from 2 periodic attractors to chaos and an additonal example demonstrating how the characteristic of Chua's diode could be implemented with 2 OpAmp-circuits called NIC (negative impedance converter) - that example relies on the OpAmp-model of this PR. |
HansOlsson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general good, but I would need some feedback on the filter-part.
You mean the firstOrder? Give me some time over the weekend to design initialization. |
|
@HansOlsson I hope I addressed all your concerns, please re-review. |
henrikt-ma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a domain expert, so this set of changes is too big for me to comprehend. However, I am pleased to see that @maltelenz is reporting any consequences this will have in our test suite, and that @HansOlsson is paying attention to possibly unintentional stylistic changes.
|
Ok there a some cosmetic changes (my fault I know) in the examples.
Should I include a test model (meaybe in ModelicaTest?) measuring the rise time? |
|
wait wait wait ... silly me: I forgot to check / adapt the comparisonSignals. Then I want another change of the order of the examples. After that we get:
And I'll implement the correct measurement of rise time in ModelicaTest. All done after the following commit (latest this evening). |
nice order of examples new example MeasureRiseTime
christiankral
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if ImprovedOpAmpLimited is a good choice for a component name. One day IdealizedOpAmpLimited will be removed and we might not remember exactly what was improved ...
We might want to add a comment in |
|
@christiankral I accepted all your valuable suggestions with 9209342.
The new one is not only improved comparing wih the deprecated one, but also with the older three. |
christiankral
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK then, so we stick with the name ImprovedOpAmpLimited.
If I everybody else agrees with the name, it's fine with me, too...
|
Text for release notes: |
|
Unfortunately the discussion on the naming was too quick for me to react. I don't really think that "Improved" is a good attribute for any Modelica Standard Library model. Same as "New". It is new or improved now, and then it will be soon obsolete, as all things human. The name should describe what the model represents, not how its development history went. Why don't we just call it OpAmpLimited? It's not idealized, because it has a finite gain. So, it's just an op amp. @AHaumer do you agree? |
…ding the already merged OpAmp (modelica#4643 and modelica#4653)



This PR replaces #4485 which will be closed but we still can look up the discussions.
The following explanation could be used for future release notes:
Modelica.Electrical.Analog.Ideal.IdealizedOpAmpLimited has been declared as deprecated since some of the results (Modelica.Electrical.Analog.Examples.OpAmps) could not be compared between different tools, because the usage of noEvent() and smooth() for saturation prevented exact events in switching applications.
As a replacement Modelica.Electrical.Analog.Ideal.ImprovedOpAmpLimited has been implemented, thoroughly tested with different tools and used in the above mentioned examples. Saturation has been designed simpler, events are triggered by two Boolean variables that indicate saturation. Additionally a firstOrder models rise time of the OpAmp. Initialization is now without homopotopy acting on the result of the firstOrder (before saturation acts on the output).