ModelicaStandardLibrary
ModelicaStandardLibrary copied to clipboard
modelica
Fix propagation of start time in IntersectivePWM
I think it also makes sense to update the documentation to more clearly explain what the difference between the different phase shift strategies is.
Don't know why continuous-integration/ jenkins... fails?
As far as I can tell we have a green light, at least as of now.
@henrikt-ma @AHaumer i think it requires approval from second reviewer for merging.I am not able to add @christiankral for some reason as a reviewer,he is the only other library officer other than @AHaumer who had already given his approval. Can you suggest someone who can review and approve this PR?
I'm sorry, I'm not the right person to recommend reviewers in this project.
The main purpose of this PR is not the documentation (which I provided so far), but the fix of the propagation of the start time as described in #4089. The actual implementation
- does not propagate correct units
final startTime={startTime - 1.25 + (if refType == ReferenceType.Triangle1 then 0 else k)/m for k in 0:m - 1}/f) ... - and does not (at all) calculate the correct start times.
The following graph shows how I think that the original implementation was intended.
Two issues have to be resolved before moving forward with this PR:
- Based on the shown graph: @AHaumer Is this the way how start time shall be implemented? On my opinion the first triangle signal shall start 1/f later than shown, don't you agree? Otherwise the actual start time suggests an instance in time earlier than possibly intended.
- The meaning of the start time shall also be clarified: At the start time all three bridge branches shall already be supplied with firing signals, but no longer than necessary (this is why I think the first signal shall start 1/f later).
After clarifying these issues, we can go ahead with providing a code fix for this BUG (units are wrong and times are not determined correctly! So this fix then shall rather go into maintenance ...
The main purpose of this PR is not the documentation (which I provided so far), but the fix of the propagation of the start time as described in #4089. The actual implementation
- does not propagate correct units
final startTime={startTime - 1.25 + (if refType == ReferenceType.Triangle1 then 0 else k)/m for k in 0:m - 1}/f) ...- and does not (at all) calculate the correct start times.
The following graph shows how I think that the original implementation was intended.
Two issues have to be resolved before moving forward with this PR:
- Based on the shown graph: @AHaumer Is this the way how start time shall be implemented? On my opinion the first triangle signal shall start 1/f later than shown, don't you agree? Otherwise the actual start time suggests an instance in time earlier than possibly intended.
- The meaning of the start time shall also be clarified: At the start time all three bridge branches shall already be supplied with firing signals, but no longer than necessary (this is why I think the first signal shall start 1/f later).
After clarifying these issues, we can go ahead with providing a code fix for this BUG (units are wrong and times are not determined correctly! So this fix then shall rather go into maintenance ...
@AHaumer would you please comment on the suggestions made by @christiankral
@AHaumer the missing reviewer should obviously be @christiankral, which has expressed some doubts about this PR, e.g. here. Please provide him some feedback at your earliest convenience.
@GallLeo please change the admin rights so I can give @christiankral write access to the repo and make him a reviewer.
As far as I see the PR was created by @christiankral and he can't review his own PR. So @casella please add your review and it's done. We have a lack of reviewers ...
@AHaumer Up to now the bug reported in #4089 is not yet fixed through this PR. My request was: Could you please fix this issue the way I proposed it verbally in #4089, considering, that you agree with my proposal.
This is not real a blocker, and it seems that it is not used a lot. It's better to spend some time to get a clean solution instead of having a quick and dirty solutionh. Therefore: Shift the milestone.
The main purpose of this PR is not the documentation (which I provided so far), but the fix of the propagation of the start time as described in #4089. The actual implementation
- does not propagate correct units
final startTime={startTime - 1.25 + (if refType == ReferenceType.Triangle1 then 0 else k)/m for k in 0:m - 1}/f) ...- and does not (at all) calculate the correct start times.
Kindly asking @AHaumer to
- clarify if the described behavior is the intended behavior
- and provide a fix to the problem of the wrong units
In the actual implementation the wrong units, however, lead to a "wrong" behavior as it has no physical background.
The following graph shows how I think that the original implementation was intended.
To be clear: The work on this PR has not yet started. We need a fix to the wrong propagation of start times to merge this PR.