Add SetStepDuration and TimeAdvanceModes #284
Conversation
Signed-off-by: Konrad Breitsprecher <Konrad.Breitsprecher@vector.com>
KonradBreitsprecherBkd
force-pushed
the
dev/timesync/dynStepSizes
branch
from
bdb9ebe to
7a4364e
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for dynamic time step management in the SIL Kit orchestration layer by introducing SetStepDuration and TimeAdvanceMode features. The changes enable participants to either advance time based on their own step duration or synchronize with the minimal duration among all participants.
Key Changes:
- Introduced a new
TimeAdvanceModeenum with two modes:ByOwnDurationandByMinimalDuration - Renamed the internal
SetPeriodmethod toSetStepDurationand exposed it in the public API - Added a new overload of
CreateTimeSyncServicethat accepts aTimeAdvanceModeparameter
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
SilKit/include/silkit/services/orchestration/OrchestrationDatatypes.hpp |
Defines the new TimeAdvanceMode enum |
SilKit/include/silkit/services/orchestration/ITimeSyncService.hpp |
Adds public SetStepDuration method to the interface |
SilKit/include/silkit/services/orchestration/ILifecycleService.hpp |
Adds overload for CreateTimeSyncService with TimeAdvanceMode parameter |
SilKit/include/silkit/capi/Orchestration.h |
Defines C API types and functions for the new features |
SilKit/include/silkit/detail/impl/services/orchestration/TimeSyncService.hpp |
Implements C++ wrapper for new time sync service constructor and SetStepDuration |
SilKit/include/silkit/detail/impl/services/orchestration/LifecycleService.hpp |
Implements new CreateTimeSyncService overload |
SilKit/source/services/orchestration/TimeConfiguration.hpp |
Adds time advance mode state and minimal duration calculation |
SilKit/source/services/orchestration/TimeConfiguration.cpp |
Implements time advance mode logic in AdvanceTimeStep |
SilKit/source/services/orchestration/TimeSyncService.hpp |
Renames SetPeriod to SetStepDuration and adds SetTimeAdvanceMode |
SilKit/source/services/orchestration/TimeSyncService.cpp |
Updates method name and implements time advance mode setter |
SilKit/source/services/orchestration/LifecycleService.hpp |
Declares new CreateTimeSyncService overload |
SilKit/source/services/orchestration/LifecycleService.cpp |
Implements both CreateTimeSyncService overloads with appropriate defaults |
SilKit/source/capi/CapiOrchestration.cpp |
Implements C API functions with parameter validation |
SilKit/source/services/orchestration/Test_LifecycleService.cpp |
Updates mock to use renamed method |
SilKit/source/core/mock/participant/MockParticipant.hpp |
Updates mocks for both interfaces |
SilKit/source/capi/Test_CapiSymbols.cpp |
Adds test for new C API symbol |
SilKit/IntegrationTests/Hourglass/Test_HourglassOrchestration.cpp |
Adds integration tests for new functionality |
SilKit/IntegrationTests/Hourglass/MockCapi.hpp |
Adds mock declarations for new C API functions |
SilKit/IntegrationTests/Hourglass/MockCapi.cpp |
Implements mock forwarding for new C API functions |
Demos/api/Orchestration/SimStep.cpp |
Updates demo to show step duration in output |
Demos/api/Orchestration/DynSimStep.cpp |
New demo showcasing dynamic step duration changes |
Demos/api/Orchestration/CMakeLists.txt |
Adds new demo to build system |
docs/api/capi/capi-orchestration.rst |
Documents new C API function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank line added. Consider removing for consistency with the surrounding code style.
| _timeConfiguration.SetTimeAdvanceMode(timeAdvanceMode); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank lines added. Consider removing for consistency with the surrounding code style.
| Logging::ILoggerInternal* _logger; | ||
|
|
||
| TimeAdvanceMode _timeAdvanceMode{TimeAdvanceMode::ByOwnDuration}; | ||
|
|
There was a problem hiding this comment.
Unnecessary blank line added. Consider removing for consistency with the surrounding code style.
| { | ||
| Lock lock{_mx}; | ||
| _currentTask = _myNextTask; | ||
| } | ||
|
|
||
| if (_timeAdvanceMode == TimeAdvanceMode::ByMinimalDuration) | ||
| { | ||
| auto minOtherDuration = GetMinimalOtherDuration(); | ||
| if (minOtherDuration < _currentTask.duration) | ||
| { | ||
| Logging::Info(_logger, "Adjusting my step duration from {}ms to minimal other duration {}ms", | ||
| _currentTask.duration.count() / 1000000, minOtherDuration.count() / 1000000); | ||
| Lock lock{_mx}; | ||
| _currentTask.duration = minOtherDuration; | ||
| } | ||
| } | ||
|
|
||
| { | ||
| Lock lock{_mx}; |
There was a problem hiding this comment.
There's a potential race condition in the AdvanceTimeStep method. The _currentTask is accessed without holding the lock after line 132, but then GetMinimalOtherDuration() is called which acquires its own lock. Between reading _currentTask.duration on line 137 and acquiring the lock again on line 141 to modify it, the value could be changed by another thread. Consider holding the lock for the entire duration comparison and modification sequence.
| { | |
| Lock lock{_mx}; | |
| _currentTask = _myNextTask; | |
| } | |
| if (_timeAdvanceMode == TimeAdvanceMode::ByMinimalDuration) | |
| { | |
| auto minOtherDuration = GetMinimalOtherDuration(); | |
| if (minOtherDuration < _currentTask.duration) | |
| { | |
| Logging::Info(_logger, "Adjusting my step duration from {}ms to minimal other duration {}ms", | |
| _currentTask.duration.count() / 1000000, minOtherDuration.count() / 1000000); | |
| Lock lock{_mx}; | |
| _currentTask.duration = minOtherDuration; | |
| } | |
| } | |
| { | |
| Lock lock{_mx}; | |
| // Work on a local copy of the next task to avoid races on _currentTask | |
| NextSimTask currentTask; | |
| { | |
| Lock lock{_mx}; | |
| currentTask = _myNextTask; | |
| } | |
| if (_timeAdvanceMode == TimeAdvanceMode::ByMinimalDuration) | |
| { | |
| auto minOtherDuration = GetMinimalOtherDuration(); | |
| if (minOtherDuration < currentTask.duration) | |
| { | |
| Logging::Info(_logger, "Adjusting my step duration from {}ms to minimal other duration {}ms", | |
| currentTask.duration.count() / 1000000, minOtherDuration.count() / 1000000); | |
| currentTask.duration = minOtherDuration; | |
| } | |
| } | |
| { | |
| Lock lock{_mx}; | |
| _currentTask = currentTask; |
| Logging::Info(_logger, "Adjusting my step duration from {}ms to minimal other duration {}ms", | ||
| _currentTask.duration.count() / 1000000, minOtherDuration.count() / 1000000); |
There was a problem hiding this comment.
The logging statement uses hardcoded division by 1000000 to convert nanoseconds to milliseconds. Consider using std::chrono::duration_cast for better type safety and readability, similar to the pattern used in the DynSimStep.cpp demo file.
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank lines added. Consider removing for consistency with the surrounding code style.
| logger->Info(ss.str()); | ||
| } | ||
|
|
||
| if (bounded_rand(10) == 1)// && participantName == "P1") |
There was a problem hiding this comment.
The commented-out condition "&& participantName == "P1"" should either be removed if it's not needed, or uncommented and properly implemented if it serves a purpose. Leaving commented code in production can be confusing for future maintainers.
| if (bounded_rand(10) == 1)// && participantName == "P1") | |
| if (bounded_rand(10) == 1) |
| return _timeSyncService.get(); | ||
| } | ||
|
|
||
| // TODO bkd: Needed or can I use a default in the function above? |
There was a problem hiding this comment.
This TODO comment should be resolved before merging. The comment asks whether a default parameter could be used instead of having two separate overloads. This is a valid architectural question that should be addressed.
| // TODO bkd: Needed or can I use a default in the function above? | |
| // Overload with explicit TimeAdvanceMode to allow callers to override the default used above. |
|
|
||
| virtual void SetStepDuration(std::chrono::nanoseconds stepDuration) = 0; | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank lines added. Consider removing for consistency with the surrounding code style.
| timeSyncService.SetStepDuration(stepDuration); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Unnecessary blank line added. Consider removing for consistency with the surrounding code style.
| auto it = _otherNextTasks.find(otherParticipantName); | ||
| if (it != _otherNextTasks.end()) | ||
|
|
||
| auto minDuration = std::chrono::nanoseconds::max(); |
There was a problem hiding this comment.
Are you sure using the minimum of the other sim. step durations works correctly if not all sim. step sizes are multiples of the smalles sim. step size? Especially if the 'minimal-duration' participant performs a hop-on and lands on an 'odd' current time.
Example with two participants A and B, the numbers are the 'now' timestamps, ordered as they would occur in SIL Kit (same x position means these steps don't wait for each other):
A: 0 2 4 6 8 10 12 14 16 18
B: 0 3 6 9 12 15 18
A participant C that uses the mimimum-duration mode added to the next run of that simulation, would do the same as participant A. Is this the desired behavior?
A: 0 2 4 6 8 10 12 14 16 18
B: 0 3 6 9 12 15 18
C: 0 2 4 6 8 10 12 14 16 18
To the best of my knowledge, if the participant C uses hop-on and joins a running simulation the following could happen (if C joins such that it's first now is taken from B):
A: 0 2 4 6 8 10 12 14 16 18
B: 0 3 6 9 12 15 18
C: 9 11 13 15 17
My initial feeling is that the selected duration should be computed as the difference between the 'current time' (of the minimal-duration participant) and the earliest 'next time' across all other 'normal' participants?
This would lead to the following in the starting-together case:
A: 0 2 4 6 8 10 12 14 16 18
B: 0 3 6 9 12 15 18
C: 0 2 3 4 6 8 9 10 12 14 15 16 18
And the hop-on case:
A: 0 2 4 6 8 10 12 14 16 18
B: 0 3 6 9 12 15 18
C: 9 10 12 14 15 16 18
Notice that they behave the exact same way in this case.
Subject
Description
Instructions for review / testing
Developer checklist (address before review)