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

Time sync metadata #53

Merged
merged 11 commits into from Nov 27, 2018

Conversation

Projects
None yet
2 participants
@pavel-kirienko
Copy link
Member

commented Nov 18, 2018

Implemented the changes as proposed here: #50 (comment)

This PR supersedes #50; I have kept Kjetil's commits as well to ensure that the authoring information is reflected in the log.

# If this message is published for the first time, this field must be zero.
Point.1 previous_transmission_timestamp
truncated uint56 previous_transmission_timestamp_microsecond

This comment has been minimized.

Copy link
@kjetilkjeka

kjetilkjeka Nov 21, 2018

Member

What is the point of SynchronizedTimestamp if you avoid using it here?

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Nov 21, 2018

Author Member

Helps metaprogramming: #50 (comment)

This comment has been minimized.

Copy link
@kjetilkjeka

kjetilkjeka Nov 21, 2018

Member

Right, I re-read https://forum.uavcan.org/t/on-timestamping/237 again. I don't think we want to use 12 byte messages as default. And I don't think this message is suitable for coordinating/synchronizing data. I would opt for something between 26 (~1min) and 32 bits for this. I'm also a bit unsure about several other things regarding the SI namespace design and trying to work through them. Maybe it makes more sense to me after thinking a bit more on it.

Perhaps it's better to add this message together with the SI namespace design PR if this is its primary use case (and it's not being used by this message anyway).

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Nov 21, 2018

Author Member

SynchronizedTimestamp is also supposed to be used with the diagnostic record message, unless we don't want any timestamping there (questionable). So it's not only about SI. I prefer to keep everything time-related that is sufficiently generic under this namespace to simplify reuse in vendor-specific type definitions.

If we chose to use overflowing timestamps, then 32-bit is our only option because 24-bit overflows too frequently, and 26-bit is not byte-aligned (and it overflows too frequently, too). If we ended up wanting overflowing timestamps, we shall add another type here, SynchronizedOverflowingTimestamp or something similar; we must keep non-overflowing timestamp as well for completeness and for encouraging safer designs ("safer" here means designs that don't trade-off bandwidth for time uncertainty; in some applications this can be preferred).

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Nov 21, 2018

Author Member

Also, to answer your first question in this thread: the main reason I don't use SynchronizedTimestamp in this message is that this field is not actually a synchronized timestamp in its usual sense. It is the timestamp of the moment when the previous message was transmitted, so I used a different type to make this more explicit.

@@ -0,0 +1,20 @@
#

This comment has been minimized.

Copy link
@kjetilkjeka

kjetilkjeka Nov 21, 2018

Member

Is TimeSystem a good word? TimeReference seems better to me. Our native English speaker @thirtytwobits should probably comment on this.

This comment has been minimized.

Copy link
@pavel-kirienko

This comment has been minimized.

Copy link
@kjetilkjeka

kjetilkjeka Nov 21, 2018

Member

It uses "Atomic Time System" which I think is a bit clearer. "Time Standard" is also used by wikipedia and seems better as well.

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Nov 21, 2018

Author Member

"Time standard" does not apply to custom time like the time since boot or application-specific time, because by their very nature they are non-standard. "Atomic time system" means a time system that is maintained by atomic clocks. In this sense, what we have now ("time system") agrees with Wikipedia. Clearly, we can't say "Atomic time system" for the same reason why we can't say "time standard", unless we require every time sync master node that uses monotonic uptime as its time base to have atomic clocks onboard.

@@ -1,16 +1,14 @@
#
# Nested data type used for representing a point in time with microsecond resolution.
# Nested data type used for representing a network-wide synchronized timestamp with microsecond resolution.
# This data type is highly recommended for use both in standard and vendor-specific messages alike.
#

# Zero means that the time is not known.
uint56 UNKNOWN = 0

This comment has been minimized.

Copy link
@kjetilkjeka

kjetilkjeka Nov 21, 2018

Member

You know what I think of these "Special null values", but I think it might be acceptable here to get the same amount of bits before time sync is achieved.

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

@kjetilkjeka I just added the overflowing timestamp type as discussed. I decided to call it SynchronizedAmbiguousTimestamp because this name seems to fit the purpose. I also wanted to keep it long and annoying on purpose.

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

Since no input was provided since the last change 6 days ago, I am going to merge this tomorrow.

@pavel-kirienko pavel-kirienko merged commit b245363 into uavcan-v1.0 Nov 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pavel-kirienko pavel-kirienko deleted the kjetil-time-v1 branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.