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

Implement RL node reduced equations #283

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

martinmoraga
Copy link
Contributor

This PR implements a new component "ResIndSeries" in the SP, DP and EMT (3Ph) domains which models the series connection of a resistor with an inductor. The new component is validated against a RLC solved using the basic components (R, L, C) of DPsim. Moreover, virtual nodes and snubber components of PiLines and PowerTransformers are removed to improve the performance and accuracy of simulations.

@martinmoraga martinmoraga added the enhancement New feature or request label Mar 22, 2024
@martinmoraga martinmoraga self-assigned this Mar 22, 2024
m-mirz and others added 25 commits March 26, 2024 11:18
Signed-off-by: Martin Moraga <[email protected]>
Signed-off-by: Martin Moraga <[email protected]>
Signed-off-by: Martin Moraga <[email protected]>
Signed-off-by: Martin Moraga <[email protected]>
Signed-off-by: Martin Moraga <[email protected]>
Signed-off-by: Martin Moraga <[email protected]>
Signed-off-by: martin.moraga <[email protected]>
Copy link

sonarcloud bot commented Mar 26, 2024

Copy link
Collaborator

@georgii-tishenin georgii-tishenin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Martin,
Thank you for this nice enhancement. This is the proper way to model series RL branches! Also, really cool that you did implementation in all of the modeling domains and added a nice documentation.
I have added a few comments for your consideration, none of them are major.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be great to also add EMT domain equations here for series RL element, as EMT model is included in this PR.

dpsim-models/src/DP/DP_Ph1_ResIndSeries.cpp Show resolved Hide resolved
Real preCurrFracImag = (-2. * b) / (1. + b * b);

Real preCurrFracReal =
(1. - std::pow(b, 2) + -std::pow(**mResistance * a, 2)) /
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a + - before std::pow(**mResistance * a, 2)) on purpose? Can it be just -?

Real equivCondReal = a / (1. + b * b);
Real equivCondImag = -a * b / (1. + b * b);
Real equivCondReal = (a + **mResistance * std::pow(a, 2)) /
(std::pow(1. + **mResistance * a, 2) + std::pow(b, 2));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

denominator (std::pow(1. + **mResistance * a, 2) + std::pow(b, 2)) is calculated several times. Calculating this once and storing the result in a variable would make the code easier to read.

dpsim-models/src/DP/DP_Ph1_ResIndSeries.cpp Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResIndSeries is quite a clear name, but does not sound too nice when you try to pronounce it aloud.
I've noticed in DPsim there are two common naming conventions for the component classes:

  • Either not abbreviating the component's name at all, like EMT_Ph3_SeriesResistor.

  • Or abbreviating the component's name by a single letter, like EMT_Ph3_RXLoad.

Following these conventions, I'd propose DP_Ph1_SeriesRL, DP_Ph1_SeriesResistorInductor. Same goes also for EMT, SP and 3-phase classes, modelling ResIndSeries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is my fault :-) When I made the first commit I came up with this name but I agree with your comment. SeriesRL would be better indeed.

if (auto mnasubcomp = std::dynamic_pointer_cast<MNAInterface>(subcomp))
mnasubcomp->mnaApplySystemMatrixStamp(systemMatrix);

if (terminalNotGrounded(0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can log this in the same place that the code is (with the equations) on line 221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

5 participants