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

Fiducial sensor for ros implemented #36

Open
wants to merge 27 commits into
base: lunar-devel
Choose a base branch
from

Conversation

maxbader
Copy link

@maxbader maxbader commented Jun 9, 2016

Hi

We enhanced the stage_ros pkg to publish fiducial sensor data.
In order to publish such data it was needed to add new messages the messages description is currently located within the stage_ros pkg. We are also in discussion with the common_msgs pkg maintainer to move the description into the sensor_msgs pkg (the brunch: without_stage_ros_messages implements this already).

I addtion we added a launch file for testing. Running:
roslaunch stage_ros world.launch world:=fiducial
should start a environment with fiducial.

We tested everything on Ubuntu 64bit 14.04 with ROS Jade, as well as on 16.04 with ROS Kinetic

We are looking forward to a merge
untitled

@maxbader
Copy link
Author

At the moment there is a discussion to add the marker message to the common_msgs pkg.
ros/common_msgs#86

@wjwwood
Copy link
Member

wjwwood commented Jun 10, 2016

Ok, thanks for the pr. One of us will get to it as soon as we have time.

@gerkey FYI

@wjwwood
Copy link
Member

wjwwood commented Jun 10, 2016

@maxbader also, when the discussion on common_msgs reaches a conclusion, please let us know on this thread. Thanks again.

@maxbader
Copy link
Author

Hi
We did now a lot of enhancements and code clean ups.

  • The markers are working with a dependency to marker_msgs
  • We added the possibility to define motion contains on a differential drive

@CodeFinder2
Copy link

CodeFinder2 commented Jan 31, 2017

Regardless of the PR's content (which is great, BTW), I would advise against using const_cast's, see (e.g.) 582784b#diff-85d0e0c7dc1f3bd834819d2137cba2c0R467

Instead, wouldn't is be possible to revisit the design of the stored member variables, possibly removing the const qualifier? Clearly, one would need to have a closer look at the impact. (I've done that in a completely modified/extended/refactored version of stage_ros and it seems fine, at least in my version.)

@wjwwood
Copy link
Member

wjwwood commented Feb 1, 2017

So is the conclusion to leave the marker messages out of common_msgs in favor of it's own package/repository?

That's fine, but we have to think carefully about adding this as a dependency of stage_ros, because stage_ros is part of what we call "desktop-full" and therefore is required to be released for any new ROS release's beta (like the upcoming ROS Lunar release) and so any dependencies of stage_ros also becomes part of that push. This just means that if we merge this and make marker_msgs a dependency of stage_ros then we are committing to ensuring it is released very early in the ROS distribution, and taking on responsibility for it if, in the future, you no longer have time to maintain that package. That's why it would have been nice to get it into common_msgs.

That being said, it's not a deal breaker, but it would make merging this an obvious decision.

@maxbader
Copy link
Author

maxbader commented Feb 2, 2017

Hi,

I started a discussion on ros/common_msgs#86 without success, because of this i created a package containing the marker message. There exists also a rviz plugin to visualize this message called marker_rviz_plugin. Both pkgs are released and already available via the ros pakage server. I don't know maybe you have more influence on the ros comunity to move the message into the common msg pkg.
In order to ensure the maintains I can give you also the proper rights on the github repository but for the next years I am happy to keep the pkg maintained.

Greetings
Markus

PS.: George (@todorangrg) will target the design issue stated by CodeFinder2 with a new commit within the next hours.

@todorangrg
Copy link

Hi,

I removed the const qualifier from the StageRobot pointers (and also all the const_casts). If you think I could change something else, please let me know. Also sorry for placing two enchantments in one pull request :)

@wjwwood wjwwood closed this May 1, 2017
@wjwwood wjwwood changed the base branch from master to lunar-devel May 1, 2017 03:23
@wjwwood
Copy link
Member

wjwwood commented May 1, 2017

Sorry, I accidentally closed this when changing the branch layout of the repository. I've reopened it and updated the target base to be lunar-devel which is the new default branch. You can edit again to point at indigo-devel, which is what is used to release for ROS Indigo and Kinetic, if you like.

@wjwwood wjwwood reopened this May 1, 2017
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.

4 participants