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

add joy_remap and its sample #120

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@furushchev

furushchev commented May 22, 2018

Added a node for remapping buttons and axes in sensor_msgs/Joy to emulate other type of controllers such as DUALSHOCK4 to DUALSHOCK3.
This PR also includes a sample launch file and its config.

@JWhitleyAStuff JWhitleyAStuff changed the base branch from indigo-devel to master Jun 11, 2018

@JWhitleyAStuff

This comment has been minimized.

Show comment
Hide comment
@JWhitleyAStuff

JWhitleyAStuff Jun 11, 2018

Contributor

I completely get what you're trying to do here and I understand the utility of it. However, I have a few major issues with the implementation:

  1. You are essentially running arbitrary Python that is stored in a .yaml file. Even though Python is an interpreted language, I believe the Python scripts in a ROS package get compiled into .pyc files at build time (correct me if I'm wrong) making the compiled code essentially immutable. Passing in arbitrary Python scripts to be evaluated seems like very bad practice in this context from both a security and stability standpoint.
  2. The Joy node is written in C++. It feels like we're adding a significant amount of bloat by requiring rospy in addition to roscpp just for a data conversion node.
  3. How much general-purpose use do you think this functionality will really add? How often will people both a. know the complete mapping of the joystick that is attached (different for nearly every supported type of joystick) and b. want to remap all of those buttons/axes?

I'm certainly open to discussion on this but after a bit of thought, I'm not convinced right now that it has enough broad utility to be included in this package.

Contributor

JWhitleyAStuff commented Jun 11, 2018

I completely get what you're trying to do here and I understand the utility of it. However, I have a few major issues with the implementation:

  1. You are essentially running arbitrary Python that is stored in a .yaml file. Even though Python is an interpreted language, I believe the Python scripts in a ROS package get compiled into .pyc files at build time (correct me if I'm wrong) making the compiled code essentially immutable. Passing in arbitrary Python scripts to be evaluated seems like very bad practice in this context from both a security and stability standpoint.
  2. The Joy node is written in C++. It feels like we're adding a significant amount of bloat by requiring rospy in addition to roscpp just for a data conversion node.
  3. How much general-purpose use do you think this functionality will really add? How often will people both a. know the complete mapping of the joystick that is attached (different for nearly every supported type of joystick) and b. want to remap all of those buttons/axes?

I'm certainly open to discussion on this but after a bit of thought, I'm not convinced right now that it has enough broad utility to be included in this package.

@furushchev

This comment has been minimized.

Show comment
Hide comment
@furushchev

furushchev Jun 11, 2018

@JWhitleyAStuff Hi, thank you very much for your review!
Actually, in this PR, I tried to re-implement the past joystick_remapper node (unfortunately, I could not find binary or codes for this node now though wiki page exists: http://wiki.ros.org/joystick_remapper )
and improve its functionality.
The original one allows only remapping key assignments in either buttons or axes and no negation or putting constant values.
I'm also very welcome for further discussion.

furushchev commented Jun 11, 2018

@JWhitleyAStuff Hi, thank you very much for your review!
Actually, in this PR, I tried to re-implement the past joystick_remapper node (unfortunately, I could not find binary or codes for this node now though wiki page exists: http://wiki.ros.org/joystick_remapper )
and improve its functionality.
The original one allows only remapping key assignments in either buttons or axes and no negation or putting constant values.
I'm also very welcome for further discussion.

@JWhitleyAStuff

This comment has been minimized.

Show comment
Hide comment
@JWhitleyAStuff

JWhitleyAStuff Aug 22, 2018

Contributor

@furushchev - I appreciate your background but that doesn't really answer any of my concerns about passing arbitrary code through a launch file or adding Python.

Contributor

JWhitleyAStuff commented Aug 22, 2018

@furushchev - I appreciate your background but that doesn't really answer any of my concerns about passing arbitrary code through a launch file or adding Python.

@furushchev

This comment has been minimized.

Show comment
Hide comment
@furushchev

furushchev Aug 23, 2018

@JWhitleyAStuff Sorry for not pointing that. I introduced safe "eval" function that does not use eval function and use very limited functions / operators instead.

FYI: the available operators are now:

  • + (add)
  • - (sub)
  • * (mul)
  • / (div)
  • ^ (xor)
  • - (negative)
  • abs
  • max
  • min

furushchev commented Aug 23, 2018

@JWhitleyAStuff Sorry for not pointing that. I introduced safe "eval" function that does not use eval function and use very limited functions / operators instead.

FYI: the available operators are now:

  • + (add)
  • - (sub)
  • * (mul)
  • / (div)
  • ^ (xor)
  • - (negative)
  • abs
  • max
  • min
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment