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

Change to VariableDFG.missionnanosec alongside .timestamp #1087

Open
dehann opened this issue Aug 8, 2024 · 4 comments
Open

Change to VariableDFG.missionnanosec alongside .timestamp #1087

dehann opened this issue Aug 8, 2024 · 4 comments

Comments

@dehann
Copy link
Member

dehann commented Aug 8, 2024

What we should not do is say getTimestamp(var) is not actually the timestamp.

Perhaps we should rename .nstime field to: variable.missionnanosec as part of DFG v1. The suggested comments in code here already support both ("mission time" and ROS standard) cases where the full mission time is in the nanosecond field, or is split with the timestamp field.

This way the poweruser would know that timestamp and missionnanosec are equivalent. They can then have different start times also, namely actual calendar datetime+timezone, and 0ns. They can then be made equivalent with
datetime2unix(timestamp[1]) + missionnanosec*1e-9
but this sum is the really large number where Float64 risk of precision occurs. That would be in line with DateTime being millisecond only.

We should obviously be using Float64 for this, so don't mind ROS' use of Float32. Remember also that much of the web still uses 32 for many of its workings.

The suggested missionnanosec approach seems to solve all the problems?

Originally posted by @dehann in JuliaRobotics/RoME.jl#747 (comment)

@dehann dehann added this to the v1.0.0 milestone Aug 8, 2024
@dehann dehann changed the title What we should not do is say getTimestamp(var) is not actually the timestamp. Change to VariableDFG.missionnanosec alongside .timestamp Aug 8, 2024
@Affie
Copy link
Member

Affie commented Aug 10, 2024

What we should not do is say getTimestamp(var) is not actually the timestamp.

getTimestamp gets the .timestamp field, it's a DateTime in julia and string in the API.

We should obviously be using Float64 for this

Float64 does not provide enough resolution, it has to remain Int64 or as 2x Int32 as used in ROS. Splitting (for Int32 web support) is perhaps a good option for serialization (PackedVariable)

I'm open to any name you want to use as long as the flexibility of using any time format remains. We can also default to a unix epoch of timestamp if not provided.

@Affie
Copy link
Member

Affie commented Aug 15, 2024

They can then be made equivalent with
datetime2unix(timestamp[1]) + missionnanosec*1e-9

julia> timestamp = now();
julia> datetime2unix(timestamp) + 100*1e-9 - datetime2unix(timestamp)
0.0

@dehann
Copy link
Member Author

dehann commented Aug 21, 2024

I think we're saying the same thing in different ways. Your example of 100ns info lost is what we need to avoid. That because calendar time + nstime is too much.

Must be Int64 and DateTime. User can convert to a Float64 or whatever they want to do.

So can we go with these two names then please,

  • 'timestamp::DateTime' and
  • 'missionnanosec::Int64'.

If that data gets striped by an accidental json 32 somewhere the result is equivalent to:
'timestamp = DateTime(str)' and 'missionnanosec = Int64(Int32( nstime ))'

The important point here is that we never required all timestamp info to pass through an Int or Float -- that keeps data in tact. The tightest margin is mission time (startring from 0) should fit in a Int32 -- but again that is the legacy support case. Hopefully we always have at least Int64.

I dont want to be splitting the nanosec value across two ints, that would be very unusual.

Definitely do not want to be working in Unix epoch time.

@Affie
Copy link
Member

Affie commented Oct 16, 2024

I'm not sure I'm following. What is changed to VariableDFG.missionnanosec?
Do you want to change the .nstime field to .missionnanosec?
Or do you want to add a 3rd timestamp?

I dont want to be splitting the nanosec value across two ints, that would be very unusual.

ROS splits the timestamp into two Int32s one for nanosec and one for seconds.

Definitely do not want to be working in Unix epoch time.

Why not? It's quite common to use unix epoch time. I normally shift to unix epoch with something like nstime = Dates.value(Nanosecond(timestamp - unix2datetime(0)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants