-
Notifications
You must be signed in to change notification settings - Fork 296
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
gPTP: remove accelerated SYNC send on startup. See discussion in #465. #486
Conversation
I'm hoping @christopher-s-hall can take a quick look at this. I not 100% sure I have implemented his suggestion as discussed in issue #465. Let me know and I'll make the required/suggested changes. Anyone else have any comments? |
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.
It looks good to me, although it might introduce a slight change to the timing of the Sync interval. Since the automotive code path already had this change, and I didn't notice any misbehavior with it in my testing, I'm guessing it's not significant.
else { | ||
// Automotive Profile | ||
syncDone(); | ||
syncDone(); | ||
|
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 previous code subtracted the old wait_time
from the new wait_time
; as far as I can tell this sets the wait interval from the beginning of the Sync interval rather than from the point we've finished waiting for the hardware to give us a TX Timestamp for our first Sync.
I'm not sure whether you left this out intentionally to simplify things or if you missed it. I'm not sure if it makes much difference correctness-wise.
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 see the wait time adjustment math. I concluded it was most relevant to the automotive fast sync case where TX timestamp read time might have a large effect percentage wise on the Sync interval timing. I didn't think removing the adjustment would have any negative operational effect - although I must admit I haven't hooked up test equipment to try to figure out if there is an such effect.
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.
It looks fine. I'm not too worried about the interval adjustment at this point.
Any comment @christopher-s-hall, or anyone else? We need another reviewer on this. |
Thanks, @christopher-s-hall |
Completely remove accelerated sync on gPTP daemon startup. I think I have covered everything - comments welcomed.