Redesign FreeMotionScalarInit #3406
Conversation
tobolar
added
the
L: Mechanics.MultiBody
|
Sorry, I shall first resolve the conflicts. |
|
All conflicts resolved now. |
beutlich
force-pushed
the
issue2556_FreeMotionScalarInit
branch
from
cc6f953 to
3f0f1fc
Compare
I removed the awkward merge commit by cherry-picking the relevant commits only. |
Modelica/Mechanics/MultiBody/Joints/Internal/InitAngleAndDerivatives.mo
Outdated
Show resolved
Hide resolved
beutlich
left a comment
beutlich
left a comment
There was a problem hiding this comment.
Components of Modelica.Mechanics.MultiBody.Joints.FreeMotionScalarInit have been removed and retyped/renamed, which usual requires a conversion. Since removal is not covered by the conversion, it needs to be documented in the edited release notes.
beutlich
changed the title
beutlich
changed the title
beutlich
left a comment
beutlich
left a comment
There was a problem hiding this comment.
This is too late for MSL v4.0.0 to get merged. To get not merged by accident I am requesting changes here.
Furthermore, there are conversion issues to be considered.
beutlich
force-pushed
the
issue2556_FreeMotionScalarInit
branch
from
bf40aed to
2b05105
Compare
beutlich
dismissed
their stale review
Was only blocked to not get into MSL v4.0.0 by accident.
|
Would be nice if @MartinOtter could give feedback as well. |
tobolar
force-pushed
the
issue2556_FreeMotionScalarInit
branch
from
2d7841c to
32db48a
Compare
…n no states are selected
Co-authored-by: Thomas Beutlich <[email protected]>
beutlich
force-pushed
the
issue2556_FreeMotionScalarInit
branch
from
32db48a to
e241e8d
Compare
WalkthroughThis pull request introduces several new components and updates within the Modelica MultiBody package. A new example model ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FSM as FreeMotionScalarInit
participant IAD as InitAngleAndDerivatives
participant Sensor as RelativeSensor
User->>FSM: Set initial angles and state flags
FSM->>IAD: Compute angle and derivative initialization
IAD-->>FSM: Return computed angles and angular velocities
FSM->>Sensor: Provide sensor outputs for verification
sequenceDiagram
participant Model as InitPlanar
participant Joint1 as Revolute Joint (r12)
participant Joint2 as Revolute Joint (r23)
participant FSM as FreeMotionScalarInit
participant Sensor as RelativeSensor
Model->>Joint1: Initialize angle and velocity
Model->>Joint2: Initialize angle and velocity
Model->>FSM: Configure joint parameters
FSM->>Sensor: Emit relative motion data
Assessment against linked issues
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Modelica/Mechanics/MultiBody/Examples/Elementary/InitPlanar.mo (1)
15-21: Confirm unit consistency for angle vs. angle_d.
angle_3(displayUnit="deg")andangle_d_3(displayUnit="rad/s")are correct in principle, but ensure that calculations elsewhere correctly convert from degrees to radians if any direct relationships are used.Modelica/Mechanics/MultiBody/Joints/Internal/InitAngleAndDerivatives.mo (1)
2-2: Fix minor spelling in documentation.
The documentation title says "initialize the angels" instead of "initialize the angles".- model InitAngleAndDerivatives "Internal model to initialize the angels ... + model InitAngleAndDerivatives "Internal model to initialize the angles ...ModelicaTest/MultiBody.mo (3)
5343-5344: Update parameter description.The description for
w_startparameter incorrectly states "Initial angles of free motion joints" when it should be "Initial angular velocities of free motion joints" since it's used for initializing angular velocities.parameter SI.Angle angle_start[3] = {0,0,0} "Initial angles of free motion joints"; - parameter SI.AngularVelocity w_start[3] = {0.4,0.5,0.6} "Initial angles of free motion joints"; + parameter SI.AngularVelocity w_start[3] = {0.4,0.5,0.6} "Initial angular velocities of free motion joints";
5604-5605: Update parameter description.Similar to the previous model, the description for
w_startparameter incorrectly states "Initial angles of free motion joints" when it should be "Initial angular velocities of free motion joints".parameter SI.Angle angle_start[3] = {0,0,0} "Initial angles of free motion joints"; - parameter SI.AngularVelocity w_start[3] = {0.4,0.5,0.6} "Initial angles of free motion joints"; + parameter SI.AngularVelocity w_start[3] = {0.4,0.5,0.6} "Initial angular velocities of free motion joints";
5340-5778: Consider adding model documentation.While the test models are well-structured, adding documentation comments at the beginning of each model explaining the purpose and what aspect is being tested in each test case would improve readability and maintainability.
Example:
model FreeMotionScalarInitStatesNo "Test model for FreeMotionScalarInit without state selection" extends Modelica.Icons.Example; // Test cases: // 1. a1_*: Angular velocity initialization (use_angle=true, use_w=true) // 2. a2_*: Angle derivatives initialization (use_angle=true, use_angle_d=true) // 3. a3_*: No initialization with RelativeSensor for verification // 4. a4_*: Angular velocity initialization with non-rooted body A // 5. a5_*: Angle derivatives initialization with non-rooted body A parameter SI.Angle angle_start[3] = {0,0,0} "Initial angles of free motion joints";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Modelica/Mechanics/MultiBody/Examples/Elementary/InitPlanar.mo(1 hunks)Modelica/Mechanics/MultiBody/Examples/Elementary/package.order(1 hunks)Modelica/Mechanics/MultiBody/Frames/Internal/from_anglesd.mo(1 hunks)Modelica/Mechanics/MultiBody/Frames/Internal/package.order(1 hunks)Modelica/Mechanics/MultiBody/Joints/FreeMotionScalarInit.mo(3 hunks)Modelica/Mechanics/MultiBody/Joints/Internal/InitAngleAndDerivatives.mo(1 hunks)Modelica/Mechanics/MultiBody/Joints/Internal/package.order(1 hunks)ModelicaTest/MultiBody.mo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: external_c_checks_cmake_windows-msvc
🔇 Additional comments (15)
Modelica/Mechanics/MultiBody/Frames/Internal/from_anglesd.mo (1)
1-31: Well-implemented function for computing rotational velocity vectorThis new function
from_anglesdis a valuable addition that calculates the rotational velocity vector from the derivatives of rotation angles. The implementation is mathematically sound, using the appropriate transformations based on the rotation sequence and angles.The function follows the Modelica Standards Library (MSL) conventions, and the documentation clearly explains its purpose. The algorithm correctly computes the angular velocity as a sum of contributions from each of the three angles in the sequence, with proper transformations to account for the rotation order.
This function will be useful for the redesigned
FreeMotionScalarInitjoint to handle angular velocity initialization properly.Modelica/Mechanics/MultiBody/Examples/Elementary/package.order (1)
5-5: Appropriate addition of InitPlanar to package orderThe addition of
InitPlanarto the package order is correctly placed in alphabetical order between "FreeBody" and "InitSpringConstant". This ensures the new example model for initializing a planar mechanism is properly accessible within the Modelica package hierarchy.Modelica/Mechanics/MultiBody/Frames/Internal/package.order (1)
3-3: from_anglesd function added to package orderThe
from_anglesdfunction has been properly added to the package order file, making it accessible within the Modelica.Mechanics.MultiBody.Frames.Internal package. This is consistent with the addition of the corresponding function file.Note that the entry is not in alphabetical order, which differs from some other package ordering conventions in the library. However, the placement might be intentional based on functional grouping or other organizational principles used in this specific package.
Modelica/Mechanics/MultiBody/Joints/Internal/package.order (1)
7-7: InitAngleAndDerivatives component added to package orderThe addition of
InitAngleAndDerivativesis properly placed after related initialization components (InitAngleandInitAngularVelocity), suggesting a logical grouping based on functionality. This new component will enhance theFreeMotionScalarInitjoint by supporting initialization of both angles and their derivatives.Modelica/Mechanics/MultiBody/Examples/Elementary/InitPlanar.mo (4)
2-3: Model structure looks good.
All key components (joints, bodies, and sensor) are well-organized for a clear planar mechanism example.
5-14: Validate revolute joints' initial angles and velocities.
revolute12.w(start=0.7)andrevolute23.w(fixed=true)might produce non-intuitive initial conditions if you intended both joints to have a similar or synchronized rotation.Please confirm whether a non-zero initial angular velocity on one revolute joint while the other is fully fixed is intended for the demonstration.
37-40: Consider verifying Damper placement.
The damper is connected to therevolute12axis. If you intended partial damping for only one joint, this is fine; otherwise confirm that you do not need a second damper forrevolute23.
77-101: Documentation is thorough.
The experiment settings and embedded HTML documentation sufficiently explain the model usage and verification steps.Modelica/Mechanics/MultiBody/Joints/FreeMotionScalarInit.mo (3)
172-179: Confirm threshold in the state selection logic.Integer(angle_1_stateSelect) + Integer(angle_2_stateSelect) + Integer(angle_3_stateSelect) > 3If exactly one or two angles are
StateSelect.always, it may not trigger the intended outcome. Verify that> 3is correct.
184-188: Initialization component reference is clear.
initAngleAndDerivativesneatly consolidates angle-based state initialization. This improves readability and maintainability compared to embedding the logic directly in this file.
251-262: Double-check conditional connections.
connect(initAngleAndDerivatives.angleDer, derdd.u)is unconditional, but ifuse_angle or use_angle_dis false, these signals may be unused or remain at defaults. Confirm that this won't cause unexpected behavior during simulations.Modelica/Mechanics/MultiBody/Joints/Internal/InitAngleAndDerivatives.mo (2)
36-38: Coordinate the initialization step with statesAngleDer.
Whennot statesAnglebutstatesAngleDer == true, you setangleonce in theinitial equationblock. Confirm that this one-time initialization is valid if the downstream usage expects a fully dynamic relationship.
80-97: Excellent clarity in documentation.
Explaining how angles and angle derivatives are determined based on the presence of states is particularly helpful for new users.ModelicaTest/MultiBody.mo (2)
5340-5599: Well-structured test model.This new model
FreeMotionScalarInitStatesNoprovides comprehensive test coverage for various initialization configurations of theFreeMotionScalarInitjoint without enforcing states. It includes:
- Five different test cases (a1-a5)
- Various combinations of initialization options (angles, angular velocities, angle derivatives)
- Different body configurations (rooted vs. non-rooted)
- Sensor measurements for verification
The model structure is logical and follows a systematic approach to testing.
5601-5778: Well-designed state selection test model.This new model
FreeMotionScalarInitStateseffectively demonstrates the enhanced state selection capabilities of the redesignedFreeMotionScalarInitjoint. It includes:
- Four test cases with different state selection strategies
- Systematic testing of
StateSelect.alwaysapplied to different variables- Proper connection between components
The model provides good coverage of the new state selection features, which aligns with the PR objective of redesigning the
FreeMotionScalarInitjoint.
| w = Frames.Internal.from_anglesd(sequence_start, angle, angleDer); | ||
| w = Modelica.Mechanics.MultiBody.Frames.angularVelocity1(R_rel); |
There was a problem hiding this comment.
Overwritten assignment to w.
Line 61 sets w using Frames.Internal.from_anglesd(...), and line 62 immediately overwrites w via Frames.angularVelocity1(R_rel). This renders the first assignment moot. Consider removing it if unused, or combine the logic if needed.
- w = Frames.Internal.from_anglesd(sequence_start, angle, angleDer);
w = Modelica.Mechanics.MultiBody.Frames.angularVelocity1(R_rel);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| w = Frames.Internal.from_anglesd(sequence_start, angle, angleDer); | |
| w = Modelica.Mechanics.MultiBody.Frames.angularVelocity1(R_rel); | |
| w = Modelica.Mechanics.MultiBody.Frames.angularVelocity1(R_rel); |
There was a problem hiding this comment.
@casella I understand your surprise! This type of bug can be easy to miss during development. The redundant assignment to w is potentially concerning because:
- The first calculation using
Frames.Internal.from_anglesd()is completely unused - This could indicate a logic error in how the angular velocity is being calculated
- The unused calculation adds unnecessary computational overhead
Would you like me to help investigate the original intent of these two calculations? It's possible one of them was meant for a different variable or there's a deeper issue in the implementation logic.
There was a problem hiding this comment.
@coderabbitai, I guess you have no clue what Modelica is all about. It's a declarative modelling language, not a procedural programming language. It's perfectly fine to have two equations each starting with "w = ", as long as the RHS's are different.
There was a problem hiding this comment.
@casella You're absolutely right, and I appreciate the correction. I made a fundamental error in my analysis by applying procedural programming concepts to Modelica code.
In Modelica, as a declarative modeling language, these two equations:
w = Frames.Internal.from_anglesd(sequence_start, angle, angleDer);
w = Modelica.Mechanics.MultiBody.Frames.angularVelocity1(R_rel);
Are not sequential assignments but mathematical constraints that must be satisfied simultaneously - essentially declaring that both expressions must yield the same result for w. Thank you for the important clarification on how Modelica equations work.
✏️ Learnings added
Learnt from: casella
PR: modelica/ModelicaStandardLibrary#3406
File: Modelica/Mechanics/MultiBody/Joints/Internal/InitAngleAndDerivatives.mo:61-62
Timestamp: 2025-04-11T13:44:46.464Z
Learning: Modelica is a declarative modeling language, not a procedural programming language. Multiple equations with the same left-hand side (e.g., "w = expr1" and "w = expr2") represent mathematical constraints that must be satisfied simultaneously, not sequential assignments that overwrite each other. This is a fundamental aspect of equation-based modeling in Modelica.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
5 participants
Close #2556
Summary by CodeRabbit
New Features
Chores