ROS2 port

[artivis]

WIP PR for porting teleop_tools to ROS 2.

Main changes while porting:

• key_teleop: handle ctrl+c on terminal and window
• joy_teleop: action/service types have to be explicit in yaml config file, the functions to auto-deduce types from their topic aren't ported.

Fix #34 .

Port:

• joy_teleop
• key_teleop
• mouse_teleop
• teleop_tools_msgs

Let us add some unit-test alongside the port.

Tests:

• linters / copyrights
• joy_teleop
• key_teleop
• mouse_teleop (no idea how to test this guy)

Review:

• license
• maintainers
• mails

[artivis]

During porting, I don't know if the CHANGELOG files should be kept or not. Similarly what about the package version ?? Any idea @bmagyar ?

[artivis]

joy_teleop is mostly ported, however I could only test 'topic/action/srv' manually with 'buttons'-click sent from ros2 pub in terminal. It requires some more extensive testing.

[artivis]

@bmagyar @efernandez Could any of you tell me if the KeyTeleop class in key_teleop is useful for anything or can we simply remove it ??

Also I would propose to move the IncrementServer somewhere else as it is not really related to joy_teleop and very much related to Tiago. What do you think ?
Maybe @v-lopez (Hi there:) ) has an idea about it ?

[artivis]

@bmagyar @efernandez This is a fairly large PR, let me know if you would like me to break it down into smaller pieces (e.g. PR per pkg).

[v-lopez]

Also I would propose to move the IncrementServer somewhere else as it is not really related to joy_teleop and very much related to Tiago. What do you think ?
Maybe @v-lopez (Hi there:) ) has an idea about it ?

Hi!!
We rewrote the IncrementerServer in C++, in part to fix a drifting issue. here

I'd say it's useful to other robots, and its main (and so far) only user is joy_teleop. Although I agree that in itself is not related to it.

So I can't decide, having a separate package just for 120 lines of code seems overkill, but it does not entirely belong here.

[bmagyar]

I was thinking about the very same thing when I wrote the incrementerserver back in the day and decided to bundle it with joy_teleop as it is it's sole client and the amount of code does not justify making an entirely new packages for it. I'd argue it's not tiago-specific, it can be used for anything that requires incremental changes at the press of a button and has a position-like interface. Separating would only make it rot over time.

[bmagyar]

Package version: if ROS2 doesn't want to live in an entirely different universe as ROS, I suggest we bump the major version number. Considering that teleop_tools is quite mature, it's unlikely we'd want to bump major anytime soon in ROS.

[artivis]

Thanks @v-lopez @bmagyar for the feedback concerning the IncrementServer, I'll leave it their as part of the port 👍

[artivis]

Merged artivis#1. Manually checked the whole example yaml 👍

[bmagyar]

I'm actually happy with maintainer emails, author emails are somewhat of a bummer as they carry the affiliation of the authors at the time of authoring + there's no point in tracking their emails unless they want to.

[bmagyar]

almost there sir

[bmagyar]

yo

[artivis]

@bmagyar One of the functionality of joy_teleop is to instante msg/srv/action given a name in the config file. Something similar is done here (action only) using some helper class/functions from rosidl* (see here). At the moment we do this 'manually' and I would propose to rely on those rosidl* thingy. The current implementation only support interfaces fully 'qualified' such as std_msgs/msg/String but not std_msgs/String while it should. Also we would avoid code duplication and benefit from further improvement from this core package. What do you think ?

[bmagyar]

Yes I agree adding the parsers seems to be a good idea. Does it support both the fully qualified and vague names?

[artivis]

Does it support both the fully qualified and vague names?

It will, I'm waiting on ros2/rosidl_python#78 which factorize the previously linked code to functions.

[artivis]

@bmagyar unlinke what I said before, the rosidl* functions are not doing much more than what 's already done here. We can either use them at the expense of another dependency arguing about being more future proof or leave it as it is for now and revisite later if necessary.

[bmagyar]

Thanks for sticking through the review, merging!