Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROS2 port #35

Merged
merged 40 commits into from Sep 3, 2019

Conversation

@artivis
Copy link

commented Jun 25, 2019

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
ROS2 port
Signed-off-by: artivis <jeremie.deray@canonical.com>

@artivis artivis force-pushed the artivis:dashing-devel branch from 6b9f3f1 to 853d104 Jun 25, 2019

artivis added 4 commits Jun 25, 2019
key_teleop pkg format 3
Signed-off-by: artivis <jeremie.deray@canonical.com>
port teleop_tools_msgs
Signed-off-by: artivis <jeremie.deray@canonical.com>
key_teleop catch KeyboardInterrupt
Signed-off-by: artivis <jeremie.deray@canonical.com>
port mouse_teleop
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

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 added 4 commits Jun 26, 2019
wip port joy_teleop
Signed-off-by: artivis <jeremie.deray@canonical.com>
add key_teleop.yaml
Signed-off-by: artivis <jeremie.deray@canonical.com>
fix msg cmake & pkg.xml
Signed-off-by: artivis <jeremie.deray@canonical.com>
wip port joy_teleop
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

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 added 2 commits Jul 2, 2019
wip port joy_teleop
Signed-off-by: artivis <jeremie.deray@canonical.com>
wip port joy_teleop
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

@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

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

@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).

artivis added 3 commits Jul 2, 2019
prepare tests
Signed-off-by: artivis <jeremie.deray@canonical.com>
add xmllint test
Signed-off-by: artivis <jeremie.deray@canonical.com>
fix xmllint tests
Signed-off-by: artivis <jeremie.deray@canonical.com>
@v-lopez

This comment has been minimized.

Copy link

commented Jul 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

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

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

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

joy_teleop/package.xml Show resolved Hide resolved
joy_teleop/test/key_teleop.test.py Outdated Show resolved Hide resolved
joy_teleop/test/key_teleop.test.py Outdated Show resolved Hide resolved
joy_teleop/test/test_copyright.py Show resolved Hide resolved
key_teleop/config/key_teleop.yaml Show resolved Hide resolved
key_teleop/scripts/key_teleop.py Outdated Show resolved Hide resolved
key_teleop/setup.py Outdated Show resolved Hide resolved
key_teleop/setup.cfg Show resolved Hide resolved
mouse_teleop/package.xml Show resolved Hide resolved
artivis added 6 commits Jul 4, 2019
simplify joy_teleop retrieve_config
Signed-off-by: artivis <jeremie.deray@canonical.com>
todo tests
Signed-off-by: artivis <jeremie.deray@canonical.com>
rename test
Signed-off-by: artivis <jeremie.deray@canonical.com>
remove useless class KeyTeleop
Signed-off-by: artivis <jeremie.deray@canonical.com>
@bmagyar
Copy link
Member

left a comment

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.

@artivis artivis marked this pull request as ready for review Aug 2, 2019

@bmagyar
Copy link
Member

left a comment

almost there sir

joy_teleop/config/joy_teleop_example.yaml Outdated Show resolved Hide resolved
joy_teleop/scripts/incrementer_server.py Outdated Show resolved Hide resolved
joy_teleop/scripts/incrementer_server.py Outdated Show resolved Hide resolved
joy_teleop/scripts/incrementer_server.py Outdated Show resolved Hide resolved
joy_teleop/scripts/joy_teleop.py Outdated Show resolved Hide resolved
joy_teleop/test/test_key_teleop.py Outdated Show resolved Hide resolved
key_teleop/package.xml Outdated Show resolved Hide resolved

import curses
import math
# import math

This comment has been minimized.

Copy link
@bmagyar
key_teleop/setup.py Outdated Show resolved Hide resolved
key_teleop/test/test_xmllint.py Outdated Show resolved Hide resolved
typo
Co-Authored-By: Bence Magyar <bence.magyar.robotics@gmail.com>
@artivis

This comment has been minimized.

Copy link
Author

commented Aug 3, 2019

@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 ?

artivis added 3 commits Aug 3, 2019
rm comments
Signed-off-by: artivis <jeremie.deray@canonical.com>
@bmagyar

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

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

@artivis

This comment has been minimized.

Copy link
Author

commented Aug 6, 2019

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

This comment has been minimized.

Copy link
Author

commented Sep 3, 2019

@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
bmagyar approved these changes Sep 3, 2019
@bmagyar

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Thanks for sticking through the review, merging!

@bmagyar bmagyar merged commit a4f697f into ros-teleop:dashing-devel Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.