uORB: add timestamp field to uORB msgs #10178

TSC21 · 2018-08-07T02:30:49Z

Describe problem solved by the proposed pull request
In order to properly generate micro-RTPS agent code and headers compatible with both ROS2 and the micro-RTPS client, the uORB .msg require to have a timestamp field on their definition.

Describe your preferred solution
Basically add the timestamp field to each of the uORB topics and removes the auto-generated one. Also, whenever possible, I added the timesync between the mavlink propagated timestamp and the system time (to be reviewed).

Additional context
This still needs to be reviewed per module and check what are the repercussions, if at all, on each one, including logs and replay.

@thomasgubler @bkueng FYI. First suggestions and reviews are welcomed, though I still have to walk through the source code and check for duplicated/redundant timestamps and also how each timestamp is being used and how it will influence this PR.

mhkabir · 2018-08-07T02:37:36Z

The timestamp field is in every message already. The generator automatically adds it.

TSC21 · 2018-08-07T02:42:11Z

The timestamp field is in every message already. The generator automatically adds it.

I know @mhkabir. But that was removed as well if you check it on the PR. It needs to be on the msg definition, and not auto-generated so what I described above works.

TSC21 · 2018-08-07T02:46:02Z

Unfortunately, px4fmu-v2 is again killing the progress. Have to check what's causing buffer size must be a multiple of element size on the tests.

bkueng · 2018-08-07T05:20:54Z

Looks good so far. Note that you don't have to go through the modules since there is no change for them, execpt for the debug_*.msg topics.
Make sure to also check with listener sensor_baro that the output is still the same.

thomasgubler · 2018-08-07T06:43:41Z

The timestamp field is in every message already. The generator automatically adds it.

I know @mhkabir. But that was removed as well if you check it on the PR. It needs to be on the msg definition, and not auto-generated so what I described above works.

@mhkabir this is needed for ros rtps bridge that nuno is working on. We can not depend on some magic in the generator (which is only in px4 but in none of the ros generator)s

TSC21 · 2018-08-07T15:14:02Z

@bkueng:

pxh> listener sensor_baro

TOPIC: sensor_baro instance 0 #1
 sensor_baro_s
	(0.004982 seconds ago)
	timestamp: 33179575
	error_count: 0
	device_id: 478459
	pressure: 955.936
	temperature: 32.000

