Skip to content

Add negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace - #592

Merged
leonardocarreras merged 9 commits into
sogno-platform:masterfrom
leonardocarreras:feature/dp-avvsi-statespace-m11
Jul 31, 2026
Merged

Add negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace#592
leonardocarreras merged 9 commits into
sogno-platform:masterfrom
leonardocarreras:feature/dp-avvsi-statespace-m11

Conversation

@leonardocarreras

Copy link
Copy Markdown
Contributor

Extends DP::Ph3::AvVoltSourceInverterStateSpace from positive-sequence-only to dual-sequence current control, adding a negative-sequence measurement and PI current-control loop.

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.93%. Comparing base (9e864da) to head (dda24c3).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #592      +/-   ##
==========================================
+ Coverage   72.87%   72.93%   +0.06%     
==========================================
  Files         497      497              
  Lines       32305    32398      +93     
  Branches    17470    17551      +81     
==========================================
+ Hits        23542    23631      +89     
- Misses       8742     8766      +24     
+ Partials       21        1      -20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@georgii-tishenin georgii-tishenin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @leonardocarreras, thanks for the PR! From the code style/consistency and application of SSN/DP concepts I do not have any comments.
Here are some wishes:

  1. As myself I am one of the intended users, I would appreciate being able to select if I want a positive sequence only control scheme or positive + negative sequence. My motivation is mainly eigenvalue analysis: if we have some more states due to negative sequence control path, they will result in some more eigenvalues. I still want to be able to compare the positive sequence only case to the same control structure in EMT. If this configuration is hard to achieve with the same component, maybe component with negative sequence control can be a new one?
  2. Can you add documentation + references (if this control scheme is based on some reference?). Maybe this can trigger further ideas from @matthiasmees.

@leonardocarreras
leonardocarreras force-pushed the feature/dp-avvsi-statespace-m11 branch from da4fe6b to 9710094 Compare July 23, 2026 20:47

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

Found 2 medium (0 anchored to lines below).

🔵 Optional / low-confidence (2)
  • Add documentation page for new DP Ph3 AvVoltSourceInverterStateSpace with negative-sequence control [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Concepts/DP_Ph3_AvVoltSourceInverterStateSpace.md
  • Add pytest for DP Ph3 AvVoltSourceInverterStateSpace negative‑sequence control [medium · 35% confidence · unconfirmed] in tests/python/test_av_volt_source_inverter_state_space.py
Claim vs. implementation
  • Claimed: Extend DP::Ph3::AvVoltSourceInverterStateSpace from positive-sequence-only to dual-sequence current control with negative-sequence measurement and PI loop.
  • Done: Adds two negative-sequence control states, computes negative-sequence current feedback, injects a negative-sequence voltage reference, and exposes/logs the new attributes and parameters.
  • Difference: none
How this review was produced

13 specialized finder passes raised 16 findings over the diff and the full changed sources. After de-duplication, 16 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 14 refuted as unsupported, 2 kept (2 tentative).

Refuted by verification:

  • Incorrect negative-sequence current measurement derivation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The code intentionally computes negative-sequence measurement as 0.5 * K23 * expJPsi * std::conj(nI) and mirrors the same formula in updateLogAttributes.
  • Incorrect negative-sequence sensitivity coefficient hIrcN (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): hIrcN[p] is explicitly documented as the coefficient of conj(vc_p) in ircNDQ, so using projCoeff[p] here matches the conjugated negative-sequence projection.
  • Incorrect negative-sequence voltage reference transformation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): vRefNEnv0 is deliberately formed as K23 * std::conj(vRefNDQ) * expJPsi, matching the negative-sequence inverse-Park convention used in the surrounding code.
  • Incorrect derivative of negative-sequence voltage reference w.r.t. psi (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The derivative uses std::conj(ircNDQ) and std::conj(vRefNDQ) consistently with the conjugated negative-sequence reference defined at line 208.
  • Incorrect derivative of negative-sequence voltage reference w.r.t. gammaNQ (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The code sets dVRefNEnv0DGammaNQ = -j * K23 * expJPsi * mKiCurrCtrl, which is consistent with the conjugated vRefNEnv0 definition.
  • Incorrect base term for negative-sequence voltage reference derivatives w.r.t. phase variables (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): base is intentionally built from std::conj(hIrcN[p]) because the comment says the term comes via conj(vRefNDQ)'s conjugate-of-conjugate path.
  • Incorrect negative-sequence current measurement in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes uses the same negative-sequence measurement formula as buildStateSpaceModel, so the logged value is consistent with the runtime model.
  • Move noisy initialization log to DEBUG level (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The verbose initialization message is a pre-existing INFO log in the file and this diff did not change its level or placement.
  • Missing initialization of new negative-sequence state derivatives in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The new negative-sequence state derivatives are explicitly set with f(GammaND, 0) = mIRefNd - ircND and f(GammaNQ, 0) = mIRefNq - ircNQ.
  • Incorrect RHS vector size for new state vector (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The RHS vector is zeroed to Matrix::Zero(22, 1), matching the expanded 22-state model.
  • Incorrect Jacobian and input matrices sizes for new state vector (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): A and B are zeroed to Matrix::Zero(22, 22) and Matrix::Zero(22, 6), matching the new state dimension.
  • Incorrect C matrix column dimension for new state vector (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): C is sized as Matrix::Zero(6, 22), so the output matrix already matches the 22-state vector.
  • Incorrect sign for GammaND psi Jacobian entry (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The comment says d(ircND)/dpsi = -ircNQ, and the code sets A(GammaND, Psi) = ircNQ, which is the correct negative sign after applying f = iRefNd - ircND.
  • Incorrect sign for negative-sequence integrator psi derivative (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): A(GammaND, Psi) = ircNQ is consistent with the stated derivative of ircND and the minus sign in gammaND_dot = iRefNd - ircND.

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@leonardocarreras

Copy link
Copy Markdown
Contributor Author

Thanks @georgii-tishenin

  1. Selectable case: now is possible to use a constructor flag enableNegSeqControl (false is default)
  2. Docs and references: the full docu will be a follow-up, for the moment some comments are added

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Found 10 high, 11 medium (8 anchored to lines below).

🔴 Critical & high

  • Avoid dynamic allocation of Matrix f inside buildStateSpaceModel [high · 76% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:236 (details inline)
  • Avoid dynamic allocation of Matrix A and B inside buildStateSpaceModel [high · 76% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:257 (details inline)
  • Division by near-zero in negative-sequence current calculation [high · 72% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:209 (details inline)

🟡 Suggestions

  • Missing Doxygen comment for public setParameters method [medium · 75% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:96 (details inline)
  • Guard negative-sequence control path against division by zero in mRc [medium · 72% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:209 (details inline)
  • Potential NaN in negative-sequence current calculation [medium · 70% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:209 (details inline)
  • Remove redundant zero-initialization of Attributes in constructor body [medium · 61% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:50 (details inline)
  • Prefer parameter struct over long positional list [medium · 58% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:99 (details inline)
  • Group related parameters into a dedicated struct [medium · 55% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:99
    The constructor and setParameters expose a long positional list of scalar parameters (lf, cf, rf, rc, ...). This is error-prone and inconsistent with similar DP components that use a dedicated parameter struct for maintainability and safety.
    Fix: Introduce a struct AvVoltSourceInverterStateSpaceParameters containing all scalar parameters and provide a constructor overload that takes this struct. Keep the existing setParameters for backward compatibility if needed.
  • Remove redundant zero-initialization of derivative arrays [medium · 55% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:386
    dVRefNEnv0DPsi, dVRefNEnv0DGammaND, dVRefNEnv0DGammaNQ, and the derivative arrays are zero-initialized before the if(mEnableNegSeqControl) block. This is redundant because the block immediately overwrites them. It violates DRY and adds noise.
    Fix: Remove the redundant zero-initializations and declare the variables inside the if block to limit their scope and avoid dead stores.
🔵 Optional / low-confidence (11)
  • Incorrect negative-sequence current reference transformation [high · 35% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:220
  • Missing negative-sequence frame rotation in negative-sequence current measurement [high · 35% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:210
  • Negative-sequence current measurement uses wrong rotation [high · 35% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:210
  • Negative-sequence bridge-voltage reference uses wrong rotation [high · 35% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:220
  • Expose enableNegSeqControl parameter name in Python bindings [high · 35% confidence · unconfirmed] in dpsim/src/pybind/DPComponents.cpp:663
  • Missing Attribute dependency for mIrcNd/mIrcNq when negative-sequence control is disabled [high · 30% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:528
  • Side-effecting attribute update not tagged as external task [high · 30% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:528
  • Add documentation page for DP_Ph3_AvVoltSourceInverterStateSpace with dual-sequence control [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/DP_Ph3_AvVoltSourceInverterStateSpace.md
  • Outdated copyright header in pybind file [medium · 35% confidence · unconfirmed] in dpsim/src/pybind/DPComponents.cpp:1
  • Missing automated test for new negative‑sequence control [medium · 35% confidence · unconfirmed] in dpsim/examples/cxx/Components/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
  • Documentation not updated for optional negative‑sequence control [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Concepts/DP_Ph3_AvVoltSourceInverterStateSpace.md
How this review was produced

13 specialized finder passes raised 60 findings over the diff and the full changed sources. After de-duplication, 60 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 39 refuted as unsupported, 21 kept (11 tentative).

Refuted by verification:

  • Incorrect sign in negative-sequence integrator state derivatives (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): because f(GammaND/GammaNQ)=iRef-irc, the code's A entries are the negatives of d(ircN*)/dpsi and match the comment on lines 324-325
  • Incorrect derivative of negative-sequence reference w.r.t. psi (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): vRefNDQ depends on ircNDQ, so differentiating K23*conj(vRefNDQ)expJPsi includes the mKpCurrCtrlconj(ircNDQ) term used here
  • Incorrect rotation convention for expJPsi in DP state-space model (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the expJPsi definition is pre-existing/out of changed scope, and the positive path still uses rot=exp(-j*psi) for dq measurements
  • Derivative of negative-sequence bridge voltage w.r.t. psi uses wrong rotation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the derivative expression is consistent with the line-220 formula and the psi dependence of ircNDQ on line 210
  • Inconsistent rotation convention in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes recomputes expJPsi as conj(rot), the same convention used in buildStateSpaceModel at lines 152-153
  • Division by near-zero in positive-sequence current calculation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the positive-sequence division by mRc is pre-existing and not on a changed line
  • Division by near-zero in initialization loop (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the initialization loop checks abs(vc) before the division and this code is outside the changed lines
  • Use of ad-hoc epsilon literal instead of DOUBLE_EPSILON (dpsim/examples/cxx/Components/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the epsilon literals at line 176 are pre-existing and not part of the changed lines for this example
  • Use of ad-hoc literal j instead of Complex(0.0, 1.0) (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): j is explicitly declared as const Complex(0.0, 1.0), so later uses are not an ambiguous literal
  • Avoid dynamic allocation of Complex arrays dVRefNEnv0VcRe, dVRefNEnv0VcIm, dVRefNEnv0URe, dVRefNEnv0UIm inside buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the derivative buffers are fixed-size C arrays of three Complex values on the stack, not dynamically allocated arrays
  • Missing noexcept on buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the buildStateSpaceModel signature is not changed in this PR and the function itself does not throw std::invalid_argument
  • Add missing override specifiers to final class overrides (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): updateComponentParameters and updateLogAttributes are declared with override final on lines 80-81
  • Provide virtual destructor for polymorphic base class (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): the base MNASimPowerComp has a virtual destructor, and this derived class is final
  • Remove redundant zero-initialization of mIRefNd and mIRefNq (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): mIRefNd and mIRefNq have no default member initializers in the header; the constructor initializer list is their initialization
  • Use stateSize() consistently instead of hard-coded 20 (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the former hard-coded 20 sizes have been replaced with stateSize() at lines 236, 257, and 471

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp Outdated
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp Outdated
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp Outdated
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

Found 1 critical, 2 high, 5 medium (2 anchored to lines below).

🔴 Critical & high

  • State index layout mismatch when negative-sequence control is enabled [critical · 90% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:40 (details inline)

🟡 Suggestions

  • Prefer parameter struct over long positional list in setParameters [medium · 70% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:111 (details inline)
🔵 Optional / low-confidence (6)
  • Expose DP::Ph1::AvVoltSourceInverterStateSpace negative-sequence control parameters to Python [high · 35% confidence · unconfirmed] in dpsim/src/pybind/DPComponents.cpp:213
  • Use correct DP scaling for negative-sequence current references [high · 34% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:119
  • Plain C++ member used for externally relevant setpoint [medium · 35% confidence · unconfirmed] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:72
  • Add documentation page for DP_Ph3_AvVoltSourceInverterStateSpace [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/DP_Ph3_AvVoltSourceInverterStateSpace.md
  • Missing documentation for new DP::Ph3::AvVoltSourceInverterStateSpace component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Concepts
  • No Python test exercising the new negative‑sequence control [medium · 35% confidence · unconfirmed] in tests
Claim vs. implementation
  • Claimed: Add optional negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace, with new measurement/PI loop, extra states, and example/Python exposure.
  • Done: Implements an opt-in dual-sequence version of AvVoltSourceInverterStateSpace with two added negative-sequence states, control logic, logging, example runs for both modes, and pybind bindings.
  • Difference: none
How this review was produced

13 specialized finder passes raised 59 findings over the diff and the full changed sources. After de-duplication, 59 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 51 refuted as unsupported, 8 kept (6 tentative).

Refuted by verification:

  • Incorrect negative-sequence current reference frame transformation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The code intentionally defines rot = exp(-j*psi) and expJPsi = conj(rot), then uses expJPsi consistently for the negative-sequence loop.
  • Incorrect negative-sequence voltage reference frame transformation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): vRefNEnv0 is formed as K23 * conj(vRefNDQ) * expJPsi, matching the file’s chosen negative-sequence convention.
  • Incorrect sign in negative-sequence integrator state derivatives (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file’s comment explicitly states d(ircND)/dpsi = -ircNQ and d(ircNQ)/dpsi = ircND, and the Jacobian entries A(GammaND,Psi)=ircNQ, A(GammaNQ,Psi)=-ircND are the corresponding derivatives of f = iRef - irc.
  • Incorrect derivative of negative-sequence voltage reference w.r.t. psi (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): dVRefNEnv0DPsi is derived from vRefNEnv0 = K23*conj(vRefNDQ)*expJPsi, so expJPsi is the matching factor in this code path.
  • Incorrect negative-sequence current measurement rotation in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes uses the same expJPsi convention as buildStateSpaceModel for the negative-sequence measurement.
  • Guard negative-sequence control coupling with mEnableNegSeqControl (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): All negative-sequence computations are inside if (mEnableNegSeqControl), so the loop is already guarded when disabled.
  • Add validation for negative-sequence current references in setParameters (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): setParameters validates lf, cf, rf, rc, omegaN, omegaCutoff, and the integral gains; the finding claims missing validation but the file already has substantial checks.
  • Guard division by mRc in negative-sequence branch with DOUBLE_EPSILON (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): mRc is validated as positive at lines 58-59 before any division by mRc occurs.
  • Guard division by mRc in ircNDQ computation with DOUBLE_EPSILON (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The same positive rc check in setParameters covers the ircNDQ path, so there is no unguarded near-zero division in this file.
  • Guard division by mRc in updateLogAttributes negative-sequence branch (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes computes nI only after mRc has already been validated positive in setParameters.
  • Missing dependency declaration for state update task (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The component inherits dependency/task handling from MixedVTypeVariableSSNComp/MNASimPowerComp, and the claim about missing getModifiedAttributes is not supported by this file or the related sources.
  • Missing attribute dependency for input voltage in updateComponentParameters (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateComponentParameters calls inputAttribute() directly, but the related base class already provides the MNA dependency/task machinery; no missing declaration is shown here.
  • Plain C++ member updated without scheduler notification (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): mIRefNd and mIRefNq are plain members by design in this DP component and are consumed immediately in buildStateSpaceModel.
  • Conditional read of state indices without dependency guard (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The code explicitly reads GammaND/GammaNQ only when mEnableNegSeqControl is true, otherwise it uses 0.0.
  • Avoid dynamic Eigen allocation inside hot-path buildStateSpaceModel call (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The finding is about Eigen temporaries in a call site, but the file does not show a defect and the base/model update path is intentional.

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: One real robustness issue stands out: the new negative-sequence current references are accepted without validation, while the rest are lower-value design/test-coverage nits and one binding concern that appears already addressed in the shown diff. The PR intent and commit history do not indicate a DCO problem, and no other high-confidence correctness or scheduling issues surfaced.

Found 2 high, 5 medium (3 anchored to lines below).

🟡 Suggestions

  • Long positional parameter list in setParameters [medium · 80% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:111 (details inline)
  • Parameter passing improvement: use a dedicated parameter struct for setParameters [medium · 78% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:111 (details inline)
  • Missing validation for negative‑sequence current references [medium · 70% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:111 (details inline)
🔵 Optional / low-confidence (4)
  • Expose new negative-sequence current reference parameters in Python bindings [high · 35% confidence · unconfirmed] in dpsim/src/pybind/DPComponents.cpp:674
  • Missing DCO sign-off in commit history [high · 25% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/power-electronics.md
  • Add DP Ph3 AvVoltSourceInverterStateSpace to TEST_SOURCES [medium · 35% confidence · unconfirmed] in dpsim/examples/cxx/Components/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:107
  • Missing automated test for dual‑sequence inverter component [medium · 35% confidence · unconfirmed] in dpsim/examples/cxx/Components/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:27
Claim vs. implementation
  • Claimed: Extend DP::Ph3::AvVoltSourceInverterStateSpace from positive-sequence-only to optional dual-sequence current control, with a negative-sequence measurement/PI loop and updated example/docs.
  • Done: Adds an opt-in negative-sequence current-control path with new states/attributes, reordered state packing, pybind support, example dual-mode runs, and docs updates.
  • Difference: none
How this review was produced

13 specialized finder passes raised 88 findings over the diff and the full changed sources. After de-duplication, 88 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 81 refuted as unsupported, 7 kept (4 tentative).

Refuted by verification:

  • Incorrect sign in negative-sequence current measurement (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 200 uses expJPsi with std::conj(nI) exactly as the matching updateLogAttributes measurement does
  • Incorrect sign in negative-sequence voltage reference rotation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 210 intentionally forms vRefNEnv0 = K23 * conj(vRefNDQ) * expJPsi in the negative-sequence branch
  • Incorrect sign in negative-sequence integrator ψ-derivative coupling (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 315 documents d(ircND)/dpsi = -ircNQ and d(ircNQ)/dpsi = ircND, matching A(GammaND,Psi)=ircNQ and A(GammaNQ,Psi)=-ircND
  • Incorrect sign in negative-sequence voltage-reference ψ-derivative (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 380 computes the derivative of vRefNEnv0 using the same expJPsi/conjugation convention as the state equation
  • Inconsistent projection definition for negative-sequence (docs/hugo/content/en/docs/Models/power-electronics.md): The document already states the dq projection as e^{-j\psi},\underline{V}c and e^{-j\psi},\underline{I}{rc} on line 461, with no negative-sequence projection formula present.
  • Potential division-by-zero in negative-sequence current calculation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 58 in setParameters rejects rc <= 0.0, so the runtime division by mRc is guarded by parameter validation
  • Incorrect unpacking of complex envelope states in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 130 indexes vc with mVcReCol/mVcImCol and line 131 indexes iF with mIfReCol/mIfImCol, which are the correct per-phase columns
  • Incorrect indexing of input envelope vector in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 133 packs u as (u0,u1),(u2,u3),(u4,u5), matching the stated 6x1 real input layout
  • Incorrect loop bounds for envelope state derivatives in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 239 iterates over three phases and writes the envelope derivatives into the packed state indices used throughout the file
  • Incorrect state indexing in capacitor dynamics in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 332 uses the packed per-phase indices mVcReCol/mVcImCol, which are defined in the header as the VcA/B/C real and imaginary rows
  • Incorrect loop bounds for inductor dynamics in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 393 loops over three output phases and uses the packed mIfReCol/mIfImCol indices consistently
  • Incorrect unpacking of envelope states in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 501 unpacks x with mVcReCol/mVcImCol and mIfReCol/mIfImCol exactly as the packed state ordering in the header
  • Incorrect state indexing when initializing per-phase envelope states (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 612 initializes x0 using the same packed per-phase indices for Vc and If as the rest of the model
  • Division by coupling resistance mRc without guarding against near-zero (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 58 in setParameters throws if rc <= 0.0, so the division by mRc is not unguarded
  • Initialization loop uses ad-hoc tolerance instead of mInitializationTolerance (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): line 548 compares against mInitializationTolerance, the base-class constant

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@leonardocarreras
leonardocarreras force-pushed the feature/dp-avvsi-statespace-m11 branch from a5b3403 to cd7b8f9 Compare July 28, 2026 12:24

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: One medium-risk testability gap remains: the new dual-sequence example is not clearly wired into automated regression coverage, while the scheduler-dependent logging concern is low-confidence and may be a false positive; no stronger correctness issue was established from the provided passes.

Found 1 high, 2 medium (0 anchored to lines below).

🔵 Optional / low-confidence (3)
  • Missing scheduler dependency for new negative-sequence attributes [high · 35% confidence · unconfirmed] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:520
  • Add reference result files for the C++ example [medium · 35% confidence · unconfirmed] in dpsim/examples/cxx/Components/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:153
  • C++ example not registered as a test source [medium · 35% confidence · unconfirmed] in dpsim/examples/cxx/Components/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:27
Claim vs. implementation
  • Claimed: Add optional negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace, with a negative-sequence measurement/PI loop and updated example/docs.
  • Done: Implements an opt-in dual-sequence control mode with extra negative-sequence states, measurements, logging, Python bindings, and example runs for both modes.
  • Difference: none
How this review was produced

13 specialized finder passes raised 55 findings over the diff and the full changed sources. After de-duplication, 55 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 51 refuted as unsupported, 4 kept (3 tentative).

Refuted by verification:

  • Incorrect sign in negative-sequence current measurement (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file already uses ircNDQ = 0.5 * K23 * expJPsi * std::conj(nI); and the negative-sequence branch is intentionally baseband with expJPsi.
  • Incorrect rotation in negative-sequence voltage reference (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The code explicitly computes vRefNEnv0 = K23 * std::conj(vRefNDQ) * expJPsi;, matching the documented negative-sequence envelope form.
  • Incorrect sign in derivative of negative-sequence voltage reference w.r.t. ψ (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The derivative line is present and paired with the negative-sequence convention comment rotN = e^{+j*psi}; the claimed missing sign is not supported by the file.
  • Incorrect rotation in negative-sequence current logging (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes intentionally mirrors the same negative-sequence measurement formula used in buildStateSpaceModel, so this is not an inconsistent rotation.
  • Missing MNA stamp helpers for Norton admittance and history current (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The base class MixedVTypeVariableSSNComp already implements mnaCompApplySystemMatrixStamp and mnaCompApplyRightSideVectorStamp, so the derived component does not need to override them.
  • Incorrect initialization of negative-sequence integrator states (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file explicitly initializes the negative-sequence integrator states in the constructor and setParameters; the init routine only leaves them at the zero seed by design.
  • Documentation mismatch in initializeFromNodesAndTerminals signature (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): The header already declares void initializeFromNodesAndTerminals(Real frequency) override; and the base class also provides the same hook signature.
  • Use of absolute phase angle psi0 in initialization may break phase alignment (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The initialization code is the intended steady-state setup and the file comments state thetaN(0) = 0, so psi0 = theta0 exactly.
  • Division by coupling resistance mRc without guarding against near-zero (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): setParameters rejects nonpositive coupling resistance with if (rc <= 0.0) throw ..., so the division by mRc is guarded.
  • Conjugation of complex current in negative-sequence control without guarding (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): This is the same negative-sequence current formula as the main model path; the issue described is just the upstream mRc check, which already exists.
  • Use of ad-hoc tolerance 1e-12 instead of mInitializationTolerance (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The code uses mInitializationTolerance directly in the convergence check, not a hard-coded literal.
  • Use of ad-hoc literal 1.5 instead of K32 for power scaling (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): 1.5 appears in the steady-state initialization formula powerRef / (1.5 * vc); it is not the power-scaling expression claimed.
  • New negative-sequence attributes wrapped in Attribute<> unnecessarily (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): mIrcNd and mIrcNq are public-facing const Attribute<Real>::Ptr members, matching the established DPsim attribute pattern.
  • Conditional attribute update without set() call (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The attributes are updated via **mIrcNd = ... and **mIrcNq = ...; the claim about missing set() is not supported by the file.
  • buildStateSpaceModel allocates Matrix f with stateSize() each step (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): Matrix f = Matrix::Zero(stateSize(), 1); is a local stack object in buildStateSpaceModel, not a heap allocation claim.

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@leonardocarreras
leonardocarreras force-pushed the feature/dp-avvsi-statespace-m11 branch from cd7b8f9 to 0225d47 Compare July 28, 2026 12:56
@leonardocarreras
leonardocarreras marked this pull request as ready for review July 28, 2026 13:02
@leonardocarreras

Copy link
Copy Markdown
Contributor Author

Hi @georgii-tishenin, I think most of the concerns and requests are addressed, including the ones of the automated tests

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: Two high-severity issues remain: the new negative-sequence path lacks finite-value validation and can divide by a numerically tiny coupling resistance, while the notebook coverage item is unconfirmed and lower value; no other correctness, scheduling, or state-indexing problems were surfaced here.

Found 2 high, 1 medium (2 anchored to lines below).

🔴 Critical & high

  • Missing validation for negative-sequence reference parameters [high · 86% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:111 (details inline)
  • Division by near-zero in negative-sequence measurement when mRc is small [high · 72% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:199 (details inline)
🔵 Optional / low-confidence (1)
  • Notebook not registered in test suite [medium · 35% confidence · unconfirmed] in examples/Notebooks/Components/DP_Ph3_AvVoltSourceInverterStateSpace.ipynb
Claim vs. implementation
  • Claimed: Add optional negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace, with docs, bindings, example, and notebook updates.
  • Done: Implements an opt-in dual-sequence controller with negative-sequence measurement/PI loop, new state ordering, pybind accessors, example runs for both modes, notebook coverage, and documentation updates.
  • Difference: none
How this review was produced

13 specialized finder passes raised 87 findings over the diff and the full changed sources. After de-duplication, 87 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 84 refuted as unsupported, 3 kept (1 tentative).

Refuted by verification:

  • Incorrect negative-sequence projection formula in documentation (docs/hugo/content/en/docs/Models/power-electronics.md): The formula at line 610 explicitly uses the conjugated phasor after projection, matching the surrounding text that says the negative-sequence image is obtained by conjugating and rotating by +ψ.
  • Incorrect negative-sequence projection in implementation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file intentionally uses redistFactor=conj(projCoeff) for the negative-sequence branch at lines 139-141 and 196-197, matching the implemented symmetrical-components split.
  • Incorrect negative-sequence current rotation sign (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): ircNDQ is computed with expJPsi and then used consistently with the negative-sequence branch; the code also documents the +j*psi loop at line 188 and derives the same convention in updateLogAttributes at line 519.
  • Incorrect negative-sequence voltage reference rotation (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): vRefNEnv0 is explicitly formed as K23*conj(vRefNDQ)*expJPsi at line 210, and the same expression is used in the derivative block at line 373, so the finding contradicts the source.
  • Jacobian entries for negative-sequence integrators may be incorrect under disabled loop (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The negative-sequence Jacobian rows are guarded by if (mEnableNegSeqControl) at lines 316-328, so they are not computed unconditionally.
  • Derivative of negative-sequence voltage reference w.r.t. psi may be incorrect (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The derivative expression at line 380 is the same linearization used consistently for vRefNEnv0 in the surrounding code, not an isolated mismatch.
  • Typo in negative-sequence loop description (docs/hugo/content/en/docs/Models/power-electronics.md): The phrase is part of an intentional explanatory sentence about baseband operation and no second carrier; it is not a defect in the documented model.
  • Missing override on mnaCompApplySystemMatrixStamp and mnaCompApplyRightSideVectorStamp (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): MixedVTypeVariableSSNComp already declares mnaCompApplySystemMatrixStamp as a virtual hook, and this file does not override it; only updateComponentParameters/updateLogAttributes are overridden in the header.
  • Missing override on mnaCompUpdateVoltage and mnaCompUpdateCurrent (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file does not declare mnaCompUpdateVoltage or mnaCompUpdateCurrent overrides; those hooks remain in the base class and are not touched here.
  • Missing override on mnaCompPreStep and mnaCompPostStep (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file does not override mnaCompPreStep or mnaCompPostStep; the base class provides those hooks and this component only overrides updateComponentParameters/updateLogAttributes.
  • Incorrect state vector size used in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): buildStateSpaceModel indexes x using the component’s own StateIndex enum and stateSize() is used to size x0/A/B/C in the same file, so the claimed mismatch is not present here.
  • Incorrect per-phase envelope indexing in uEnv unpacking (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The unpacking comment and indexing are consistent with uEnv being built from u(0..5) at lines 133-134; there is no contradictory comment on this line in the current file.
  • Negative-sequence measurement uses expJPsi instead of exp(-j*psi) (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The negative-sequence measurement branch explicitly uses expJPsi at line 200, matching the same convention used in the rest of the negative-sequence code.
  • Negative-sequence reference rotation uses expJPsi instead of exp(-j*psi) (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): vRefNEnv0 is intentionally computed with expJPsi at line 210, and the derivative block later reuses that same form.
  • Per-phase redistribution of negative-sequence reference uses projCoeff instead of conj(projCoeff) (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The total reference intentionally adds the negative-sequence term with projCoeff[p] at line 216; the negative-sequence projection itself was already formed with redistFactor/conj(projCoeff) at lines 196-197.

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: One medium-risk review item remains: the new inverter parameter API now exposes a long positional argument list that is easy to misuse, while the rest of the change set did not surface additional correctness or scheduling issues. The main functional risk is maintainability and argument-order mistakes rather than an immediate runtime defect.

Found 1 medium (1 anchored to lines below).

🟡 Suggestions

  • Long positional parameter list in setParameters [medium · 82% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:158 (details inline)
Claim vs. implementation
  • Claimed: Add optional negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace, including measurement, PI loop, docs, bindings, and example coverage.
  • Done: Implements an opt-in dual-sequence controller with new negative-sequence states/attributes, updated state ordering, pybind and notebook/example changes, and documentation updates.
  • Difference: none
How this review was produced

13 specialized finder passes raised 97 findings over the diff and the full changed sources. After de-duplication, 97 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 96 refuted as unsupported, 1 kept (0 tentative).

Refuted by verification:

  • Incorrect negative-sequence measurement projection (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): computeNegSeqTerms intentionally uses redistFactor for nV/nU and the file comment says it is the negative-sequence measurement from that projection
  • Incorrect negative-sequence rotation sign in computeNegSeqTerms (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): ircNDQ is formed with expJPsi in computeNegSeqTerms and the file comment labels this as the baseband +j*psi loop
  • Missing negative-sequence integrator dynamics when loop disabled (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the negative-sequence RHS is guarded by if (mEnableNegSeqControl) and the state vector size is constructed as 10 or 8 in the constructor
  • Incorrect sign in negative-sequence integrator ψ coupling (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the Jacobian signs are documented in the adjacent comment and the code sets A(GammaND,Psi)=ircNQ and A(GammaNQ,Psi)=-ircND accordingly
  • Incorrect sign in negative-sequence ψ-sensitivity of vRefNEnv0 (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): sens.negPsi is computed as j * K23 * expJPsi * (...) in the negative-sequence sensitivity block, matching the code’s stated convention
  • Incorrect comment for buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the comment says psi := theta0 - thetaN (never a state itself), which matches the code using Psi as a state index elsewhere
  • Missing SPDX header in new header file (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): The file starts with SPDX-FileCopyrightText and SPDX-License-Identifier headers.
  • Missing include for in header (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): is included and Complex comes from included Definitions.h via the base header chain.
  • Incorrect sign in negative-sequence rotation expression (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): expJPsi is defined as conj(rot) where rot = exp(-j*psi), and the code uses that consistently for the negative-sequence terms
  • Incorrect negative-sequence measurement projection in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes uses conj(projCoeff[p]) for nV/nU exactly as written in the file and only when the negative loop is enabled
  • Division by coupling resistance without guarding against near-zero values (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): setParameters validates rc > 0 before any division by mRc, so the per-step path is not missing the only guard present in this file
  • Division by near-zero KiCurrCtrl in negative-sequence PI control (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): setParameters validates kiCurrCtrl != 0 before storing mKiCurrCtrl, so the claimed runtime division issue is not present in this code path
  • Missing runtime guard for mEnableNegSeqControl in state-space build (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): neg is only consumed after computeNegSeqTerms is conditionally called, and the constructor/state size already accounts for the enabled/disabled mode
  • Negative-sequence measurement computed even when loop is disabled (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the negative-sequence measurement is intentionally computed only inside if (mEnableNegSeqControl)
  • Re-invented epsilon literal in negative-sequence projection (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the code divides by mRc, not by a near-zero epsilon literal, and mRc is validated positive in setParameters

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

@georgii-tishenin georgii-tishenin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks so much for addressing my comments!😊

…rStateSpace

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
…es in the example

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
…fixed

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
…d flag

Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
@leonardocarreras
leonardocarreras force-pushed the feature/dp-avvsi-statespace-m11 branch from d276c62 to dda24c3 Compare July 31, 2026 14:09

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: Two high-severity issues remain in the new negative-sequence path: both compute (nV - nU) / mRc without a finiteness/zero guard, which can propagate NaN/Inf into control and logging; the only other item is a medium design nit about the long parameter list. The state ordering and opt-in control changes did not surface additional correctness concerns.

Found 2 high, 1 medium (3 anchored to lines below).

🔴 Critical & high

  • Division by coupling resistance without finiteness guard [high · 88% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:122 (details inline)
  • Missing finiteness guard when computing negative-sequence measurement in updateLogAttributes [high · 88% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:540 (details inline)

🟡 Suggestions

  • Prefer a parameter struct for setParameters [medium · 82% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:158 (details inline)
Claim vs. implementation
  • Claimed: Add optional negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace, including measurement, PI loop, docs, bindings, and examples.
  • Done: Implements an opt-in dual-sequence control path with new negative-sequence states/attributes, updated state-space/Jacobian logic, pybind exposure, docs, notebook, and example runs for both modes.
  • Difference: none
How this review was produced

13 specialized finder passes raised 62 findings over the diff and the full changed sources. After de-duplication, 62 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 59 refuted as unsupported, 3 kept (0 tentative).

Refuted by verification:

  • Add missing negative-sequence current reference parameters to setParameters documentation (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): The setParameters declaration already includes Real iRefNd = 0.0, Real iRefNq = 0.0 on lines 158-159.
  • Use consistent state vector size in buildStateSpaceModel (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): buildStateSpaceModel sizes A/B/C/D/E/F from stateSize() and the class constructor passes 10 or 8 real states, matching the optional negative-sequence states
  • Unpack psi from state vector using correct index (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the comment says psi is never a state itself, and the code indeed reads psi from x(Psi, 0) as the phase angle state
  • Use consistent naming for positive-sequence quantities (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): pV/pU are used for positive-sequence projections in the measurement and control equations, so the naming matches the code’s own-sequence convention
  • Guard negative-sequence computation with mEnableNegSeqControl check (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): NegSeqTerms is default-initialized with zero members in the header, and the code only uses neg.ircNDQ/neg.vRefNEnv0 after either zero default or computeNegSeqTerms
  • Use safe access to neg.ircNDQ with mEnableNegSeqControl guard (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): neg.ircNDQ is a Complex member defaulted to {0.0,0.0}, so the unconditional real()/imag() access is safe when the loop is disabled
  • Guard negative-sequence integrator Jacobian rows with mEnableNegSeqControl (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the GammaND/GammaNQ Jacobian rows are inside if (mEnableNegSeqControl), so they are only stamped when those states exist
  • Guard negative-sequence logging with mEnableNegSeqControl (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): mIrcNd and mIrcNq are updated only inside the mEnableNegSeqControl guard, so there is no unconditional logging of negative-sequence values
  • Division by current-control integral gain without finiteness guard (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): setParameters rejects kiCurrCtrl == 0.0 before this code can run, so the denominator is guarded against zero by the file itself
  • Use of ad-hoc literal for near-zero check instead of DOUBLE_EPSILON (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): this is the same division as the prior finding, and rc is validated positive in setParameters; the file does not use DOUBLE_EPSILON here, but that is a style choice not a defect
  • Potential catastrophic cancellation in negative-sequence measurement (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the subtraction nV - nU is the intended negative-sequence measurement formula in the comment and implementation
  • Uninitialized matrix entries in NegSeqTerms when loop is disabled (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): NegSeqTerms members are explicitly value-initialized to zero in the header, so the default construction is initialized, not uninitialized
  • Unconditional construction of NegSeqTerms even when loop is disabled (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): NegSeqTerms is a small stack object with zero-initialized members; constructing it unconditionally is intentional and harmless
  • Missing documentation for mIRefNd and mIRefNq (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): mIRefNd and mIRefNq are already documented with Doxygen comments on lines 80-83.
  • Missing SPDLOG log for negative-sequence measurement (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): the file already contains SPDLOG_LOGGER_INFO at initialization, and the new computeNegSeqTerms helper is a pure computation function with no logging requirement

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: Three real issues remain: the new negative-sequence path can divide by mRc without a minimum/finiteness guard, and setParameters still accepts NaN/Inf inputs that can poison the state-space model; the rest is a lower-value API-structure concern and the sequence-control/scheduling changes did not surface additional correctness issues.

Found 4 high, 1 medium, 1 low (6 anchored to lines below).

🔴 Critical & high

  • Missing finiteness validation for model parameters [high · 88% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:93 (details inline)
  • Missing finiteness validation for user‑provided parameters [high · 88% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:93 (details inline)
  • Division by mRc without finiteness guard [high · 82% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:122 (details inline)
  • Division by mRc without finiteness guard in updateLogAttributes [high · 80% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:540 (details inline)

🟡 Suggestions

  • Prefer parameter struct over long positional list in setParameters [medium · 76% confidence] in dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h:158 (details inline)
  • Missing Math::isFinite guard on division by mRc in computeNegSeqTerms [low · 70% confidence] in dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp:122 (details inline)
Claim vs. implementation
  • Claimed: Add optional negative-sequence current control to DP::Ph3::AvVoltSourceInverterStateSpace, including docs, bindings, example, and notebook coverage.
  • Done: Implements an opt-in dual-sequence control path with new negative-sequence measurement/PI loop, extra states and attributes, updated docs/bindings, and example/notebook demonstrations.
  • Difference: none
How this review was produced

13 specialized finder passes raised 84 findings over the diff and the full changed sources. After de-duplication, 84 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 78 refuted as unsupported, 6 kept (0 tentative).

Refuted by verification:

  • Incorrect sign in the negative-sequence dq rotation formula (docs/hugo/content/en/docs/Models/power-electronics.md): The file explicitly states the negative-sequence dq image is obtained by conjugating the projected phasor and rotating by +ψ, matching the claimed convention.
  • Incorrect redistribution factor for negative-sequence bridge-voltage reference (docs/hugo/content/en/docs/Models/power-electronics.md): The equation uses a_p with a_{a/b/c} = {1, a, a^2}, exactly the redistribution set the finding says is wrong.
  • Incorrect negative-sequence measurement projection (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): computeNegSeqTerms uses redistFactor only for nV/nU, but the file’s own comment and updateLogAttributes show the negative-sequence projection is the conjugate set via std::conj(projCoeff[p])
  • Incorrect sign in the negative-sequence integrator ψ-derivative (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The Jacobian signs match the code comment and the implemented equations: A(GammaND,Psi)=ircNQ and A(GammaNQ,Psi)=-ircND are exactly what the file states
  • Incorrect derivative of vRefNEnv0 w.r.t. ψ (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): sens.negPsi is computed as jK23expJPsi*(mKpCurrCtrl*conj(neg.ircNDQ)+conj(neg.vRefNDQ)), which is the same algebraic form the finding claims is wrong
  • Incorrect sign for the negative-sequence integrator derivative w.r.t. γ_{nd} (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): sens.negGammaND is set to K23expJPsimKiCurrCtrl, and the derivative of vRefNEnv0 w.r.t. GammaND is positive as implemented
  • Missing factor in the negative-sequence dq definition (docs/hugo/content/en/docs/Models/power-electronics.md): The displayed formula already includes the factor 1/2·sqrt(2/3)·e^{jψ}·conj(·), so there is no missing factor.
  • Clarify the redistribution set for the negative-sequence term (docs/hugo/content/en/docs/Models/power-electronics.md): The text says the negative-sequence output is redistributed through the sequence-orthogonal set and adds to the positive-sequence command, which is the intended description.
  • Add missing override specifier to setParameters (dpsim-models/include/dpsim-models/DP/DP_Ph3_AvVoltSourceInverterStateSpace.h): setParameters is declared in the file, and the base class only requires overrides for virtual hooks like updateComponentParameters/updateLogAttributes, not setParameters
  • Add missing override specifier to computeNegSeqTerms (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): computeNegSeqTerms is declared in the header and defined here with the exact same signature; no override is needed because it is a private helper, not a virtual base hook
  • Add missing override specifier to buildInductorRows (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): buildInductorRows is likewise declared in the header and defined here as a private helper, not a virtual override
  • Use explicit DOUBLE_EPSILON instead of ad-hoc 1e-12 (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The file does not use 1e-12 here; it divides by mRc directly, so this literal-based complaint does not match the code
  • Use explicit DOUBLE_EPSILON instead of ad-hoc 1e-12 in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): The updateLogAttributes path also uses mRc directly, not an ad-hoc 1e-12 tolerance
  • Negative-sequence measurement uses uninitialized mRc in computeNegSeqTerms (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): mRc is assigned in setParameters and initializeFromNodesAndTerminals explicitly checks mParametersSet before use; this is not an uninitialized-member case
  • Negative-sequence measurement uses uninitialized mRc in updateLogAttributes (dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp): updateLogAttributes is only called after initialization in this component, and mRc is set by setParameters before any use

Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.

Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
Comment thread dpsim-models/src/DP/DP_Ph3_AvVoltSourceInverterStateSpace.cpp
@leonardocarreras
leonardocarreras merged commit 17fc3dd into sogno-platform:master Jul 31, 2026
29 checks passed
@leonardocarreras
leonardocarreras deleted the feature/dp-avvsi-statespace-m11 branch July 31, 2026 14:43
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants