On 8/21/2023 2:04 PM, Miroslav Lichvar wrote:
> On Fri, Aug 18, 2023 at 12:31:47PM +0200, Maciek Machnikowski wrote:
>> Current implementation saves s->drift as an offset between current clock
>> frequency and the remote clock frequency calculated on first two samples.
>>
>> Locked state of the PI servo assumes the s->drift holds the history
>> of clock frequency difference scaled by s->ki * weight.
>
> s->drift is the drift of the uncompensated clock. It's not supposed to be
> scaled by anything.
If that's the intended use - then the line 147:
s->drift += ki_term;
where (line 140)
ki_term = s->ki * offset * weight;
is incorrect. s->drift is used as the ki accumulator in the SERVO_LOCKED
state.
>
>> As a result the lock needs more time to stabilize when the frequency
>> offset is very large (ex. set to extreme values with phc_ctl) or may
>> totally prevent servo from correctly locking.
>
> How exactly did you test it? Have you tried it with the P2P delay
> mechanism?
>
I connected my DUT B2B to the GM and ran the "default" ptp4l profile.
In this case it wouldn't matter whether I use E2E or P2P, as this is
just a B2B connection.
To set the offset I used phc_ctl freq 5000000 (which is the max default)
I did that test on couple different NICs and in all cases this the clock
frequency converged faster and with a smaller swing.
How did you test it?
> I suspect you are trying to compensate for the error in E2E delay
> measured before the frequency is corrected. I think a proper fix would
> be to adjust the timestamps from E2E and recalculate the delay before
> stepping the clock.
>
The intended way is to correct the s->drift to hold correct value when
entering SERVO_LOCKED (case 2:)
In an extreme case - if the initial offset is significantly larger than
servo->max_frequency current code will deadlock at line 142 creating
infinite correction of +/- servo->max_frequency, as it is always added
to the ppb calculated.
>> This patch fixes the s->drift when entering the stable state to hold
>> the scaled drift.
>
> That looks wrong to me and in my test with a clock which has a large
> frequency error it performs worse.
>
It's either this, or change the s->drift to continue accumulating the
drift and changing the 142 to
ppb = s->kp * offset * weight + s->drift * s->ki * weight + ki_term;
Both would work, but I assume they should give the same results.
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
