-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: master
Are you sure you want to change the base?
Conversation
@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? It seems like a good idea if you do not only care about real-time in the simulation but also across the interface. |
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.
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? |
There was a problem hiding this 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.
@@ -10,6 +10,7 @@ | |||
|
|||
#include <dpsim-models/MNASimPowerComp.h> | |||
#include <dpsim-models/Signal/CosineFMGenerator.h> | |||
#include <dpsim-models/Signal/DCGenerator.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the name "DCGenerator" pretty confusing. Isnt it just a constant value? Can we call it "Constant"?
@m-mirz Whats your opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, constant is better. Generator is already used for the equipment type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not naming it Generator would be quite confusing, because all other classes used for generating the signal of a voltage source are called generator. Constant is also a bad name, because the value generated is not constant at all. Also ConstantGenerator is a pretty weird name.
I'm open to another name if you can think of a good one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant is also a bad name, because the value generated is not constant at all.
Can you elaborate? I thought the purpose is to produce a constant (non oscillating) signal?
Maybe we can simply get rid of it and just use a Sine wave generator using a frequency of 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sine wave generator is too slow.
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. |
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. |
cb78369
to
992cbcd
Compare
Quality Gate passedIssues Measures |
I don't think the CI errors are caused by this PR. The tests are missing |
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 <niklas.eiling@eonerc.rwth-aachen.de>
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 <niklas.eiling@eonerc.rwth-aachen.de>
Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
Speeds up simulation by several orders of magnitude Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
…rface Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
…ed and not always when the frequency is 0. This should avoid any regressions. Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
Signed-off-by: Niklas Eiling <niklas.eiling@eonerc.rwth-aachen.de>
@stv0g I think that the critical points are taken care of. Would you agree to merge? |
Quality Gate passedIssues Measures |
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.