Fix InterfaceVillasQueueless signal indexing, read handling, and start-of-sim sync

[leonardocarreras]

• Add the missing index increment in the export signal loop so multi-signal exports map to the right sample slots.
• Read path: replace the exit-on-error and retry-on-zero busy loop with a single read that logs and returns the last sequence; guard a null allocation.
• Take the sequence number from the sample's HAS_SEQUENCE field instead of assuming the first attribute carries it.
• syncImports blocks only when an import is marked to sync at start.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.42%. Comparing base (5780c9f) to head (8c427d1).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
dpsim-villas/src/InterfaceVillasQueueless.cpp	0.00%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
+ Coverage   72.14%   72.42%   +0.27%     
==========================================
  Files         491      491              
  Lines       31623    31634      +11     
  Branches    16950    16954       +4     
==========================================
+ Hits        22815    22910      +95     
+ Misses       8722     8647      -75     
+ Partials       86       77       -9     

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

[github-actions]

DPsim LLM review

Claim vs. code: matches the description.

TL;DR: One medium-risk code issue remains: the polymorphic InterfaceVillasQueueless class should declare a virtual destructor; the other findings are duplicate documentation/test coverage requests for the same public component, while the PR’s indexing, read handling, and start-of-simulation sync changes themselves did not surface additional correctness concerns.

Found 5 medium (0 anchored to lines below).

🔵 Optional / low-confidence (5)

• Add documentation page for InterfaceVillasQueueless [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Concepts/InterfaceVillasQueueless.md
• Add a Python test notebook for InterfaceVillasQueueless [medium · 35% confidence · unconfirmed] in tests/hugo/data/InterfaceVillasQueuelessTest.ipynb
• Add virtual destructor to polymorphic InterfaceVillasQueueless [medium · 35% confidence · unconfirmed] in dpsim-villas/src/InterfaceVillasQueueless.h
• Add documentation for InterfaceVillasQueueless component [medium · 35% confidence · unconfirmed] in docs/hugo/content/en/docs/Models/Concepts/InterfaceVillasQueueless.md
• Add test for InterfaceVillasQueueless behavior [medium · 35% confidence · unconfirmed] in tests/python/test_interface_villas_queueless.py

Claim vs. implementation

• Claimed: Fix InterfaceVillasQueueless export indexing, read robustness, sequence handling, and start-of-sim import sync behavior.
• Done: Adds the missing export index increment, makes readFromVillas do one read with null/failed-read handling and sequence extraction from HAS_SEQUENCE, and makes syncImports return early unless an import needs start sync.
• Difference: none

How this review was produced

13 specialized finder passes raised 48 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: 41 refuted as unsupported, 5 kept (5 tentative).

Refuted by verification:

• Incorrect sequence number extraction order in readFromVillas (dpsim-villas/src/InterfaceVillasQueueless.cpp): The file already sets seqnum from sample->sequence only when HAS_SEQUENCE is present, after the attribute loop.
• Add missing index increment in export signal loop (dpsim-villas/src/InterfaceVillasQueueless.cpp): createSignals increments idx exactly once per pushed export signal, including the added idx++ at line 118.
• Return last sequence on sample allocation failure (dpsim-villas/src/InterfaceVillasQueueless.cpp): On allocation failure the function intentionally returns mSequenceToDpsim, and the sample is null so no decref is needed.
• Use HAS_SEQUENCE flag for sequence number unconditionally (dpsim-villas/src/InterfaceVillasQueueless.cpp): The code does not unconditionally use sample->sequence; it gates it on HAS_SEQUENCE.
• Guard against null sample allocation in readFromVillas (dpsim-villas/src/InterfaceVillasQueueless.cpp): The failure path is before any successful allocation use; sample_decref is only needed for non-null samples, which are handled elsewhere.
• Use DOUBLE_EPSILON-style tolerance for read return code comparison (dpsim-villas/src/InterfaceVillasQueueless.cpp): This is a normal integer comparison on a return code, not a magnitude tolerance issue.
• Sequence number extraction order is fragile (dpsim-villas/src/InterfaceVillasQueueless.cpp): The later HAS_SEQUENCE check is the only sequence-source override in the file, so there is no conflicting unconditional overwrite.
• PreStep::execute may throw exceptions into the step loop (dpsim-villas/src/InterfaceVillasQueueless.cpp): PreStep::execute itself does not throw; the throw occurs inside readFromVillas and is caught in that function.
• readFromVillas performs dynamic memory allocation in the hot path (dpsim-villas/src/InterfaceVillasQueueless.cpp): The code uses node::sample_alloc from a pre-initialized sample pool, not an unbounded heap allocation.
• Add missing override specifier to PreStep::execute (dpsim-villas/src/InterfaceVillasQueueless.cpp): PreStep::execute is already defined here; the issue is not a missing override in this source file.
• Add missing override specifier to PostStep::execute (dpsim-villas/src/InterfaceVillasQueueless.cpp): PostStep::execute is already defined here; the issue is not a missing override in this source file.
• Remove redundant index increment in createSignals (dpsim-villas/src/InterfaceVillasQueueless.cpp): There is no extra increment beyond the single idx++ after each pushed export signal.
• Clarify error message for sample allocation failure (dpsim-villas/src/InterfaceVillasQueueless.cpp): The message already says 'could not allocate a sample', which is specific enough for the failure path shown.
• Improve comment clarity in syncImports (dpsim-villas/src/InterfaceVillasQueueless.cpp): The comment matches the code, which explicitly checks std::get<3>(attr) for syncOnSimulationStart.
• Use DEBUG log level for allocation failure in readFromVillas (dpsim-villas/src/InterfaceVillasQueueless.cpp): This is a policy/style complaint, not a code defect; the file intentionally logs allocation failure as an error.

_{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.}

[pipeacosta]

If I interpret this correctly, this change implies that the InterfaceVillas tries only once, what's the reason for this?

[leonardocarreras]

Hi @pipeacosta! Thanks for the question, is true that is a change on the way we do things. My way of thinking is that queued interface will work in a dedicated thread, so it can retry. And for the queueless, we read in the PreStep, so the simulation will block until we get data in the existing implementation.
I was doing some experiments with the RT mode on DPsim. We gain robustness in some edge cases, reusing the last sample if we have one miss... For some RT experiments, it blocks too long for one single de-sync, or it exits with error... So basically one best effort in reading, if nothing is there, we continue with an old sample instead of exiting with error... It might clash with some behaviour that depends on "new sample arrives, then we run one step", so sometimes will reuse an old sample for the external pacing source...
An intermediate solution would be making this behaviour as selectable option, would that be better in your opinion? We can use the blockOnRead argument for that (this is not implemented in that way currently) so is possible to have either of them

[pipeacosta]

Thanks for the explanation. I would stay with the philosophy of trying to keep real-time execution with the queueless interface, and use previous samples if needed. It would be good to log a warning in this case. Having said that, it is not necessary to implement the intermediate solution you mentioned imo.