-
Notifications
You must be signed in to change notification settings - Fork 51
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
Jesus/#169 camera info yaml #231
Jesus/#169 camera info yaml #231
Conversation
I have tried running it but as I don't possess an Andino at home, I haven't been able to fully test it, although no errors were found about the new YAML file. |
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 @JesusSilvaUtrera for contributing to Andino!
Please notice that the DCO check is failing because the commit is not signed. See https://github.com/Ekumen-OS/andino/pull/231/checks?check_run_id=22874979577 for instructions on how to fix it.
On the other hand, I would appreciate it if you could use the full PR description template (nit).
Thanks!
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.
Note that the camera intrinsics are used to publish a CameraInfo message typically at camera_info
topic
https://docs.ros2.org/foxy/api/sensor_msgs/msg/CameraInfo.html
Here you are adding ros2 parameters via a yaml file. Which is actually good however those aren't intrinsics paremeters to the camera.
(Camera intrinsics and extrinsics )
The issue is aimed to provide intrinsics values (Ideally can be obtained via calibration, we could opt for something grabbed from out there typically for the RaspiCam) so as to be loaded by the v4l2_camera_node
(or any other node that works for us) and to be ultimately published to the camera_info
topic.
(e.g when running the simulation you will notice that you have two topics for the camera: The image and the camerainfo)
@francocipollone I have been looking into this and I have some questions (maybe some are silly, sorry in advance about that jaja):
Also, I assume those distortion coefficients are set to 0 by default, not because a calibration has been done, and I also assume that when the intrinsics are computed (via calibration or via copying from the internet) they should also be specified here, as well as in the YAML file.
ros2 run camera_calibration cameracalibrator --size 7x9 --square 0.02 --ros-args -r image:=/my_camera/image_raw -p camera:=/my_camera
We can discuss these topics in a meeting if you prefer. Thanks in advance! |
Interesting finding indeed! A couple of references: That is expected behavior given how it is built (see here and here ). Note the function |
Correct. I think we should try to unify them in one place and source them everywhere else. |
Here you can find where the camera info is published in the very same However, the publisher is used only when using intra_process_communication. I honestly don't know why that default. The way we are launching the node makes it behave that way, see the default rclcpp::NodeOptions::use_intraprocess_comm value (false). Reference to our launchfile and reference to the node executable. To the best of my knowledge, to get the camera_info topic to be published we should have our own executable that takes care of creating and spinning the node. |
So, just to recap after reviewing all of @agalbachicar references:
We can discuss all of this and see which actions are better to take. |
Per F2F conversation with @JesusSilvaUtrera , we should be able to set also the intra_process_comms flag in the launch file preventing the creation of a custom binary to launch the node. |
Signed-off-by: Franco Cipollone <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: Gary Servin <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: Javier Balloffet <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: Franco Cipollone <[email protected]> Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: JesusSilvaUtrera <[email protected]>
* andino_apps package created and andino_navigation package updated Signed-off-by: JesusSilvaUtrera <[email protected]> * Minor changes from the PR Signed-off-by: JesusSilvaUtrera <[email protected]> * Update andino_navigation README with changes from PR Signed-off-by: JesusSilvaUtrera <[email protected]> * Add andino_apps package to ci.yaml Signed-off-by: JesusSilvaUtrera <[email protected]> * Fixed minor issues from the PR Signed-off-by: JesusSilvaUtrera <[email protected]> * Added 'andino_apps' to the general README Signed-off-by: JesusSilvaUtrera <[email protected]> --------- Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: JesusSilvaUtrera <[email protected]>
Signed-off-by: JesusSilvaUtrera <[email protected]>
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.
Sorry for the radio silence here.
Let me know if it is ready for review. I notice there are a couple of conflicts.
Don't worry about this for now, it won't be ready until I have the camera and calibrate it (Javi has it right now, I will probably get it the next day we meet). I will let you know when this is ready, thanks Franco! |
Signed-off-by: JesusSilvaUtrera <[email protected]>
…n andino_hardware/README Signed-off-by: JesusSilvaUtrera <[email protected]>
Some notes here:
Let me know what do you think @francocipollone @jballoffet, thanks! |
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 a lot for this contribution.
I haven't tested locally but I will. I left some comments to go
Signed-off-by: JesusSilvaUtrera <[email protected]>
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.
LGTM
image_width: 640 | ||
image_height: 480 | ||
# camera_name: narrow_stereo | ||
camera_name: hd_webcam:_hd_webcam | ||
camera_name: narrow_stereo |
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.
nit: not sure where this name is being used however it catches my attention it says stereo when it is actually mono
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.
Yeah, but the name is fixed by the hardware (happens the same when I used my webcam) so there is anything we can do about that. If I change the name on the YAML it won't use that file because it doesn't have the right name.
@francocipollone BTW, since Javi is OoO and I don't think he will review this, feel free to merge it once you don't have anything else to discuss (I can't do it because I don't have wrote permissions) . Thanks! |
🦟 Bug fix
Closes #169
Summary
I have added a YAML file with the default camera intrinsics to load into the camera.launch.py file. I also included as a comment the link to the info of the camera, just in case we need more technical parameters of it.
The path to the YAML file is included as a parameter in the launch file, just in case it needs to be modified.