Fix EMT::Ph3 averaged VSI without a connection transformer (#604)

[ysrji]

Fixes #604.

The segfault

AvVoltageSourceInverterDQ allocates three virtual nodes when withTrafo is false (setVirtualNodeNumber(3), leaving indices 0..2) while four sites dereference mVirtualNodes[3] unconditionally — three in initializeParentFromNodesAndTerminals, one in controlStep.

This resolves the control's measurement node once during initialization instead: virtual node 3 with a connection transformer, mTerminals[0]->node() without one. Without a transformer the filter interface is the component terminal, which is already how mSubResistorC is connected a few lines above. EMT_Ph3_VSIVoltageControlVCO was the reference, as suggested.

A second defect on the same branch

While confirming the fix I found that mnaCompUpdateCurrent publishes mSubResistorC->mIntfCurrent unnegated:

if (mWithConnectionTransformer)
  **mIntfCurrent = mConnectionTransformer->mIntfCurrent->get();
else
  **mIntfCurrent = mSubResistorC->mIntfCurrent->get();

The transformer is connected {terminal 0, virtual node 3} and Rc is connected {virtual node 2, terminal 0} — opposite orientations — so the two branches report opposite signs for the same physical current. controlStep() already applies exactly this negation when it feeds the same attribute to the Park transform, which I took as the intended convention.

I've included the one-line fix here since it is the same root cause (the branch was never exercised) and within the EMT::Ph3 scope you set, but I'm happy to split it out if you'd rather review it separately.

Verification

Both paths on the same network — 400 V, 12 kW load, inverter at a +4 kW setpoint behind a short LV feeder — run for 2000 steps at dt = 100 us:

	before	after
with_trafo=True	+4472.5 W, load bus 252.2 V	unchanged
with_trafo=False	segfault	+4480.4 W, load bus 252.2 V

The load bus voltage agreeing to within a volt across the two paths is what says the sign difference was in the published attribute and not in the network solution.

Scope

EMT::Ph3 only, as discussed. SP and DP have the same node-count split and can follow as separate PRs — happy to do those next if this shape looks right to you.

[leonardocarreras]

Hi @ysrji, if you agree to contribute, you should sign-off (this is to attest that you agree with the licensing and the terms of how dpsim works). The DCO is missing.

[github-actions]

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: One high-confidence scheduling bug remains: controlStep now reads the control-node voltage without declaring it as a dependency, which can race under parallel schedulers; the other items are low-confidence coverage/binding/docs gaps around the new EMT::Ph3 inverter path, and the segfault fix itself appears otherwise consistent with the PR intent.

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

🔴 Critical & high

• Missing attribute dependency for control node voltage [high · 88% confidence] in dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp:420 (details inline)

🔵 Optional / low-confidence (6)

• Missing Python binding for EMT::Ph3::AvVoltageSourceInverterDQ::mControlNode [high · 35% confidence · unconfirmed] in dpsim-models/src/pybind/EMTComponents.cpp
• Add documentation page for EMT_Ph3_AvVoltageSourceInverterDQ [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/EMT_Ph3_AvVoltageSourceInverterDQ.md
• Add pytest for EMT_Ph3_AvVoltageSourceInverterDQ with and without connection transformer [medium · 35% confidence · unconfirmed] in dpsim-models/tests/EMT_Ph3/EMT_Ph3_AvVoltageSourceInverterDQTest.cpp
• Add Python notebook demonstrating EMT_Ph3_AvVoltageSourceInverterDQ [medium · 35% confidence · unconfirmed] in dpsim-models/tests/EMT_Ph3/EMT_Ph3_AvVoltageSourceInverterDQTest.cpp
• Missing documentation for modified component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/EMT/Ph3/AvVoltageSourceInverterDQ.md
• Missing automated test for withTrafo=false path [medium · 35% confidence · unconfirmed] in tests/python/test_av_voltage_source_inverter_dq.py

Claim vs. implementation

• Claimed: Fix EMT::Ph3 averaged VSI segfault without a connection transformer and correct the published current sign on that path.
• Done: Adds a cached control measurement node chosen at initialization, uses it instead of unconditionally indexing virtual node 3, and negates the no-transformer interface current in mnaCompUpdateCurrent.
• Difference: none

How this review was produced

13 specialized finder passes raised 46 findings over the diff and the full changed sources. After de-duplication, 46 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, 7 kept (6 tentative).

Refuted by verification:

• Incorrect sign convention in filter interface current for Park transform initialization (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): initializeParentFromNodesAndTerminals negates filterInterfaceInitialCurrent consistently with controlStep’s -**mSubResistorC->mIntfCurrent
• Correct runtime negation in controlStep is consistent with MNA convention (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the negation in controlStep matches the component’s current orientation comment and the published interface convention
• Incorrect sign convention in mnaCompUpdateCurrent for non-transformer path (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the non-transformer branch intentionally negates mSubResistorC->mIntfCurrent to match the transformer branch’s published current sign
• Reuse of precomputed theta without validation across initialization and controlStep (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): theta is computed once during initialization and then intentionally reused to seed the PLL state/output
• Re-invented normalization factor in Park transform matrix (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the Park matrix uses sqrt(2./3.) exactly as written in the source
• Missing attribute dependency for mControlNode->mVoltage in addControlStepDependencies (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): addControlStepDependencies already declares mIntfVoltage and mIntfCurrent, and mControlNode is not an attribute dependency
• Missing attribute dependency for mIntfCurrent in addControlStepDependencies (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): mSubResistorC->mIntfCurrent is a subcomponent internal attribute, while the component dependency list already includes the published interface attributes
• mControlNode lacks const-correctness and documentation of ownership semantics (dpsim-models/include/dpsim-models/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.h): The member is already documented as a node pointer and the file shows no evidence of a const-ownership bug; public pointer idioms are accepted in this codebase.
• Use of uninitialized mControlNode in controlStep path (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): mControlNode is assigned during initializeParentFromNodesAndTerminals before any use in controlStep
• Use of EMT-domain instantaneous voltage for control in DP/SP contexts (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the class is EMT-specific and the code path uses EMT real-valued node voltages by design
• Add missing encapsulation for mControlNode (dpsim-models/include/dpsim-models/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.h): This is a protected data member, not a public one, so the encapsulation complaint contradicts the file.
• Remove redundant comment block (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the comment block is new in this file and documents the newly added mControlNode selection
• Misleading member variable name (dpsim-models/include/dpsim-models/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.h): The comment explicitly says it is a node reference resolved during initialization and used by controlStep(), which matches the name.
• Misleading comment (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the comment accurately distinguishes transformer-present versus absent control-node selection
• Move noisy initialization log to DEBUG level (dpsim-models/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp): the INFO log is a one-time initialization diagnostic and not a correctness defect

_{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]

Missing attribute dependency for control node voltage
severity: high · confidence: 88%

ControlStep reads the voltage of mControlNode (**mControlNode->mVoltage) but does not declare this attribute in its attributeDependencies list. When a connection transformer is present, mControlNode points to virtual node 3, whose voltage is distinct from the component's mIntfVoltage. Under parallel schedulers (ThreadLevelScheduler/OpenMPLevelScheduler) the missing dependency can lead to race conditions or stale voltage values, causing incorrect control actions without any compile‑time error.

Suggested fix: Add the voltage attribute of mControlNode to the attributeDependencies of ControlStep. Since mControlNode is a SimNode::Ptr, its voltage attribute is mControlNode->mVoltage (Attribute::Ptr).

Suggested change

	vcdq = parkTransformPowerInvariant(theta, **mControlNode->mVoltage);
	attributeDependencies.push_back(mControlNode->mVoltage);

Checked against the source: controlStep reads mControlNode->mVoltage, while addControlStepDependencies only declares mIntfCurrent, mIntfVoltage, and mVsref at lines 367-369

_{stage: scheduling-attributes}

[leonardocarreras]

Hi @ysrji ! Thanks for the interest in contributing.

Here just some small requests: sign-off with git commit -s (for the DCO), and use the pre-commit. Additionally, about comments is always better if the code is clear and we do not need them. If we do, please keep the comments short (1-2 lines) related to the code, not to the tests done or the experiments to discover, or the way is solved.

About the sign of the current, that change seems correct. Thanks for that fix too.

[leonardocarreras]

This comment is too long, and additionally does not help to the documentation. Here it should be

/// Help text of the thing below
VariableOrClass declaratedVarOrClass; 

[leonardocarreras]

A shorter comment is preferable

[leonardocarreras]

This comment is too long

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.68%. Comparing base (47b236e) to head (0be4199).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...dels/src/EMT/EMT_Ph3_AvVoltageSourceInverterDQ.cpp	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #611      +/-   ##
==========================================
- Coverage   72.87%   72.68%   -0.20%     
==========================================
  Files         497      497              
  Lines       32305    32307       +2     
  Branches    17470    17471       +1     
==========================================
- Hits        23542    23481      -61     
- Misses       8762     8769       +7     
- Partials        1       57      +56     

☔ 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.