-
Notifications
You must be signed in to change notification settings - Fork 167
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
Implementation and test of UnwrapAngle (angle tracking observer) #3914
base: master
Are you sure you want to change the base?
Conversation
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.
Disregarding the implementation itself, my comments here:
- It is great to have such a block.
- IMO, the documentation is leaned too close to electrics. Maybe better to write it in a general way, just like e.g. WrapAngle do.
- The output
w
is useful, and shall probably be renamed to "dy", "der_y", "y_der", or something like that. (Also delete that string from the icon completely.) - A bit odd to use an element (Rotator) from Electrical within Blocks. But I understand it is needed here.
- Example
DemoATO
shall be renamed to not include an abbreviation. ATO is not explained. (And yes, I have no clue what it is.)
|
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'm a bit torn here. I really dislike the "shopping" for the Rotator
of a base component from deep within another domain library. So ideally I would see the Rotator
(and also the other phasor blocks) into Blocks.Math
. But this would automatically mean this extension being pushed towards the next MAJOR version. And who knows when that is going to happen.
Alternatively one could accept the "dirty" implementation for the 4.x series but create a 5.0.0 ticket at the same time where the move of those blocks will be executed.
@AHaumer One minor thing, the documentation of Rotator
says: "Rotates a space phasor (voltage or current) input u by the angle in negative mathematical direction." but the images shows the rotation happening in positive (anti-clockwise) direction. So which one is it?
I hope I don't mix it up now but it seems to be consistent: |
@AHaumer I still think you mix things up. The negative mathematical direction is clockwise. The arrow that is shown in the documentation is pointing anti-clockwise (i.e., positive direction) whilst the icon shows clockwise (i.e., negative direction). Now the main confusion here is that what is rotated is not the actual signal but the signal with regard to its reference coordinate system. I suggest changing the formulation so it says: (changes in bold) Now this should be a different ticket really. |
I think it's a good idea to improve the documentation of the |
I agree that |
Replaced angles phi and phi' by u and y
@AHaumer Regarding example |
Me too. |
* Icon: use common color of "signal" line and delete "w" * Documentation: minor corrections
In Simulink a block UnwrapAngle exists, in Modelica it's missing. I'm pretty sure that this is a block used in many applications, here is a robust implementation (angle tracking observer) of this functionality.