So it seems good. Though now something might be wrong with uLog (http://ci.px4.io:8080/blue/organizations/jenkins/PX4_misc%2FFirmware-SITL_tests/detail/PR-10178/3/pipeline#step-129-log-262)

jlecoeur · 2018-08-07T15:55:45Z

In my experience, the timestamp field is a major blocking point for new users (both students and experienced programmers): they do not see that the timestamp field is automatically generated and has to be filled manually.

What about leaving the automatically generated timestamp, but automatically filling it upon call to publish()?

I think this would disambiguate the meaning of the timestamp field. Now it is unclear whether this field refers to the moment the message was published, or to the timestamp of the data contained in the message. If it was automatically field upon at publish time, timestamp would always be the time when a message was published, and if the attached data requires additional timing info, then a field has to be added into the msg file.

dagar · 2018-08-07T15:58:36Z

This is probably the right change short term, but longer term what we really need is a timestamp metadata field that corresponds with the actual orb publish. When a sample timestamp is needed it should be added and set explicitly.

Think about it from the perspective of log review and replay. What you're plotting is quite often not what the rest of the system actually saw at that particular time. This is also why things like ekf2_timestmaps are needed.

EDIT: this was written before @jlecoeur's well timed input.

TSC21 · 2018-08-07T16:02:37Z

This is probably the right change short term, but longer term what we really need is a timestamp metadata field that corresponds with the actual orb publish. When a sample timestamp is needed it should be added and set explicitly.

Think about it from the perspective of log review. What you're plotting is quite often not what the system actually saw at that particular time.

I see your point but currently ROS IDL generators require the timestamps to be explicitly set on the msg definitions, and not autogenerated.

What about leaving the automatically generated timestamp, but automatically filling it upon call to publish()?

@jlecoeur the problem here is not about filling it or not, it's about having the timestamp explicitly defined on the msg definition. Though I agree with you regarding the ambiguity of the the definition of a timestamp, the current thing that this PR solves is not how the timestamp is set, but rather how it is exposed.

dagar · 2018-08-07T17:30:48Z

This is probably the right change short term, but longer term what we really need is a timestamp metadata field that corresponds with the actual orb publish. When a sample timestamp is needed it should be added and set explicitly.

Think about it from the perspective of log review. What you're plotting is quite often not what the system actually saw at that particular time.

I see your point but currently ROS IDL generators require the timestamps to be explicitly set on the msg definitions, and not autogenerated.

This is only because the timestamp (uorb metadata) was manually added to microcdr and urtps, which was a bit of a hack.

A solution that would both be simpler now and better for PX4 long term is to leave the uORB metadata side alone and remove the manual timestamp additions for RTPS.

Then in the small number of cases where you actually need the sample timestamp (IMU) you add that field explicitly in the .msg (timestamp_sample). Later in PX4 we can push the timestamping into orb_publish itself and cleanup the replay system.

TSC21 · 2018-08-07T18:01:15Z

Then in the small number of cases where you actually need the sample timestamp (IMU) you add that field explicitly in the .msg (timestamp_sample). Later in PX4 we can push the timestamping into orb_publish itself and cleanup the replay system.

The thing is, on the ROS side, you don't actually have a sample timestamp: the ROS stamp (which is set on the header, pretty much similar to uORB) represents the time since Unix epoch and it matches the sample time - so in the end, they do represent the same thing, there is no convention for splitting sample time and time since system epoch. So, in that sense, I don't actually think we should have a explicit separation between them.

TSC21 · 2018-08-08T10:14:28Z

If it was automatically field upon at publish time, timestamp would always be the time when a message was published, and if the attached data requires additional timing info, then a field has to be added into the msg file.

@jlecoeur there already exist those fields in several msgs as they are. They thing here is about exposing the timestamp field as something readable by the ROS IDL generator. ROS itself exposes the timestamp field through the Header, and that's the sort of convention compatibility we are looking for here, since, again, there's no actual splitting difference between the msg pub time and the sample time in ROS.

TSC21 · 2018-08-08T10:30:51Z

So it seems good. Though now something might be wrong with uLog (http://ci.px4.io:8080/blue/organizations/jenkins/PX4_misc%2FFirmware-SITL_tests/detail/PR-10178/3/pipeline#step-129-log-262)

This is now solved with the latest commit: http://ci.px4.io:8080/blue/organizations/jenkins/PX4_misc%2FFirmware-SITL_tests/detail/PR-10178/6/pipeline/

thomasgubler · 2018-08-08T12:26:04Z

Blocked by FMUv2 flash overflow

dagar · 2018-08-08T16:07:04Z

I'll try to find some space on FMUv2.

TSC21 · 2018-08-08T16:11:44Z

Thanks @dagar!

…the struct size matches

…mp field exists

TSC21 · 2018-08-08T21:12:34Z

By magic, now px4fmu-v2 doesn't complain. @dagar can we bring this in?

bkueng

One more detail, otherwise this is good to go.

bkueng · 2018-08-09T08:00:55Z

msg/tools/px_generate_uorb_topic_files.py

+        try:
+                assert 'timestamp' in field_name_list
+        except AssertionError:
+                print("[ERROR] uORB topic files generator:\n\tgenerate_output_from_file:\tNo 'timestamp' field found in " + spec.short_name + " msg definition!")


Nice error message 👍
Can you also ensure that the type is uint64_t, our logging infrastructure depends on that?

bkueng · 2018-08-09T09:40:34Z

msg/tools/px_generate_uorb_topic_files.py

@@ -43,6 +43,7 @@
 import filecmp
 import argparse
 import sys
+import numpy as np


Why did you add this?

Oh yeah sorry. This was when I was comparing types. Will remove.

bkueng · 2018-08-09T09:42:49Z

msg/tools/px_generate_uorb_topic_files.py

+        for field in spec.parsed_fields():
+                field_name_and_type.update({field.name:field.type})
+        # assert if the timestamp field exists
+        try:


You can change this to one line like:
if 'timestamp' not in field_name_and_type:

I actually do think it is a better practice to use a try catch approach for this kind of assertions, rather than using if/else.

Generally yes, but not if you catch it with the next line.
Not a big deal to me however.

bkueng

Let's get it in.

bkueng · 2018-08-09T11:40:14Z

msg/tools/px_generate_uorb_topic_files.py

+        for field in spec.parsed_fields():
+                field_name_and_type.update({field.name:field.type})
+        # assert if the timestamp field exists
+        try:


Generally yes, but not if you catch it with the next line.
Not a big deal to me however.

TSC21 force-pushed the pr-add_timestamp_to_msgs branch from ddf2531 to 15133c8 Compare August 7, 2018 15:10

TSC21 added 7 commits August 8, 2018 21:34

add timestamp field to uORB msgs; sync timestamp whenever possible

ea5da74

beautify some identation

d2ce567

uorb templates: remove redundant timestamp field

04bdc12

px_generate_uorb_topic_helper: do not account with the timestamp, so …

96687f3

…the struct size matches

reformat how the passed time is printed

dfedcb6

px_generate_uorb_topic_files: add check and throw error if no timesta…

ab4fd20

…mp field exists

replace printf with PX4_INFO_RAW

7e85a97

TSC21 force-pushed the pr-add_timestamp_to_msgs branch from 9d8355e to 7e85a97 Compare August 8, 2018 20:40

TSC21 added 2 commits August 8, 2018 22:52

sync submodules with upstream

47e82b9

fix radio_status timestamp init

f274b2c

bkueng requested changes Aug 9, 2018

View reviewed changes

bkueng reviewed Aug 9, 2018

View reviewed changes

assert over the type of the 'timestamp' field

cefe103

TSC21 force-pushed the pr-add_timestamp_to_msgs branch from 5ce5bce to cefe103 Compare August 9, 2018 09:51

bkueng approved these changes Aug 9, 2018

View reviewed changes

bkueng merged commit 7c76028 into PX4:master Aug 9, 2018

TSC21 deleted the pr-add_timestamp_to_msgs branch August 9, 2018 14:21

bkueng changed the title ~~[WIP] uORB: add timestamp field to uORB msgs~~ uORB: add timestamp field to uORB msgs Aug 22, 2018

bkueng mentioned this pull request Aug 22, 2018

mavlink: switch back from _mavlink_timesync.sync_stamp to hrt_absolute_time #10298

Merged

dagar mentioned this pull request Mar 11, 2020

[RFC] Metadata uORB message #14366

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uORB: add timestamp field to uORB msgs #10178

uORB: add timestamp field to uORB msgs #10178

TSC21 commented Aug 7, 2018 •

edited by AuterionWrikeBot

Loading

mhkabir commented Aug 7, 2018

TSC21 commented Aug 7, 2018

TSC21 commented Aug 7, 2018 •

edited

Loading

bkueng commented Aug 7, 2018

thomasgubler commented Aug 7, 2018

TSC21 commented Aug 7, 2018

jlecoeur commented Aug 7, 2018

dagar commented Aug 7, 2018 •

edited

Loading

TSC21 commented Aug 7, 2018

dagar commented Aug 7, 2018

TSC21 commented Aug 7, 2018

TSC21 commented Aug 8, 2018

TSC21 commented Aug 8, 2018 •

edited

Loading

thomasgubler commented Aug 8, 2018

dagar commented Aug 8, 2018

TSC21 commented Aug 8, 2018

TSC21 commented Aug 8, 2018

bkueng left a comment

bkueng Aug 9, 2018

TSC21 Aug 9, 2018

TSC21 Aug 9, 2018

bkueng Aug 9, 2018

TSC21 Aug 9, 2018

bkueng Aug 9, 2018

TSC21 Aug 9, 2018

bkueng Aug 9, 2018

bkueng left a comment

bkueng Aug 9, 2018

uORB: add timestamp field to uORB msgs #10178

uORB: add timestamp field to uORB msgs #10178

Conversation

TSC21 commented Aug 7, 2018 • edited by AuterionWrikeBot Loading

mhkabir commented Aug 7, 2018

TSC21 commented Aug 7, 2018

TSC21 commented Aug 7, 2018 • edited Loading

bkueng commented Aug 7, 2018

thomasgubler commented Aug 7, 2018

TSC21 commented Aug 7, 2018

jlecoeur commented Aug 7, 2018

dagar commented Aug 7, 2018 • edited Loading

TSC21 commented Aug 7, 2018

dagar commented Aug 7, 2018

TSC21 commented Aug 7, 2018

TSC21 commented Aug 8, 2018

TSC21 commented Aug 8, 2018 • edited Loading

thomasgubler commented Aug 8, 2018

dagar commented Aug 8, 2018

TSC21 commented Aug 8, 2018

TSC21 commented Aug 8, 2018

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TSC21 commented Aug 7, 2018 •

edited by AuterionWrikeBot

Loading

TSC21 commented Aug 7, 2018 •

edited

Loading

dagar commented Aug 7, 2018 •

edited

Loading

TSC21 commented Aug 8, 2018 •

edited

Loading