Map duration and time messages #106
Conversation
|
Waiting for review. |
| @@ -31,27 +31,26 @@ namespace ros1_bridge | |||
| template<> | |||
| void | |||
| convert_1_to_2( | |||
| const ros::Duration & ros1_msg, | |||
| const ros::Duration & ros1_type, | |||
There was a problem hiding this comment.
This change doesn't make sense to me. Can you clarify why you changed only one of the names of this prototype? and that the names don't match what's used in the implementation at https://github.com/ros2/ros1_bridge/pull/106/files#diff-e7604d29ef87753b67dcf843c9e940caR72?
There was a problem hiding this comment.
these are not messages but ROS 1 types hence the change in name. The prototype you're pointing to is taking messages and not types so using the _msg there is appropriate
There was a problem hiding this comment.
This patch changes the ros1_msg argument to ros1_type argument for all convert_1_to_2 / convert_2_to_1 functions in the header as well as the source file. The rational is that neither of the two arguments ros::Duration nor ros::Time is a message but just a plain type.
The function you are referencing is unrelated to this signature. That one is for actual message in the builtin_interfaces packages and therefore those arguments still use _msg as the argument suffix.
There was a problem hiding this comment.
ok, I'll keep looking at this to get my head around it myself, in the meantime the approval from @mikaelarguedas suggests it's just my misunderstanding so not a blocker 😄
There was a problem hiding this comment.
following up, @mikaelarguedas pointed out what I was missing, which is that these particular msgs, fundamentally, are represented differently in ROS 1 and ROS 2: time/duration isn't a message in ROS 1, it is a data type, whereas in ROS 2 time/duration has message definitions associated with them. The contents of ros1_msg.data at this line is not another message.
resource/get_factory.cpp.em
Outdated
| #include "ros1_bridge/builtin_interfaces_factories.hpp" | ||
| #include "ros1_bridge/factory.hpp" |
There was a problem hiding this comment.
I would rather revert this change. Before it was including first the factory interface and then all specialized factories. That made sense to me. Also the include order isn't being checked at the moment since some includes are hard coded and some other are being generated. So the overall order won't pass the inter anyway (without custom logic ordering the superset).
There was a problem hiding this comment.
Feel free to revert it, I wanted to save an iteration but apparently I should have just commented to get feedback rather than commiting the change.
In my mind as the specialized factories include the generic ones the order didn't matter and the day we remove the include of any specialized one the factory interface will still be imported
There was a problem hiding this comment.
Np, better being proactive - I appreciate it!
dirk-thomas
force-pushed
the
builtin_interfaces_messages
branch
from
a55748f to
f2caf7a
Compare
Only slightly related to #104.
While the bridge can already handle fields with a
DurationorTimetype it currently can't map the messagesstd_msgs/Duration|Timewithbuiltin_interfaces/Duration|Time.With this patch the following appears when invoking the dynamic bridge with
--print-pairs:And the following example works:
ros2 topic echo /time builtin_interfaces/Time