Add InterfaceCosimSync for real-time DPsim co-simulation start-time synchronization - #581
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 72.14% 72.16% +0.01%
==========================================
Files 491 493 +2
Lines 31623 31907 +284
Branches 16950 17078 +128
==========================================
+ Hits 22815 23026 +211
- Misses 8722 8862 +140
+ Partials 86 19 -67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0e1c8b5 to
ba5fbbe
Compare
Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
ba5fbbe to
47bc7ad
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
TL;DR: One real correctness issue stands out: the new co-simulation interface lacks a platform guard despite being Linux-only, and the rest is mostly lower-value naming, logging, and documentation cleanup with no clear equation/stamping problems. The timeouts and socket-path findings are largely robustness-oriented and several review notes are duplicates or speculative, so the overall risk is moderate rather than broad functional breakage.
Found 8 high, 28 medium, 4 low (0 anchored to lines below).
🔵 Optional / low-confidence (40)
- Incorrect parameter naming in publishConfig signature
[high · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:48 - Parameter naming mismatch in publishConfig implementation
[high · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:234 - Missing Scheduler::external tag for side-effecting Interface task
[high · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:43 - Use steady_clock for timeouts to avoid system clock adjustments
[high · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:42 - Avoid repeated std::to_string in hot path
[high · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:135 - Backoff sleep uses variable rem which can wrap to large values
[high · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:189 - Missing SO_REUSEPORT on leader socket to avoid TIME_WAIT conflicts
[high · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:105 - Missing TCP keepalive on leader and follower sockets to detect dead peers
[high · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:118 - Static assertion message does not match wire layout
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:250 - Parameter naming mismatch in waitForConfig implementation
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:328 - Document role semantics for host parameter in InterfaceCosimSync
[medium · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:16 - Clarify srcFreq semantics for DP/SP vs EMT in publishConfig
[medium · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:48 - Use DPsim's shared constants for time units
[medium · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:21 - Validate ConfigNs fields before use
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:234 - Use DOUBLE_EPSILON for sanity checks on time deltas
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:250 - Use DOUBLE_EPSILON for sanity checks on time deltas in waitForConfig
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:391 - Use typed member access for mHost, mPort, mRole
[medium · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:29 - SO_REUSEADDR should be set before bind, not after
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:102 - Static assertion message is misleading
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:250 - Stack buffer for wire config can be constexpr sized
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:361 - Use DEBUG level for socket setup failures
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:96 - Use DEBUG level for bind/listen failures
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:113 - Use DEBUG level for getaddrinfo failures
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:184 - Use DEBUG level for select failures
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:280 - Use DEBUG level for accept failures
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:293 - Use ERROR level for incomplete config reception
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:364 - Use ERROR level for invalid header
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:372 - Use ERROR level for failed ACK send
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:387 - Missing error handling for setsockopt(SO_REUSEADDR)
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:102 - Non-blocking connect may succeed spuriously without writability
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:153 - sendAll ignores EAGAIN/EWOULDBLOCK on non-blocking sockets
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:206 - recvAll may block indefinitely despite SO_RCVTIMEO on leader side
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:221 - Missing validation of timeStepNs and durationNs parameters
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:258 - Missing Linux-specific include guard
[medium · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:7 - Missing documentation for InterfaceCosimSync
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Concepts - Notebook not registered as a test source
[medium · 35% confidence · unconfirmed]inexamples/Notebooks/Features/CosimStartSync.ipynb - ConfigNs struct field naming inconsistency
[low · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:21 - Use consistent non-blocking flag handling
[low · 35% confidence · unconfirmed]indpsim/src/InterfaceCosimSync.cpp:105 - Use DPsim::String consistently
[low · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:29 - Inconsistent naming of host parameter in constructor comment
[low · 35% confidence · unconfirmed]indpsim/include/dpsim/InterfaceCosimSync.h:29
Claim vs. implementation
- Claimed: Add a Linux-only InterfaceCosimSync to synchronize the wall-clock start time of separately launched real-time co-simulation processes over TCP.
- Done: Adds a new TCP-based InterfaceCosimSync class with leader/follower rendezvous, config broadcast/ack, Python bindings, and an example notebook.
- Difference: none
How this review was produced
13 specialized finder passes raised 40 findings over the diff and the full changed sources. After de-duplication, 40 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 0 refuted as unsupported, 40 kept (40 tentative).
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.
What
InterfaceCosimSynccoordinates the start of separately-launched real-time simulation processes so they begin at the same wall-clock instant.Why
Running a co-simulation split across two or more independently-started processes, potentially on different hosts, requires all of them to enter their
RealTimeSimulationloop at the same absolute time. There was no built-in way to agree on that instant across processes.Obs