Add QoS profiles field to metadata struct and provide serialization utilities #330

emersonknapp · 2020-03-26T01:25:03Z

Part of #125

But, don't use this feature anywhere yet. Next step will be to query and store the QoS profiles for recorded topics.

rosbag2_transport constructs the TopicMetadata, so that's where the serialization/deserialization will need to occur (in recorder.cpp / player.cpp)

thomas-moulard · 2020-03-26T01:44:02Z

rosbag2_storage/include/rosbag2_storage/bag_metadata.hpp

@@ -33,7 +33,7 @@ struct TopicInformation

 struct BagMetadata
 {
- int version = 3; // upgrade this number when changing the content of the struct
+ int version = 4; // upgrade this number when changing the content of the struct


is it the right time to update the bag metadata? Could we delay that until the feature is built completely? What's the impact if we were to release rosbag2 just after this PR gets merged?

Yeah I was considering this, but I think that now is the time because now is when the data model actually changes. Moving forward that model will just start to hold more useful values. If we were to release this now, nothing bad would happen, it's fully backward compatible with previous metadata versions.

rosbag2_storage/include/rosbag2_storage/topic_metadata.hpp

thomas-moulard · 2020-03-26T01:46:57Z

rosbag2_transport/src/rosbag2_transport/qos.cpp

+ qos
+ .keep_last(node["depth"].as<int>())
+ .history(history)
+ .reliability(reliability)
+ .durability(durability)
+ .deadline(node["deadline"].as<rmw_time_t>())
+ .lifespan(node["lifespan"].as<rmw_time_t>())
+ .liveliness(liveliness)
+ .liveliness_lease_duration(node["liveliness_lease_duration"].as<rmw_time_t>())
+ .avoid_ros_namespace_conventions(node["avoid_ros_namespace_conventions"].as<bool>());


I know it's a hard problem to solve, but any idea about how we could try to keep this in sync as qos gets new fields?

I don't have a good answer to this. My thought as of right now is just that rosbag2 needs to have a pass to support new fields if they are added, and until that time it will only support the default values for any given policy.

I'm not aware of a way to introspect fields of a struct in C++, but that doesn't mean there isn't a way

Ah - I thought of something we can do. It is a "change detector" like we try to avoid, but in this case of serialization method, it seems like the right thing to do. I'm adding a test that explicitly builds a rmw_qos_profile_t using {} initializer, so if new fields are added, the build will fail.

Thinking about it, deserialize(serialize(x)) == x should do the trick?

If you're sure the qos struct will always be a POD, memcmp is an option otherwise (tread carefully as it can blow up easily)

I don't think it will. If new fields are added - when I construct the object it will use the default values for those new fields. Now when I serialize, it will write out all the fields we specified here. Then, on deserializing, it will read the fields that it knows, with the unknown fields having default values. The equality will pass and new fields will have slipped through undetected

we could maybe apply something like a builder pattern which only allows to build the object if all parts are present. But I don't see a real way around updating the code base if the QoS struct gets more fields.

rosbag2_transport/src/rosbag2_transport/qos.hpp

rosbag2_transport/src/rosbag2_transport/recorder.cpp

emersonknapp · 2020-03-26T19:13:53Z

@Karsten1987 how do you feel about the structure of this? These utilities are gating the rest of the functionality, which is started but I can't finish until this is finalized.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

piraka9011

Does it make sense to add the QoS profile stubs in the tests now or will the interface change a bit? (I don't think it will?)

rosbag2_transport/test/rosbag2_transport/test_qos.cpp

rosbag2_storage/src/rosbag2_storage/metadata_io.cpp

rosbag2_transport/test/rosbag2_transport/test_qos.cpp

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Karsten1987

looks good to me.

For future PRs, I would make my life a bit easier if you could quickly summarize what part of #125 the PR is addressing. That makes it a more straightforward to get to the meat of the PR.

Karsten1987 · 2020-03-26T23:18:06Z

rosbag2_transport/src/rosbag2_transport/qos.hpp

+{
+public:
+ Rosbag2QoS()
+ : rclcpp::QoS(0) {} // 0 history depth is always overwritten on deserializing


I guess we could also default to rclcpp::SystemsDefaultQos() or something. But I think this doesn't really matter as far as I can understand your comment.

Note that rmw_qos_profile_system_default is different than rmw_qos_profile_default - system_default is dependent on the rmw implementation, and because of that I don't think it's something anybody should ever use.

But, I will instantiate this with the default profile (not system_default)

Karsten1987 · 2020-03-26T23:22:48Z

rosbag2_transport/src/rosbag2_transport/qos.hpp

+ static bool decode(const Node & node, rosbag2_transport::Rosbag2QoS & qos);
+};
+} // namespace YAML
+


Suggested change

Karsten1987 · 2020-03-26T23:26:46Z

rosbag2_transport/src/rosbag2_transport/qos.hpp

+# pragma warning(pop)
+#endif
+
+


Suggested change

Karsten1987 · 2020-03-26T23:40:14Z

rosbag2_transport/src/rosbag2_transport/qos.cpp

+ qos
+ .keep_last(node["depth"].as<int>())
+ .history(history)
+ .reliability(reliability)
+ .durability(durability)
+ .deadline(node["deadline"].as<rmw_time_t>())
+ .lifespan(node["lifespan"].as<rmw_time_t>())
+ .liveliness(liveliness)
+ .liveliness_lease_duration(node["liveliness_lease_duration"].as<rmw_time_t>())
+ .avoid_ros_namespace_conventions(node["avoid_ros_namespace_conventions"].as<bool>());


we could maybe apply something like a builder pattern which only allows to build the object if all parts are present. But I don't see a real way around updating the code base if the QoS struct gets more fields.

Karsten1987 · 2020-03-26T23:40:27Z

rosbag2_transport/src/rosbag2_transport/qos.hpp

+ : rclcpp::QoS(value) {}
+};
+} // namespace rosbag2_transport
+


