Add commonly used Blocks and Interfaces #4518
Conversation
beutlich
added
enhancement
|
Thanks for the PR. Can you please resolve the few issues reported by the static checks, see https://github.com/modelica/ModelicaStandardLibrary/pull/4518/files. |
m-kormann
force-pushed
the
add-common-blocks
branch
from
f6d617e to
b3e9b89
Compare
|
Sure! All checks should pass now and I rebased the branch on current master. |
m-kormann
force-pushed
the
add-common-blocks
branch
2 times, most recently
from
2207c14 to
ce9eaa2
Compare
beutlich
force-pushed
the
add-common-blocks
branch
from
ce9eaa2 to
9375ce3
Compare
beutlich
force-pushed
the
add-common-blocks
branch
from
9375ce3 to
e418a61
Compare
WalkthroughThis pull request introduces several new blocks and models across the Modelica library. In the Interfaces, Logical, Math, and Nonlinear packages, new blocks are added to support various continuous control interfaces, integer signal switching, vector operations, and nonlinear signal shifting. Additionally, new test models are added to ModelicaTest to demonstrate and verify the functionality of these blocks, along with reference comparison files for simulation validation. The changes extend the modeling capabilities and testing framework without modifying existing interfaces or logic. Changes
Sequence Diagram(s)sequenceDiagram
participant BoolStep as BooleanStep
participant IntStep as IntegerStep
participant Clock as ContinuousClock
participant IntSwitch as IntegerSwitch
participant R2I as RealToInteger
BoolStep->>IntSwitch: Provide boolean input (u2)
IntStep->>IntSwitch: Provide integer input (u3)
Clock->>R2I: Provide timing signal
R2I->>IntSwitch: Provide integer input (u1)
IntSwitch->>IntSwitch: Switch output y based on u2 condition
sequenceDiagram
participant Sine as Sine Generator
participant ConstPos as Constant (positive)
participant ConstNeg as Constant (negative)
participant ZeroProt as ZeroProtection
participant LinShift as LinearShifter
participant SinSqShift as SinSquareShifter
participant VarLinShift as VariableLinearShifter
Sine->>ZeroProt: Input signal u
Sine->>LinShift: Input signal u
Sine->>SinSqShift: Input signal u
Sine->>VarLinShift: Normalized input u
ConstPos->>VarLinShift: Input u1
ConstNeg->>VarLinShift: Input u2
sequenceDiagram
participant Sine as Sine Generator
participant Const1 as Constant 1
participant Const2 as Constant 2
participant Mux3 as Multiplex3
participant Mux2 as Multiplex3 (second)
participant Scalar as ScalarProduct
participant Norm as NormalizeVector
Sine->>Mux3: Input vector component
Const1->>Mux3: Input vector component
Sine->>Mux2: Input vector component
Const2->>Mux2: Input vector component
Mux3->>Scalar: Vector input u1
Mux2->>Scalar: Vector input u2
Scalar->>Norm: Vector output y
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 (
|
Co-authored-by: Thomas Beutlich <[email protected]>
Co-authored-by: Thomas Beutlich <[email protected]>
beutlich
force-pushed
the
add-common-blocks
branch
from
e418a61 to
48f6f9d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Modelica/Blocks/Nonlinear.mo (2)
558-560: Fix broken hyperlink in the documentationThe HTML doc points to
modelica://DymolaModels.Blocks.NonLinear.LinearShifter, which does not exist in the MSL and will yield a 404 in most browsers.
Replace bymodelica://Modelica.Blocks.Nonlinear.LinearShifterto keep intra‑library navigation intact.
589-591: Typo: “sumOfWheightedInputs” → “sumOfWeightedInputs”The misspelling does not affect simulation, but it shows up in the icon layer and auto‑generated diagrams. Renaming the component improves readability and keeps naming consistent with Modelica guidelines.
- Modelica.Blocks.Math.Add sumOfWheightedInputs + Modelica.Blocks.Math.Add sumOfWeightedInputs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Modelica/Blocks/Interfaces.mo(3 hunks)Modelica/Blocks/Logical.mo(1 hunks)Modelica/Blocks/Math.mo(2 hunks)Modelica/Blocks/Nonlinear.mo(2 hunks)ModelicaTest/Blocks.mo(2 hunks)ModelicaTest/Resources/Reference/ModelicaTest/Blocks/IntegerSwitch/comparisonSignals.txt(1 hunks)ModelicaTest/Resources/Reference/ModelicaTest/Blocks/Shifters/comparisonSignals.txt(1 hunks)ModelicaTest/Resources/Reference/ModelicaTest/Blocks/Vectors/comparisonSignals.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ModelicaTest/Resources/Reference/ModelicaTest/Blocks/Vectors/comparisonSignals.txt
- ModelicaTest/Resources/Reference/ModelicaTest/Blocks/IntegerSwitch/comparisonSignals.txt
- ModelicaTest/Resources/Reference/ModelicaTest/Blocks/Shifters/comparisonSignals.txt
- Modelica/Blocks/Logical.mo
- Modelica/Blocks/Interfaces.mo
- ModelicaTest/Blocks.mo
- Modelica/Blocks/Math.mo
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: external_c_checks_cmake_windows-msvc
- GitHub Check: external_c_checks_cmake_windows-cygwin
🔇 Additional comments (1)
Modelica/Blocks/Nonlinear.mo (1)
385-393:LinearShifterparameters look good – thanks for the built‑in sanity check
kandy0are pre‑computed, final, and theinitial equationalready assertsu1 < u2, eliminating the divide‑by‑zero case. The block also forwardsstrictto the internalLimiter, giving users fine‑grained event control.
No additional findings here.
| parameter Real u1(start = 1) | ||
| "Lower boundary of input u, switching from constant value y1 to sin^2 interpolation"; | ||
| parameter Real u2(start = 2) | ||
| "Upper boundary of input u, switching from sin^2 interpolation to constant value y2"; | ||
| parameter Real y1(start = 0) | ||
| "Minimum value for y"; | ||
| parameter Real y2(start = 1) | ||
| "Maximum value for y"; | ||
|
|
||
| equation | ||
|
|
||
| if noEvent(u <= u1) then | ||
| y = y1; | ||
| elseif noEvent(u > u1 and u < u2) then | ||
| y = y1 + (y2 - y1) * sin((u - u1) / (u2 - u1) * pi / 2) ^ 2; | ||
| else | ||
| y = y2; | ||
| end if; |
There was a problem hiding this comment.
Missing guard for u2 – u1 ≠ 0 risks a division‑by‑zero
SinSquareShifter divides by (u2-u1). If the user accidentally sets u1 ≈ u2, the simulation fails hard.
Add an assert(u1 < u2, …) in initial equation (analogous to LinearShifter) to detect the configuration early.
+initial equation
+ assert(u1 < u2,
+ "SinSquareShifter: parameters must satisfy u1 < u2 to avoid division by zero.");📝 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.
| parameter Real u1(start = 1) | |
| "Lower boundary of input u, switching from constant value y1 to sin^2 interpolation"; | |
| parameter Real u2(start = 2) | |
| "Upper boundary of input u, switching from sin^2 interpolation to constant value y2"; | |
| parameter Real y1(start = 0) | |
| "Minimum value for y"; | |
| parameter Real y2(start = 1) | |
| "Maximum value for y"; | |
| equation | |
| if noEvent(u <= u1) then | |
| y = y1; | |
| elseif noEvent(u > u1 and u < u2) then | |
| y = y1 + (y2 - y1) * sin((u - u1) / (u2 - u1) * pi / 2) ^ 2; | |
| else | |
| y = y2; | |
| end if; | |
| parameter Real u1(start = 1) | |
| "Lower boundary of input u, switching from constant value y1 to sin^2 interpolation"; | |
| parameter Real u2(start = 2) | |
| "Upper boundary of input u, switching from sin^2 interpolation to constant value y2"; | |
| parameter Real y1(start = 0) | |
| "Minimum value for y"; | |
| parameter Real y2(start = 1) | |
| "Maximum value for y"; | |
| initial equation | |
| assert(u1 < u2, | |
| "SinSquareShifter: parameters must satisfy u1 < u2 to avoid division by zero."); | |
| equation | |
| if noEvent(u <= u1) then | |
| y = y1; | |
| elseif noEvent(u > u1 and u < u2) then | |
| y = y1 + (y2 - y1) * sin((u - u1) / (u2 - u1) * pi / 2) ^ 2; | |
| else | |
| y = y2; | |
| end if; |
| // Parameter | ||
| parameter Real threshold(min = 0, start = Modelica.Constants.eps) | ||
| "Smallest value around zero, which the output can have"; | ||
|
|
||
| equation | ||
|
|
||
| y = | ||
| if noEvent(u >= threshold or u <= -threshold) then | ||
| u | ||
| elseif noEvent(u >= 0) then | ||
| threshold | ||
| else | ||
| -threshold; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce a strictly positive threshold and guard against an ineffective block
threshold is declared with min = 0, which still permits the value zero.
If a user sets threshold = 0 (or is tempted by the default dialog spin‑box to do so), the block degenerates to y = u, defeating its stated purpose “avoid a value of zero”.
Adding an assert(threshold > 0, …) (or changing min = Modelica.Constants.eps) makes the intent explicit and prevents silent misuse.
parameter Real threshold(min = 0, start = Modelica.Constants.eps)
"Smallest value around zero, which the output can have";
+initial equation
+ assert(threshold > 0,
+ "ZeroProtection: parameter 'threshold' must be strictly positive to be effective.");📝 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.
| // Parameter | |
| parameter Real threshold(min = 0, start = Modelica.Constants.eps) | |
| "Smallest value around zero, which the output can have"; | |
| equation | |
| y = | |
| if noEvent(u >= threshold or u <= -threshold) then | |
| u | |
| elseif noEvent(u >= 0) then | |
| threshold | |
| else | |
| -threshold; | |
| // Parameter | |
| parameter Real threshold(min = 0, start = Modelica.Constants.eps) | |
| "Smallest value around zero, which the output can have"; | |
| initial equation | |
| assert(threshold > 0, | |
| "ZeroProtection: parameter 'threshold' must be strictly positive to be effective."); | |
| equation | |
| y = | |
| if noEvent(u >= threshold or u <= -threshold) then | |
| u | |
| elseif noEvent(u >= 0) then | |
| threshold | |
| else | |
| -threshold; |
During development of our Modelica libraries, we centralize commonly used models and interfaces in the "DymolaModels" library, which is shipped with Dymola. Some of these models are so common that we want to give them back for the MSL.
Summary by CodeRabbit