Rebased joystick_drivers on the latest

[clalancette]

This is a rebase of the ROS2 version of joystick_drivers onto the upstream master to pull in a few fixes. While I was doing that, I also made some changes to make this fork closer to the upstream (such as restoring some files that we had earlier deleted). As this is a rebase, the GitHub diff won't work and this will be force pushed once approved. Instead, here are some diffs that may be more interesting to review:

1. https://gist.github.com/clalancette/45291c03f1448e7a0c9be97c2fb29699 has the differences between 1fdd2bf (ros2 branch) and this branch.
2. https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1 has the differences between ros-drivers@e2ab008 and this branch.

[clalancette]

CI:

• Linux

[mikaelarguedas]

A few things are unclear to me, could you clarify the following points?

• Motivation for adding back rosbag migration rules
• Motivation for adding back diagnostic variables
• Motivation for removing OSRF from the copyright and use /***/ comment block for the copyright
• If we pull in new changelogs and version bump commits, should we pull the tags matching these versions as well ?

[clalancette]

Motivation for adding back rosbag migration rules

Just to make it closer to upstream, since it doesn't hurt anything to have it.

Motivation for adding back diagnostic variables

Ditto.

Motivation for removing OSRF from the copyright and use /***/ comment block for the copyright

Ditto. I wasn't quite sure whether to keep OSRF in the copyright as well. I could go either way on that.

If we pull in new changelogs and version bump commits, should we pull the tags matching these versions as well ?

Hm, I'm not sure. How have we handled it before when we did a rebase and a force push?

[mikaelarguedas]

Motivation for adding back rosbag migration rules

Just to make it closer to upstream, since it doesn't hurt anything to have it.

As the previous version of that message never existed in ROS 2 these rules will never apply.
Given that we didnt discussed/decide how migration rules could work in ROS 2 in the future. I think this sets a wrong expectation that this file makes sense in ROS 2. So I'm in favor of keeping it out of the ros2 branches.

Motivation for adding back diagnostic variables

Ditto.

As all the diagnostic code is commented out, I think it makes more sense to keep the diag variables commented out as well as they are not used.

Motivation for removing OSRF from the copyright and use /***/ comment block for the copyright

Ditto.

It looks like this code now will be very close of passing the linters (hence the copyright format question).
Regarding the OSRF copyright notice. I don't feel strongly either way I was just surprised to see it being removed.

Hm, I'm not sure. How have we handled it before when we did a rebase and a force push?

I don't think we faced it before. As the last ROS1 release of this was beginning of 2017 so we already had all the upstream tags when we forked/ported it.

[clalancette]

As the previous version of that message never existed in ROS 2 these rules will never apply.
Given that we didnt discussed/decide how migration rules could work in ROS 2 in the future. I think this sets a wrong expectation that this file makes sense in ROS 2. So I'm in favor of keeping it out of the ros2 branches.

All right, I've removed them again in 216c4c4.

As all the diagnostic code is commented out, I think it makes more sense to keep the diag variables commented out as well as they are not used.

Personally, I'd rather have them uncommented and a bit closer to upstream, as they really don't affect very much. But I won't hold up the whole PR for that, so if you really think I should comment them back out, I will.

It looks like this code now will be very close of passing the linters (hence the copyright format question).
Regarding the OSRF copyright notice. I don't feel strongly either way I was just surprised to see it being removed.

I'm not sure whether it makes sense to run the linters on "ported" code like this anyway. I guess I could push for the // on the upstream repo, but it doesn't seem like it is much worth it.

I don't think we faced it before. As the last ROS1 release of this was beginning of 2017 so we already had all the upstream tags when we forked/ported it.

Oh, I see. I think I misunderstood your earlier point. Yeah, if we are pulling these in, we should definitely update "master" and push all of the tags there as well. As this PR goes in I'll do that.

[mikaelarguedas]

TL;DR: sounds good to me to keep the changes in for this PR, we can always revisit later.

Personally, I'd rather have them uncommented and a bit closer to upstream, as they really don't affect very much. But I won't hold up the whole PR for that, so if you really think I should comment them back out, I will.

I agree it will not impact the performance of the node, just didn't get the motivation as in my mind it made sense for unused code to be commented out.
So fine either way 👍.

I'm not sure whether it makes sense to run the linters on "ported" code like this anyway. I guess I could push for the // on the upstream repo, but it doesn't seem like it is much worth it.

Sorry for the confusion, my point was more that it seemed that being able to lint was the original motivation for changing the license header.
But we should not push to get it changed upstream, we should fix ament_copyright instead. ament/ament_lint#82 should be addressed instead.
But that's fine to keep the upstream one as this code is currently not linted 👍

I don't think we faced it before. As the last ROS1 release of this was beginning of 2017 so we already had all the upstream tags when we forked/ported it.
Oh, I see. I think I misunderstood your earlier point. Yeah, if we are pulling these in, we should definitely update "master" and push all of the tags there as well. As this PR goes in I'll do that.

