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

Added explicitness to `uavcan::time` #50

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@kjetilkjeka
Copy link
Member

kjetilkjeka commented Oct 26, 2018

The following things are, in my opinion, improved by this change.

  • We have explicitness for when a monotonic time reference is needed and when a timestamp is needed.
  • The time frame is explicit allowing diagnostics to pretty print a time down to the microsecond.
  • The time frame is explicit allowing nodes that are dependent on gps/glonass/galileo to be explicit about time reference.

The drawback is that nodes will saturate the timer in 285 years instead of 2k years. But the current changes also allows a system to smoothly migrate to a different time frame starting in 2200 when we're approaching 2265 (GPS time + 285).

#

# A monotonically nondecreasing duration since boot.
Duration.1 since_boot

This comment has been minimized.

Copy link
@thirtytwobits
#
# - CAN 2.0-based transports carry up to 7 bytes per frame. Time sync messages must use single-frame
# transfers, which means that the `SystemTime` type can't be wider than 56 bits.
truncated uint53 microseconds

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Oct 29, 2018

  1. If we used more bits we could accommodate UTC as well.
  2. epoch time is signed. should this be int54 instead?
@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 2, 2018

I like the general idea, but how about altering the implementation a bit. I am generally against unnecessary unions because they restrict our ability to type-check statically and require applications to perform additional computations to handle types at runtime, but this seems like a worthy exception. How about we define some new types:

...each containing a truncated uint53 number of microseconds.

Then we define uavcan.time.Point as a union of the above?

Regarding negative time, I am not entirely sure whether we need that. Seeing as these types are generally intended for time stamping and synchronization, we are unlikely to need negative time unless we time-traveled back to the 20th century. To support negative time we'd have to either limit ourselves to just four time bases forever, reduce the resolution, or to further reduce the already limited range of 285 years even further; can we really afford any of that?

Do we really want a dedicated monotonic time, considering that we already have Boot? I suggest to push back on that until there is a stronger case for that feature.

The new per-time-base types will allow us to require particular time bases statically when required; e.g. for the file info message we would just use uavcan.time.base.UTC.

@kjetilkjeka

This comment has been minimized.

Copy link
Member Author

kjetilkjeka commented Nov 3, 2018

I'm OK with your proposal of types.

Regarding negative time, I am not entirely sure whether we need that. Seeing as these types are generally intended for time stamping and synchronization, we are unlikely to need negative time unless we time-traveled back to the 20th century. To support negative time we'd have to either limit ourselves to just four time bases forever, reduce the resolution, or to further reduce the already limited range of 285 years even further; can we really afford any of that?

Most of these timescales provide dates many years previous to today even without negative numbers. I suspect the ability to be able to add more timebases at a later point will be more valuable than negative numbers. Changing the range to [-140, +140] years? I don't think it's better, but not completely certain.

Do we really want a dedicated monotonic time, considering that we already have Boot? I suggest to push back on that until there is a stronger case for that feature.

I agree. Should we perhaps require boot to be monotonic?

The new per-time-base types will allow us to require particular time bases statically when required; e.g. for the file info message we would just use uavcan.time.base.UTC.

Great!


I will not be able to do much before Wednesday, feel free to hijack the PR if you want it done faster.

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 3, 2018

Most of these timescales provide dates many years previous to today even without negative numbers. I suspect the ability to be able to add more timebases at a later point will be more valuable than negative numbers. Changing the range to [-140, +140] years? I don't think it's better [...]

We're on the same page.

I agree. Should we perhaps require boot to be monotonic?

Yes; in fact, we could just call it uavcan.time.base.MonotoniсSinceBoot.

I will not be able to do much before Wednesday, feel free to hijack the PR if you want it done faster.

No rush, this can wait.

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 8, 2018

I thought about this some more and realized that we should discuss this further.

The old (current) approach is to state that there is some time base of unknown origin that all nodes must sync with not knowing its properties, other than that its rate of progress is close to 1 second per second. The minimal information provided by the old (current) solution is enough to convey and reason about temporal relationships between data items, which is the only problem addressed by sensor feed timestamping.

Actually, for the old (current) approach, time is irrelevant. The objective is to cluster related measurements together. It is convenient to use time for that, but it could be any other source of unique identifiers as long as it is available to all nodes uniformly.

The new approach adds more context and complicates timestamping, requiring nodes to not only sync their clocks, but also to remember what type of clock is currently in use.

We should further think about that and detail whether it is permitted for time sync masters to broadcast sync messages carrying several different time bases simultaneously, and if so, how time sync slaves (and other masters) should react to that. If there are multiple time bases concurrently available in the system, which one is used for timestamping? Should we prioritize them? Should we prohibit such plurality?

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 9, 2018

I have thought about it some more; what follows is just an idea, not sure about its merits.

We could add the explicitness while avoiding the complications I talked about earlier if we were to report the time base in use with the time sync messages and remove that information from timestamps.

The type uavcan.time.Duration would be just a single field uint56 microseconds, nothing else. It would be implied that the time base the timestamp was made in matches that which is used by the time sync master. The fact that the time base is not reported with timestamps might make things simpler by eliminating the unnecessary redundancy. We call this type "duration" because the point of origin is not specified, so the term "point" is not applicable anymore.

The time synchronization message uavcan.time.Synchronization would just be a wrapper for uavcan.time.Point:

