ROS1 recent feature port #613
Conversation
"prodives" -> "provides"
|
Ok, let me know when you want me to take a look |
|
I still need to get my ROS2 workspace properly dusted off again and build this to make sure it's OK. And then I need to find a way to run the stuff with the local Cartesian coordinate frame. |
|
Does the build farm not automatically test commits for ROS2 PRs? |
|
It should if you setup the webhooks. You set it up for ROS1 with
|
|
Yeah, the build farm should automatically add those, because I gave it permission to. And it did, but for some reason, they're showing up as reporting 503 errors:
|
|
I do not think the build farm automatically adds those, I always have to add those build.ros.org webhooks into my projects and are listed on some instructions somewhere. Anyhow, I'm also getting that warning symbol on other projects, I think something is just down right now. Did you add that webhook just now or has it been there for a long time? If a long time, then I'm not sure why it wasn't triggering PR builds.. |
|
I think if you can build / run tests on foxy we can call it good since this is having issues. Let me know if you need me to do it instead. |
|
Apparently the webhook URLs needed to be updated to use https instead of http. I've done so. Will close and reopen this to see if it triggers anything. |
|
Heh, looks like I have some things to fix. Will get my environment back up. |
|
whoop, linting 😅 |
|
A bit more than linting, it seems. The last published message time wasn't be initialized to use ROS time, and so was defaulting to system time. Comparing the two was a problem. Moreover, I was publishing the filtered position with a Neither of these issues are present in the ROS1 branches, so no worries there. We still get the occasional failure with some of the I/O tests. I really need to run those down. |
| @@ -133,7 +135,10 @@ void RosFilter<T>::reset() | |||
|
|
|||
| // Also set the last set pose time, so we ignore all messages | |||
| // that occur before it | |||
| last_set_pose_time_ = rclcpp::Time(0); | |||
| last_set_pose_time_ = rclcpp::Time(0, 0, RCL_ROS_TIME); | |||
There was a problem hiding this comment.
Do you really want to set the clock? This would make it so that you always use the ROS clock and not the other options.
There was a problem hiding this comment.
If you launched a test with use_sim_time it would use ROS time
There was a problem hiding this comment.
If you don't set it, it defaults to RCL_SYSTEM_TIME, right? And if I don't set this, the code throws an exception, because we end up comparing the current message time stamp (which is in ROS time) with this, which is in system time.
There was a problem hiding this comment.
Wouldn't the tests automatically do that?
Also, is this not like ROS1? Does ROS time not == system time for a live system, and sim time in bags?
There was a problem hiding this comment.
Just linking this so we're talking about the same stuff. You obviously have been working with ROS2 a lot more than I have, so apologies if I have this wrong.
https://design.ros2.org/articles/clock_and_time.html
The ROSTime will report the same as SystemTime when a ROS Time Source is not active. When the ROS time source is active ROSTime will return the latest value reported by the Time Source. ROSTime is considered active when the parameter use_sim_time is set on the node.
This is just like ROS1. The problem here is that if you don't provide constructor arguments, you'll get the second constructor signature here:
https://docs.ros2.org/foxy/api/rclcpp/classrclcpp_1_1Time.html
https://docs.ros2.org/foxy/api/rclcpp/classrclcpp_1_1Time.html#a683d5111f099d16532f10fcd82e8f5af
That means an uninitialized rclcpp::Time object will default to SYSTEM_TIME, as I read it. And that was the issue: the new last_published_stamp_ was not being initialized with a different constructor, so it was defaulting to RCL_SYSTEM_TIME, but we later compared it to a message stamp, which is obviously going to be of type RCL_ROS_TIME. That throws an exception, causing all my tests to fail.
In any case, tests are now all passing. 🥳
Looks like the remaining test failures are just linting |
|
Yeah, I fixed them locally. About to push. |
* navsat_transform diagram to address cra-ros-pkg#550 (cra-ros-pkg#570) * add diagram for navsat_transform Signed-off-by: Mabel Zhang <mabel@openrobotics.org> * add diagram to tutorial Signed-off-by: Mabel Zhang <mabel@openrobotics.org> * Fix frame id of imu in differential mode, closes cra-ros-pkg#482 * Added local Cartesian option to navsat_transform * Fix navsat_transform issue with starting on uneven terrain * Fix typo in navsat_transform_node.rst (cra-ros-pkg#588) "prodives" -> "provides" * Fix sign error in dFY_dP in transfer function jacobian * Making repeated state publication optional (cra-ros-pkg#595) * PR feedback * Fixing build issues * Fixing stamp issues * Linting and uncrustifying Co-authored-by: Mabel Zhang <mabel.m.zhang@gmail.com> Co-authored-by: Jeffrey Kane Johnson <jeff@mapless.ai>
| "Please use 'broadcast_cartesian_transform' instead."); | ||
| } else { | ||
| broadcast_cartesian_transform_ = | ||
| this->declare_parameter("broadcast_utm_transform", broadcast_cartesian_transform_); |
There was a problem hiding this comment.
Shouldn't this be broadcast_cartesian_transform then?
I have one more to add to this.