Suggested change

emersonknapp · 2020-03-26T23:52:57Z

Thanks Karsten for the feedback - sorry we merged before you got to it, I am feeling time pressure for the API freeze and would like to start getting to the features. I'll put up a quick PR addressing these comments

And I will make sure to put a more descriptive explanation on the upcoming PRs of what part of #125 they are achieving.

Karsten1987 · 2020-03-26T23:56:01Z

no worries. I guess I was just too slow with my review ;-)

No need for an extra PR for the new lines. You might as well just put them in your next one.

dirk-thomas · 2020-03-27T03:50:13Z

@emersonknapp Can you please point to the CI builds which have been run for this change before it was merged? This change seems to break the build on Windows: https://ci.ros2.org/job/ci_windows/9656/console#console-section-29

…zation utilities (#330)" This reverts commit 5810588. Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp · 2020-03-27T04:36:43Z

Argh, sorry, false sense of security from the automatic actions, I've reverted the change here #334 and will reopen this PR with a CI run on it and the windows fix

zmichaels11 · 2020-03-27T04:40:44Z

rosbag2_transport/src/rosbag2_transport/qos.cpp

+ time.sec = node["sec"].as<uint>();
+ time.nsec = node["nsec"].as<uint>();


Suggested change

time.sec = node["sec"].as<uint>();

time.nsec = node["nsec"].as<uint>();

time.sec = node["sec"].as<uint64_t>();

time.nsec = node["nsec"].as<uint64_t>();

I think this would fix it,

…zation utilities (#330)" (#334) This reverts commit 5810588. Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* serialize and deserialize QoS in metadata Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp requested review from Karsten1987, piraka9011 and dabonnie March 26, 2020 01:28

thomas-moulard approved these changes Mar 26, 2020

View reviewed changes

emersonknapp force-pushed the emersonknapp/metadata-with-qos branch from fd8b7fe to 55ac449 Compare March 26, 2020 18:51

emersonknapp added 14 commits March 26, 2020 12:50

WIP serialize and deserialize QoS in metadata

e6c64ef

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Get the qos profiles

6ee2b30

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Fixing up tests

9d86df5

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Build passing again

1dfc541

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Minimize changes

90b93db

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Bump metadata version

98f2ddc

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Fix build

84ac660

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Move yaml conversions into cpp

99e8ed3

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Fix linters

aa9d0a4

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Remove query functionality and add some tests

7ba5585

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Not reading metadata yet

1fe86ce

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Address review comments

b715038

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Fix linters for test fileO

5b5caf6

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Add field detector test

aa55d43

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp force-pushed the emersonknapp/metadata-with-qos branch from e4587a3 to aa55d43 Compare March 26, 2020 19:50

piraka9011 approved these changes Mar 26, 2020

View reviewed changes

prajakta-gokhale approved these changes Mar 26, 2020

View reviewed changes

rosbag2_transport/test/rosbag2_transport/test_qos.cpp Outdated Show resolved Hide resolved

zmichaels11 reviewed Mar 26, 2020

View reviewed changes

rosbag2_storage/src/rosbag2_storage/metadata_io.cpp Show resolved Hide resolved

rosbag2_transport/test/rosbag2_transport/test_qos.cpp Outdated Show resolved Hide resolved

Remove leftover print

d0802b3

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

zmichaels11 approved these changes Mar 26, 2020

View reviewed changes

emersonknapp merged commit 5810588 into ros2:master Mar 26, 2020

Karsten1987 approved these changes Mar 26, 2020

View reviewed changes

emersonknapp added a commit that referenced this pull request Mar 27, 2020

Revert "Add QoS profiles field to metadata struct and provide seriali…

d84f8c3

…zation utilities (#330)" This reverts commit 5810588. Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp mentioned this pull request Mar 27, 2020

Revert PR #330 "Add QoS profiles field to metadata struct" #334

Merged

zmichaels11 reviewed Mar 27, 2020

View reviewed changes

emersonknapp added a commit that referenced this pull request Mar 27, 2020

Revert "Add QoS profiles field to metadata struct and provide seriali…

78ffcd7

…zation utilities (#330)" (#334) This reverts commit 5810588. Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

emersonknapp mentioned this pull request Mar 27, 2020

Add QoS to metadata (re-do #330) #335

Merged

emersonknapp added a commit that referenced this pull request Mar 29, 2020

Add QoS to metadata (re-do #330) (#335)

88d7178

* serialize and deserialize QoS in metadata Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QoS profiles field to metadata struct and provide serialization utilities #330

Add QoS profiles field to metadata struct and provide serialization utilities #330

emersonknapp commented Mar 26, 2020

thomas-moulard Mar 26, 2020

emersonknapp Mar 26, 2020

thomas-moulard Mar 26, 2020

emersonknapp Mar 26, 2020

emersonknapp Mar 26, 2020

thomas-moulard Mar 26, 2020 •

edited

Loading

thomas-moulard Mar 26, 2020

emersonknapp Mar 26, 2020

Karsten1987 Mar 26, 2020

emersonknapp commented Mar 26, 2020

piraka9011 left a comment

Karsten1987 left a comment

Karsten1987 Mar 26, 2020

emersonknapp Mar 26, 2020

Karsten1987 Mar 26, 2020

Karsten1987 Mar 26, 2020

Karsten1987 Mar 26, 2020

Karsten1987 Mar 26, 2020

emersonknapp commented Mar 26, 2020

Karsten1987 commented Mar 26, 2020

dirk-thomas commented Mar 27, 2020

emersonknapp commented Mar 27, 2020

zmichaels11 Mar 27, 2020

zmichaels11 Mar 27, 2020

		time.sec = node["sec"].as<uint>();
		time.nsec = node["nsec"].as<uint>();

Add QoS profiles field to metadata struct and provide serialization utilities #330

Add QoS profiles field to metadata struct and provide serialization utilities #330

Conversation

emersonknapp commented Mar 26, 2020

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

thomas-moulard Mar 26, 2020 • edited Loading

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

emersonknapp commented Mar 26, 2020

piraka9011 left a comment

Choose a reason for hiding this comment

Karsten1987 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

emersonknapp commented Mar 26, 2020

Karsten1987 commented Mar 26, 2020

dirk-thomas commented Mar 27, 2020

emersonknapp commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-moulard Mar 26, 2020 •

edited

Loading