Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New queueless VILLAS interface / improve real-time performance #316

Merged
merged 11 commits into from
Nov 8, 2024

Conversation

n-eiling
Copy link
Contributor

@n-eiling n-eiling commented Jul 29, 2024

villas interface: improve real-time capability

The original villas interface was not real-time capable because of the queue it uses. This commit creates an interface for villas interfaces, moves the old implementation to InterfaceQueued and adds a new queueless, threadless implementation InterfaceQueueless.

models: Add DC voltage source

The DP_Ph1_VoltageSource always calculates a sinusoidal signal, even if we only want to set a DC value. This commit adds a DC generator for the model so it can be significantly faster if we do not need the sinusoidal signal.

FpgaExample: use new villas interface and make logging optional.

@m-mirz
Copy link
Contributor

m-mirz commented Jul 29, 2024

@n-eiling this reduces the latency by removing the intermediate queue, hoping that the sync call to villas returns very quickly and does not block dpsim for too long?
Do I remember correctly that villas also has internal queues for some node types to avoid blocking?

It seems like a good idea if you do not only care about real-time in the simulation but also across the interface.

@stv0g
Copy link
Contributor

stv0g commented Jul 30, 2024

Do I remember correctly that villas also has internal queues for some node types to avoid blocking?

Yes there are some, but not for the reason you are thinking. Some nodes have internal queues as the underlying libraries have callback based APIs or work in a background thread which require a queue to exchange samples with the main VILLASnode thread.

@n-eiling this reduces the latency by removing the intermediate queue, hoping that the sync call to villas returns very quickly and does not block dpsim for too long?

I agree here. I think both use cases are valid. But most people probably will use DPsim with the non hard real-time VILLASndoe node-types which use syscalls / network I/O anyway. In that cases, I would still recommend to use the queue-based interface

@n-eiling Does your new interface also support non-blocking operation?

Copy link
Contributor

@stv0g stv0g left a comment

Choose a reason for hiding this comment

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

I did a first pass of review. But I think also @m-mirz or @dinkelbachjan should have a closer look.

dpsim-models/include/dpsim-models/Attribute.h Outdated Show resolved Hide resolved
dpsim-villas/examples/cxx/FpgaExample.cpp Show resolved Hide resolved
dpsim-villas/examples/cxx/FpgaExample.cpp Outdated Show resolved Hide resolved
dpsim-villas/examples/cxx/FpgaExample.cpp Show resolved Hide resolved
dpsim-villas/src/InterfaceVillasQueueless.cpp Outdated Show resolved Hide resolved
dpsim-villas/src/InterfaceVillasQueueless.cpp Outdated Show resolved Hide resolved
dpsim-villas/src/InterfaceVillasQueueless.cpp Outdated Show resolved Hide resolved
dpsim-villas/src/InterfaceWorkerVillas.cpp Outdated Show resolved Hide resolved
dpsim-villas/src/InterfaceVillasQueueless.cpp Outdated Show resolved Hide resolved
@n-eiling
Copy link
Contributor Author

@n-eiling this reduces the latency by removing the intermediate queue, hoping that the sync call to villas returns very quickly and does not block dpsim for too long? Do I remember correctly that villas also has internal queues for some node types to avoid blocking?

It seems like a good idea if you do not only care about real-time in the simulation but also across the interface.

The FpgaExample uses no queue, no VILLAS path and consequently is purely single-threaded. To achieve time steps in the microsecond range, even simple synchronization primitives create too much delay - even polling would create jitter.
In the example the simulation is hardware synchronized - so the time step is created on the FPGA, making it more accurate. This is achieved by disabling real-time in DPsim and having the VILLASnode read call block until the next time step starts. The FPGA also creates a sequence number that makes it possible for the VILLAS interface to detect time step overruns.
With some quite heavy OS optimizations (e.g., PREEMPT_RT, isolcpu, ...) and custom compiler flags for VILLAS and DPsim (-flto -Ofast), I was able to achieve time steps around 10 us.

stv0g
stv0g previously requested changes Jul 30, 2024
dpsim/include/dpsim/InterfaceQueued.h Outdated Show resolved Hide resolved
dpsim/include/dpsim/InterfaceQueued.h Outdated Show resolved Hide resolved
dpsim/src/InterfaceQueued.cpp Outdated Show resolved Hide resolved
dpsim/src/InterfaceQueued.cpp Outdated Show resolved Hide resolved
dpsim/src/InterfaceQueued.cpp Outdated Show resolved Hide resolved
dpsim/src/InterfaceQueued.cpp Outdated Show resolved Hide resolved
dpsim/src/InterfaceQueued.cpp Outdated Show resolved Hide resolved
dpsim/src/InterfaceQueued.cpp Outdated Show resolved Hide resolved
dpsim-villas/src/InterfaceVillasQueueless.cpp Outdated Show resolved Hide resolved
@n-eiling
Copy link
Contributor Author

n-eiling commented Aug 1, 2024

I changed the clang format file so it enforces @stv0g comment style. If this does not work for you I would prefer that we fix the clang format file in a separate PR, because this gets really messy here.

@n-eiling n-eiling force-pushed the villas-fpga branch 7 times, most recently from cb78369 to 992cbcd Compare August 26, 2024 12:27
Copy link

sonarcloud bot commented Aug 26, 2024

@n-eiling
Copy link
Contributor Author

n-eiling commented Aug 26, 2024

I don't think the CI errors are caused by this PR. The tests are missing docker-compose. The tests also fail on master

The original villas interface was not real-time capable because of the queue it uses. This commit creates an interface for villas interfaces, moves the old implementation to InterfaceQueued and adds a new queueless, threadless implementation InterfaceQueueless.

Signed-off-by: Niklas Eiling <[email protected]>
The DP_Ph1_VoltageSource always calculates a sinusoidal signal, even if we only want to set a DC value. This commit adds a DC generator for the model so it can be significantly faster if we do not need the sinusoidal signal.

Signed-off-by: Niklas Eiling <[email protected]>
Speeds up simulation by several orders of magnitude

Signed-off-by: Niklas Eiling <[email protected]>
@m-mirz
Copy link
Contributor

m-mirz commented Oct 10, 2024

@stv0g I think that the critical points are taken care of. Would you agree to merge?

Copy link

sonarcloud bot commented Oct 10, 2024

@n-eiling
Copy link
Contributor Author

@stv0g ping :)

@n-eiling
Copy link
Contributor Author

@stv0g ping!

@n-eiling
Copy link
Contributor Author

n-eiling commented Nov 4, 2024

@stv0g pong!

@n-eiling
Copy link
Contributor Author

n-eiling commented Nov 6, 2024

@m-mirz can you just merge it without steffens approval? I addressed the most important points of his review and the remaining points are not really critical.

I have quite a bit of changes depending on this that have been waiting for this PR for months now.

@m-mirz m-mirz dismissed stv0g’s stale review November 8, 2024 21:07

I think the important issues are addressed and we should move forward with this.

@m-mirz m-mirz merged commit ad81d7e into master Nov 8, 2024
18 checks passed
@m-mirz m-mirz deleted the villas-fpga branch November 8, 2024 21:08
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.

3 participants