# The time base must not be changed while the system is running
uavcan.time.Point previous_transmission_at

Where uavcan.time.Point is just a union of time bases with four slots reserved:

@union                      # The tag is 3 bits wide
uavcan.time.base.TAI tai
uavcan.time.base.UTC utc
uavcan.time.base.MonotonicSinceBoot monotonic_since_boot
uavcan.time.base.ApplicationSpecific application_specific
uint53 _reserved_a
uint53 _reserved_b
uint53 _reserved_c
uint53 _reserved_d
@assert offset == {56}

On the other hand, one might question whether such discrepancy between time syncing and time stamping actually makes things even more unnecessarily involved.

@kjetilkjeka kjetilkjeka force-pushed the kjetilkjeka:time-v1 branch from 7addade to 694f711 Nov 11, 2018

@kjetilkjeka kjetilkjeka force-pushed the kjetilkjeka:time-v1 branch from 694f711 to f163873 Nov 11, 2018

@kjetilkjeka

This comment has been minimized.

Copy link
Member Author

kjetilkjeka commented Nov 11, 2018

We could add the explicitness while avoiding the complications I talked about earlier if we were to report the time base in use with the time sync messages and remove that information from timestamps.

I agree, but would like to push it a bit further.

It seems like there are many different use cases for timestamps and as many arguments for different resolutions (s/ms/us) and bit lengths. It, therefore, seems like defining a SystemTime type correctly that all messages will use would be a big compromise between many factors without giving much in return.

It might be more useful to define some standard time reference that we use throughout the standard definitions and use the resultion/size that fits best with the applications, and even allow small overflowing timers where that make sense.

This would make it idiomatic to use 53 bits in sync timestamps, 56 where needed, and would even make the 32 bits second counter in Heartbeat not stand out.

I've updated the PR to reflect this, what do you think?

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 11, 2018

Indeed. That would pretty much bring us back to where we started. What I like here is that we make things simple yet flexible by going back from uavcan.time.Point to uint56 timestamp_microsecond (or uint53). What I do not like is that it adds unnecessary variance where it could be avoided. Suppose you are building a generic time synchronizer: if each timestamp is of the same type (uavcan.time.Point), the task is trivial; if message definitions are allowed to pick their own time representations (bit widths, overflowing time, different resolutions, whatever), things get complicated fast.

I think we don't gain a lot by unifying timestamping with more specialized usages like the node uptime counter or file modification time since they serve different purposes, but it's still nice to have for consistency.

I would really like to hear more opinions on this as we seem to be a bit stuck here.

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 12, 2018

Let me go back to what I said earlier here. The main purpose of timestamping is the detection of temporally-near data items. For that purpose, the exact point of origin of time does not matter; what matters instead is that the point of origin is the same and the rate of progress of time is similar for all nodes across the network. That suggests that, as discussed, data timestamps should be just plain integers, without any additional time system specifiers. I would still like, though, to wrap the plain integer into a container type, because that facilitates type safety and generic programming (see the previous post), and the type of timestamp can be chosen to suit the vast majority of sensor timestamping needs:

uavcan.time.SynchronizedTimestamp:

truncated uint56 microsecond

It has been proposed here that the synchronization message would carry some information about the time value contained therein. I think we would all agree that changing the time base on a live system should be prohibited, as such a transition would undoubtedly introduce all kinds of nasty transients that can severely disrupt the operation of real-time processes. This also implies that if there is more than one time sync master in the system, they all must adhere to the same time base (TODO spec this). The upshot is that if there is any time base information included in the time sync message, it would stay constant for the entire operating time of the bus, which makes this information redundant to have in every message. I propose that we define the sync message as before, except that we use a plain integer for the avoidance of confusion:

uavcan.time.Synchronization:

truncated uint56 previous_transmission_timestamp_microsecond

And then we also provide a service that can be made mandatory for all time sync masters, which provides information about the reported time, and this information is also required to stay constant as long as the node is running (other fields such as leap seconds are allowed to change, obviously, since they are out of control of the time sync master):

uavcan.time.GetTimeSynchronizationMasterInfo:

uint4 TIME_SYSTEM_UPTIME = 0
uint4 TIME_SYSTEM_TAI    = 1
uint4 TIME_SYSTEM_APPLICATION_SPECIFIC = 15
truncated uint4 time_system

# Difference between TAI and UTC (a.k.a. leap seconds); as Earth is decelerating, can be only positive.
# Do not use outside Earth.
uint12 tai_utc_difference

I am beginning to question the validity of UTC for time synchronization needs given its inherent dependency on the Earth's rotation; see https://derickrethans.nl/leap-seconds-and-what-to-do-with-them.html:

As the Earth's rotation is slowing down at an ever increasing rate, the use of leap seconds will get more and more annoying. After the 25th century, it is likely that more than 4 leap seconds would be required every year. This is of course highly unpractical and a solution needs to be found for this.

All other uses of time, e.g. the file modification time and the node uptime, should probably stay as they are because they need to adhere to completely different requirements (very low resolution, very low precision, synchronization not needed, the uptime is always monotonic).

pavel-kirienko added a commit that referenced this pull request Nov 18, 2018

@pavel-kirienko pavel-kirienko referenced this pull request Nov 18, 2018

Merged

Time sync metadata #53

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Nov 21, 2018

Moving over to #53

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.