Fix propagation of start time in IntersectivePWM

[christiankral]

Refs #4089

[christiankral]

I think it also makes sense to update the documentation to more clearly explain what the difference between the different phase shift strategies is.

[AHaumer]

fine with me, thanks!

[AHaumer]

Don't know why continuous-integration/ jenkins... fails?

[henrikt-ma]

Don't know why continuous-integration/ jenkins... fails?

As far as I can tell we have a green light, at least as of now.

[TManikantan]

@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?

[henrikt-ma]

I'm sorry, I'm not the right person to recommend reviewers in this project.

[christiankral]

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

1. 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) ...
2. 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:

1. 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.
2. 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 ...

[TManikantan]

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

1. 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) ...
2. 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:

1. 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.
2. 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]

We are missing a review, and the checks seem to be stuck

[casella]

@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.

[AHaumer]

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 ...

[christiankral]

@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.

[AHaumer]

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.

[AHaumer]

LGTM

[christiankral]

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

1. 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) ...
2. 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.

[christiankral]

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.

[AHaumer]

fine with me, sorry for the delay

[AHaumer]

Sorry @christiankral for the long delay.
I'm just now finding the time to understand this PR.
I would sketch the timing (for sawtooth) as follows:

So more or less we agree.
Yes you're right the propagation of startTime is wrong.
For sawtooth it should be:

startTime={startTime - 1.5/f + 
  (if refType == ReferenceType.Sawtooth1 then 0 else k)/m/f for k in 0:m - 1}

Testing with f=1/3 and startTime=0.5 gives:

For triangle it should be:

startTime={startTime - 1.25/f + 
  (if refType == ReferenceType.Triangle1 then 0 else k)/m/f for k in 0:m - 1}

Testing with f=1/3 and startTime=0.5 gives:

So all m=3 signals start at the desired startTime, for time < startTime they are "out of reach" (no fire signal).

Please accept my apologies for the delay - we might have a phone call to clarifiy how we shall proceed.
IMHO I should correct both formulas, and the PR should get backported.
As far as I see the only change in this PR is instantiating the trapezoid[m] insteat of trapezoid[3].
This is no big deal since m=3 is a constant. I could incorporate this into my PR.
Anything to clarify in the documentation?

We should also discuss the parameter uMax because the sawtooth and the triangle are shifted uMax/2 below zero.

I have enclosed a small test with which I derived the above results.
PC40894090.zip

[AHaumer]

Replaced by #4812