Skip to content
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: Add monotonic raw clock option #795

Merged
merged 2 commits into from
Jun 19, 2018

Conversation

christopher-s-hall
Copy link
Contributor

Timestamp clocks that implement the PTP_SYS_OFFSET_PRECISE ioctl return
cross-timestamps with respect to monotonic raw and realtime
Add configuration file option allowing clock selection for system/local
clock relation

The default clock type is Realtime which is equivalent to the
CLOCK_REALTIME option to clock_gettime
Optionally, MonotonicRaw (equivalent to CLOCK_MONOTONIC_RAW) can be
specified in the gPTP daemon configuration file:

[clock]

SystemClock = MonotonicRaw

Fixed Makefile issue where some shell configurations use built-in 'echo'
and others don't
Forces all to use /bin/echo

@@ -6,6 +6,11 @@
# The lower the number, the higher the priority for the BMCA.
priority1 = 248

# Option to specify system clock type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some more details here. Specifically, we should list the currently supported options, the default option and what the options mean. The text that is part of the pull request has great information in it, but I think that 6 months from now readers will prefer to read an updated comment here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found comments in the example ini files very helpful when trying to actually use the avtp_pipeline code, so it'd be a nice help for users.

@andrew-elder
Copy link

Looks good to me. One small comment update requested. Can anyone else review this?

Timestamp clocks that implement the PTP_SYS_OFFSET_PRECISE ioctl return
 cross-timestamps with respect to monotonic raw and realtime
Add configuration file option allowing clock selection for system/local
 clock relation

The default clock type is Realtime which is equivalent to the
 CLOCK_REALTIME option to clock_gettime
Optionally, MonotonicRaw (equivalent to CLOCK_MONOTONIC_RAW) can be
 specified in the gPTP daemon configuration file:

[clock]

SystemClock = MonotonicRaw

Fixed Makefile issue where some shell configurations use built-in 'echo'
 and others don't
Forces all to use /bin/echo
Copy link
Contributor

@pinealservo pinealservo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great; just a minor formatting comment.

@@ -68,6 +69,9 @@ class GptpIniParser
/*ptp data set*/
unsigned char priority1;

/* Clock data set */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there might be a tabs vs. spaces conflict in this file; I'm not sure whether it'd be better to just make the change match or if the file itself has a mismatched convention.

@@ -6,6 +6,11 @@
# The lower the number, the higher the priority for the BMCA.
priority1 = 248

# Option to specify system clock type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found comments in the example ini files very helpful when trying to actually use the avtp_pipeline code, so it'd be a nice help for users.

@christopher-s-hall christopher-s-hall force-pushed the open-avb-next branch 3 times, most recently from f71369e to b663796 Compare June 12, 2018 21:56
@christopher-s-hall
Copy link
Contributor Author

I added more comments to the config file and I added a second commit to fix the mixed whitespace in gptp_cfg.hpp at least for that struct. My spacing was/is correct (tabs only). What was there wasn't.

@andrew-elder
Copy link

@pinealservo - looks good to me. If you agree, please go ahead and merge. Thanks for the updates @christopher-s-hall

@pinealservo pinealservo merged commit 5b95644 into Avnu:open-avb-next Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants