-
Notifications
You must be signed in to change notification settings - Fork 330
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
Added more parameters for camera topic examples. #425
Conversation
6f4682e
to
09210d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Maybe it's useful to be able to set the camera ID, but I don't we should pass the topic names as parameters since the recommended way to change topics is with remapping. For example,
ros2 run image_tools showimage --ros-args -r image:=another_topic
In fact, I've removed this from other demos in the past (#401).
Also, I don't see the value in configuring the window name with a parameter. It may distract from the demo.
I didnt think about remapping before, but will this still work if i have two cam2img nodes? In my setup i have two cameras and i would like to stream both of them to their own topic. Regarding the window name i think i will get the same problem of two windows with the same name if i want to display both camera streams. Also i dont think the extra parameters will distract users much from the demo as they are optional, but they make the demo (as a build in feature) more usable for other purposes. (I could replace an old ros1 package i used before) By the way, i was very glad when i found out yesterday that the topic argument existed before. i have ros2 crystal running on a raspi and could not build the package with my changes as a lot other things here have changed inbetween. |
Yes, remapping can be configured per node. For example, if there is more than one node in a process, then we can prefix remaps with the node name and a colon (reference):
Alternatively, launch supports remapping node topics (e.g. see launch XML design) I'll add that this demo code is just that, a demonstration. It is not intended to be used directly in other applications and so I'm not convinced we need to worry about supporting more general use-cases. I recommend adapting the code as you need for your application, or use existing tools like |
Thanks for the example and links. If i try this with one node it is working. But if i try it with two nodes it doesnt do most of the times. Using following commands:
i think the same problem would occur with my topic arguments, as i believe the problem is that im using the same node twice. I also tried to rename the node only with But in rare cases i got both nodes topics at the same time, as intended, but using the same commands and order again (im starting it with docker-compose), the next time i only get one nodes topics. Is there an argument to run both nodes at the same time? |
<launch>
<node namespace="/card_cam" pkg="image_tools" exec="cam2image" output="screen">
<param name="device_id" value="0" />
</node>
<node namespace="/face_cam" pkg="image_tools" exec="cam2image" output="screen">
<param name="device_id" value="1" />
</node>
</launch> You can start it with Edit: using namespace instead of node-name in example. |
I added your suggestion to use the topic name as window name, but only as default value so that a user still can use a custom window name if he likes to. Using a launch file to start the two camera nodes did work for me. Afterwards i tried running the two separate commands again and i found out this only works if i dont give parameters with -p. If i run it with extra parameters like frequency or width and height the topics of one node always were missing. So im using the launch file solution now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One lint error to fix, otherwise LGTM
There was a compiler warning on Windows, I think 3ba0218 should fix it. |
What do we do with this warning now? |
Sorry, this PR dropped off my radar. I can spend some time investigating on a Windows host when I have a moment. |
Added parameter for custom topic names, display window name and camera device id.
Signed-off-by: DanBmh