So I've been going back and forth on this and it's unclear to me how we should handle changelogs of upstream repo, as we generate our own changelogs each release (e.g. https://github.com/ros2/joystick_drivers/blob/2.1.0/joy/CHANGELOG.rst), and we will likely conflict in the future.
Let's keep them for now and pull the tags, and revisit this later when we discuss how to handle and maintain ros2 forks.

[mikaelarguedas]

Can you please post an updated diff when you get a chance for final review?

[clalancette]

Can you please post an updated diff when you get a chance for final review?

Sure! Here you go:

• ros1->ros2-rebased: https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1
• ros2->ros2-rebased: https://gist.github.com/clalancette/45291c03f1448e7a0c9be97c2fb29699

[mikaelarguedas]

Some comments by reading the diff between ROS 1 and ROS 2.

• As we're updating the code along the way I recommend switching to RCLCPP logging macros in C++ nodes.

• Re: diagnostic variable uncommented:
  the TODO should be moved to the appropriatep lace if we uncomment diagnostic variables in this patch

• Nit: if the goal is to keep the diff minimal I would'nt comment out enpty lines: https://github.com/ros2/joystick_drivers/pull/8/files#diff-cb87ab5ef18fe266e1e337d672bd84feR73

• Is this change necessary? https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L366 if we consider it it we should submit it upstream to avoid diverging.

• Shouldn't this be ported rather than commented out ? https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L384

• If we want to keep upstream style this should either use open braces or no braces but not the cuddled braces: https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L407

• These lines can be removed as the timestamp is zero initialized in the messages: https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L445-L446
  As for the resizes, it's unclear to me why thay are needed in ROS 2 but not in ROS 1. Do you happen to know why ?

[clalancette]

• As we're updating the code along the way I recommend switching to RCLCPP logging macros in C++ nodes.

Done, except for in the method get_dev_by_joy_name. There is no node to get a logger from there, so I left it as RCUTILS_LOG_* for now. I could pass the node into the method, but it feels a little odd to do that just for the logger context. Is there a recommended way to do this?

• Re: diagnostic variable uncommented:
  the TODO should be moved to the appropriatep lace if we uncomment diagnostic variables in this patch

Done now.

• Nit: if the goal is to keep the diff minimal I would'nt comment out enpty lines: https://github.com/ros2/joystick_drivers/pull/8/files#diff-cb87ab5ef18fe266e1e337d672bd84feR73

Done now.

• Is this change necessary? https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L366 if we consider it it we should submit it upstream to avoid diverging.

It is necessary in ROS2, at least right now. The problem is that the RCUTILS_* and RCLCPP_* macros expand to more than one line, so the construct:

if (condition)
   RCLCPP_INFO();
else {
}

Is going to have problems. This actually seems like a bug to me, so I opened up ros2/rcutils#113 . I also opened up ros-drivers#126 to put brackets in upstream.

• Shouldn't this be ported rather than commented out ? https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L384

Fixed now.

• If we want to keep upstream style this should either use open braces or no braces but not the cuddled braces: https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L407

Good call, fixed now.

• These lines can be removed as the timestamp is zero initialized in the messages: https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L445-L446
  As for the resizes, it's unclear to me why thay are needed in ROS 2 but not in ROS 1. Do you happen to know why ?

No, I can't remember why I did that during the initial port. I've now removed all of that initialization, with the exception of frame_id = "joy", which seems like a good thing to have.

I've updated the diffs as well:

ros2 -> ros2-rebased: https://gist.github.com/clalancette/45291c03f1448e7a0c9be97c2fb29699
ros1 -> ros2-rebased: https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1

[mikaelarguedas]

It is necessary in ROS2, at least right now. The problem is that the RCUTILS_* and RCLCPP_* macros expand to more than one line, so the construct:

Then all instances of this should get brackets added isn't it ? e.g. https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L393

[clalancette]

Then all instances of this should get brackets added isn't it ? e.g. https://gist.github.com/clalancette/266f9e6e3b072d73527cbb8939ef7bc1#file-ros1-ros2-diff-L393

Definitely. Done in cdd5350

[mikaelarguedas]

Done, except for in the method get_dev_by_joy_name. There is no node to get a logger from there, so I left it as RCUTILS_LOG_* for now. I could pass the node into the method, but it feels a little odd to do that just for the logger context. Is there a recommended way to do this?

Did you look at how the demos log in free functions?

[clalancette]

I changed to to pass the logger in 25b6586

[mikaelarguedas]

lgtm with new CI + manual testing

[clalancette]

lgtm with new CI + manual testing

Thanks! New CI:

• Linux

[mikaelarguedas]

This can now be updated to use seconds() (from ros2/rclcpp#526) instead of performing the conversion from nanoseconds manually: 479ffcf#diff-cb87ab5ef18fe266e1e337d672bd84feR236

[clalancette]

I've now done manual testing of this, and things seem to work (better now, in that Ctrl-C of the node actually kills it). I'm going to force push the rebase to this repository, then close out this PR. Thanks for the review.

[clalancette]

Oh, I'll also note that I did make the change referred to in #8 (comment)