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

REP 138 LaserScan Common Topics, Parameters, and Diagnostic Keys #25

Merged
merged 18 commits into from
Mar 14, 2013

Conversation

chadrockey
Copy link
Contributor

First REP in a series to define standardized topic names, parameter names, and diagnostic keys for various driver groups. If this one goes well, other types should follow such as GPS, IMU, Camera, and Joystick.

Posting for initial review on ROS SIG drivers on 2/21/2013.

Posted to ROS users on 2/25/2013.

@jack-oquin
Copy link
Contributor

+1 looks good

@ericperko
Copy link
Contributor

Could we make the fact that not all of the listed parameters are applicable to all types of lasers? For instance, there's a number of places where IP address is marked "only applicable for ethernet devices", but serial port is not marked similarly. I think simply stating "Not all of the below parameters should be supported by all drivers" and then using the ip_address vs serial_port as an example would clear things up.

Otherwise +1

@130s
Copy link
Contributor

130s commented Feb 25, 2013

In diagnostics section, listing possible keys would be very helpful for the developers.

I even thought that making classes for these key types might help to clarify, but looking at REP-107, it seems diagnostics defines keys only loosely on purpose. So if you say Devices should publish as many of the following keys, it might worth clarifying consistency with REP.

I think what @ericperko says can be applicable to diagnostics if we decided to require diagnostic keys, and +1.

@chadrockey
Copy link
Contributor Author

@ericperko @130s I added more description to the topics and parameters sections. Does this resolve your concerns?

@chadrockey
Copy link
Contributor Author

@tfoote Can I have a number assigned for this REP? Thanks!

@tfoote
Copy link
Member

tfoote commented Feb 25, 2013

Please Use 138

@ericperko
Copy link
Contributor

@chadrockey Yup, that seems better. +1

@dornhege
Copy link

scan and echoes state: "This topic is not present for all scanners, but scanners must output either scan or echoes."
Is it really intended to be "either - or", i.e. a scanner may not publish scan and echoes?

@dornhege
Copy link

~publish_intensity (boolean)
If true, the laser will not publish intensity to save bandwidth.

I think that's supposed to be: "If false, ..."

@chadrockey
Copy link
Contributor Author

@dornhege @kwc Does chadrockey@1d5f75b help clarify 'required' vs 'common' topics?

@dornhege Fixed that parameter, thanks!

@ahendrix Updated the topic name to be most_intense.

@kwc Yes, first/last/most_intense are published for backwards compatibility. An example roscpp support library/nodelet/node will come after this REP, but will be in the model of image_pipeline and image_proc. A multi-echo driver provides a complete MultiEcho message and the library lazily publishes whichever outputs are subscribed.

@jack-oquin
Copy link
Contributor

+1 nice work!

@dornhege
Copy link

+1

1 similar comment
@kwc
Copy link
Contributor

kwc commented Feb 26, 2013

+1

@chadrockey
Copy link
Contributor Author

I've implemented a wrapper of the URG library using this REP and I think it worked pretty well.
https://github.com/ros-drivers/urg_node

The only issue is that I have one more parameter 'publish_multiecho' that controls whether or not to put a multiecho laserscanner into single or multi return mode. I've added this to the REP. Otherwise, if no one has any more feedback, I'll request a final vote tomorrow.

@chadrockey
Copy link
Contributor Author

@tfoote This should be ready to merge.

tfoote added a commit that referenced this pull request Mar 14, 2013
REP 138 LaserScan Common Topics, Parameters, and Diagnostic Keys Merging as Active
@tfoote tfoote merged commit 2b3f115 into ros-infrastructure:master Mar 14, 2013
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/discussion-standardizing-ros-interfaces/33693/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants