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

roscpp param::init() needs to parse mappings using yaml-cpp to be consistent with rospy #1498

Open
peci1 opened this Issue Aug 31, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@peci1
Copy link
Contributor

peci1 commented Aug 31, 2018

ROS users have many times observed weird behavior inconsistencies between rospy/rosparam and roscpp when using parameters (#1339, #1153, #445, #390, #436). I'm not sure about the other client libraries.

I think there needs to be an agreement made that this is unacceptable: users of nodes really should not care whether the nodes are written using rospy, roscpp, rosrust or whatever client library... to just know what kind of behavior they can expect from rosrun and rosparam. AFAIK, requirement for stable param API across all client libraries is not covered by any REP. If it is, please, point me to it. The only mention I found is that ROS uses YAML syntax to determine the parameter typing, which is an optional feature of client libraries.

I hope everybody would agree that rospy should be taken as the reference implementation (as e.g. rosparam is written using rospy). Other client libraries should therefore test their API interoperability with rospy.

I see two action points now:

  • Reimplement ros::param::init using yaml-cpp .
  • Create the interop tests for roscpp and a generic test that would call rosrun on nodes written in various languages, trying to play with the parameters from those nodes, and check the result using rospy.

Knowing about #1496, I can work on the implementation. Just before I start doing so, I'd like to know it is something that has a high probability of being merged (not necessarily to melodic or older releases, but at least to current master). And another question - is it okay to introduce yaml-cpp as a dependency to libros? I know this library is a bit cursed with all its version/API incompatibilities, but at least rviz also depends on it. However, introducing this dependency to libros would mean yaml-cpp will get installed even with ros-DISTRO-ros-base, whereas now I think it only gets installed with -desktop.

This change will definitely change behavior of some roscpp programs, but as I said in #436 , it should only break code that is already broken (but the authors did not notice/care).

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Aug 31, 2018

Maybe it would be good to start with a REP to formalize this. That document could then also clearly describe what breaking changes are necessary to make the different client libraries consistent. Depending how these breaking changes look like the decision if / when it can be merged will be made.

@peci1

This comment has been minimized.

Copy link
Contributor Author

peci1 commented Aug 31, 2018

Sounds reasonable. Should I prepare a draft, or does Open robotics want to lead this effort?

@dirk-thomas

This comment has been minimized.

Copy link
Member

dirk-thomas commented Aug 31, 2018

Please go ahead. I don't think anyone at OSRF will be able to spend time on this (beside providing feedback of course).

@peci1

This comment has been minimized.

Copy link
Contributor Author

peci1 commented Aug 31, 2018

Okay, I'll try my best :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment