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

feat: python bindings for image_transport and publish #323

Merged
merged 27 commits into from
Oct 9, 2024

Conversation

tfoldi
Copy link
Contributor

@tfoldi tfoldi commented Sep 23, 2024

work in progress

implements #248

@tfoldi
Copy link
Contributor Author

tfoldi commented Sep 24, 2024

hi @ahcorde,

can you quickly check if you like this approach? It has only ImageTransport and Publisher, and Subscriber and CameraInfo handling is the next. A few notes

  • I've added copy constructor to ImageTransport (had to move Impl definition to the hpp)
  • for the rest I took your pointcloud transport py package as a template
  • for conversion between sensor_msgs.msg.image and sensor_msgs::msg::Image I used https://github.com/heudiasyc/ros2_interface_cast_generator

Example usage:

class Publisher(Node):
    def __init__(self):
        super().__init__('publisher')

        self.image_transport = ImageTransport("imagetransport", "")
        self.img_pub = self.image_transport.advertise("camera/image", 10)

        img = Image()
        img.header.frame_id = "camera"
        img.height = 480
        img.width = 640
        img.encoding = "rgb8"
        img.step = 640 * 3
        img.data = [0, 255, 0] * (img.height * img.width)
        self.img = img

        timer_period = 0.5  # seconds
        self.timer = self.create_timer(timer_period, self.timer_callback)

    def timer_callback(self):
        self.img_pub.publish(self.img)

any comments are appreciated - if not, I will continue to implement the rest. also let me know if you prefer one big PR or prefer to have one PR for publisher (like this), subscriber and then with camera_publishers.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image_transport_py/CMakeLists.txt Outdated Show resolved Hide resolved
image_transport_py/image_transport_py/__init__.py Outdated Show resolved Hide resolved
image_transport_py/image_transport_py/__init__.py Outdated Show resolved Hide resolved
image_transport_py/package.xml Outdated Show resolved Hide resolved
image_transport_py/src/image_transport_py/pybind11.hpp Outdated Show resolved Hide resolved
@tfoldi tfoldi marked this pull request as ready for review September 24, 2024 14:04
@tfoldi tfoldi requested a review from ahcorde September 24, 2024 14:04
@tfoldi
Copy link
Contributor Author

tfoldi commented Sep 24, 2024

I have a demo (example project) along with some documentation. Where is the right place to include that?

@ahcorde
Copy link
Collaborator

ahcorde commented Sep 24, 2024

Documentation should live with the code. You have here some documentation about how to create per package documentation.

@tfoldi
Copy link
Contributor Author

tfoldi commented Sep 25, 2024

sure, I can add docstrings around the bindings just like point_cloud_transport_py. However, you might noticed that https://docs.ros.org/en/ros2_packages/rolling/api/point_cloud_transport_py/modules.html is broken - none of the docstrings you specified are actually part of the published documentation.

I am unsure about the mainpage.dox file, too. I do not think it used by rosdoc2 at all. If you check https://docs.ros.org/en/rolling/p/image_transport/ none of the pages include the documentation from the https://github.com/ros-perception/image_common/blob/rolling/image_transport/mainpage.dox file.

Given this, do you suggest creating a mainpage.dox? If yes, how do you use rosdoc2 to include that file? Also, If I use pybind11 python docstrings are you sure they are going to be included in the documentation since it does not work in case of point_cloud_transport_py?

@ahcorde
Copy link
Collaborator

ahcorde commented Sep 25, 2024

sure, I can add docstrings around the bindings just like point_cloud_transport_py. However, you might noticed that https://docs.ros.org/en/ros2_packages/rolling/api/point_cloud_transport_py/modules.html is broken - none of the docstrings you specified are actually part of the published documentation.

I am unsure about the mainpage.dox file, too. I do not think it used by rosdoc2 at all. If you check https://docs.ros.org/en/rolling/p/image_transport/ none of the pages include the documentation from the https://github.com/ros-perception/image_common/blob/rolling/image_transport/mainpage.dox file.

Given this, do you suggest creating a mainpage.dox? If yes, how do you use rosdoc2 to include that file? Also, If I use pybind11 python docstrings are you sure they are going to be included in the documentation since it does not work in case of point_cloud_transport_py?

I need to review your comments and maybe submit a PR to fix this issues. Meanwhile, you can add your documentation in a README.md and then we can open a PR to do the right thing. What do you think?

@tfoldi
Copy link
Contributor Author

tfoldi commented Sep 25, 2024

sounds good. I am pretty close to finishing the subscriber and *camera part as well - let me include that, too, so the documentation will be complete.

@tfoldi tfoldi marked this pull request as draft September 26, 2024 09:00
@tfoldi tfoldi marked this pull request as ready for review September 27, 2024 07:27
@tfoldi
Copy link
Contributor Author

tfoldi commented Sep 27, 2024

I believe the PR is not ready for review; the documentation is here: https://github.com/tfoldi/image_common/blob/feat_image_transport_py/image_transport_py/README.md

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a repository to include some tutorials https://github.com/ros-perception/image_transport_tutorials. Do you mind to open a PR with the Python examples?

You can generate a ramdon image

from cv_bridge import CvBridge, CvBridgeError
import numpy as np
....
        cvb_en = CvBridge()
        original = np.uint8(np.random.randint(0, 255, size=(640, 480)))
        image_msg = cvb_en.cv2_to_imgmsg(original)
        image_msg.header.stamp = self.get_clock().now().to_msg()

image_transport_py/CMakeLists.txt Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
image_transport_py/README.md Outdated Show resolved Hide resolved
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
@tfoldi
Copy link
Contributor Author

tfoldi commented Oct 2, 2024

Is there a strong reason this is a new package rather than putting it directly in image_transport? Just to avoid python dependencies for all users of image_transport?

perhaps @ahcorde will chime in, but the major reason is to follow the same standards as https://github.com/ros-perception/point_cloud_transport/tree/rolling/point_cloud_transport_py.

On the other hand, having both Python and c++ in the same package would make the distribution easier as we need only one package to maintain. Having the same namespace from both Python and C++ could work, however, the API is slightly different due to the fact that you need to spin a new node in rclpy.

Let me know what your preference is and what you guys need to get this merged.

image_transport_py/CMakeLists.txt Outdated Show resolved Hide resolved
image_transport_py/README.md Show resolved Hide resolved
image_transport_py/package.xml Outdated Show resolved Hide resolved
@ahcorde
Copy link
Collaborator

ahcorde commented Oct 7, 2024

Pulls: #323
Gist: https://gist.githubusercontent.com/ahcorde/f31f12fa00c8c3992c147eb717ba54b9/raw/057729a85fe439a6aff41070584ad4e651d982cd/ros2.repos
BUILD args: --packages-above-and-dependencies image_transport image_transport_py --packages-above-and-dependencies image_transport image_transport_py
TEST args: --packages-above image_transport image_transport_py --packages-above image_transport image_transport_py
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14670

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@tfoldi
Copy link
Contributor Author

tfoldi commented Oct 7, 2024

will check the rhel error

@tfoldi
Copy link
Contributor Author

tfoldi commented Oct 7, 2024

The current code requires pybind11 version 2.9 or above and on RHEL the default is 2.6.2. I will check the version and use the older _ macro for older pybind11

@tfoldi
Copy link
Contributor Author

tfoldi commented Oct 8, 2024

Pulls: #323 Gist: https://gist.githubusercontent.com/ahcorde/f31f12fa00c8c3992c147eb717ba54b9/raw/057729a85fe439a6aff41070584ad4e651d982cd/ros2.repos BUILD args: --packages-above-and-dependencies image_transport image_transport_py --packages-above-and-dependencies image_transport image_transport_py TEST args: --packages-above image_transport image_transport_py --packages-above image_transport image_transport_py ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14670

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

can you please retry?

@ahcorde
Copy link
Collaborator

ahcorde commented Oct 8, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@tfoldi
Copy link
Contributor Author

tfoldi commented Oct 8, 2024

For some reason the ament_cppcheck fails on Windows:

14:18:43     Start 1: cppcheck
14:18:43 
14:18:43 1: Test command: C:\Python38\python.exe "-u" "C:/ci/ws/install/share/ament_cmake_test/cmake/run_test.py" "C:/ci/ws/build/image_transport_py/test_results/image_transport_py/cppcheck.xunit.xml" "--package-name" "image_transport_py" "--output-file" "C:/ci/ws/build/image_transport_py/ament_cppcheck/cppcheck.txt" "--command" "C:/ci/ws/install/Scripts/ament_cppcheck.exe" "--xunit-file" "C:/ci/ws/build/image_transport_py/test_results/image_transport_py/cppcheck.xunit.xml" "--language" "c++"
14:18:43 1: Working Directory: C:/ci/ws/src/tfoldi/image_common/image_transport_py
14:18:43 1: Test timeout computed to be: 300
14:18:43 1: -- run_test.py: invoking following command in 'C:\ci\ws\src\tfoldi\image_common\image_transport_py':
14:18:43 1:  - C:/ci/ws/install/Scripts/ament_cppcheck.exe --xunit-file C:/ci/ws/build/image_transport_py/test_results/image_transport_py/cppcheck.xunit.xml --language c++
14:18:43 1: [src\image_transport_py\pybind_image_transport.cpp:39]: (error: syntaxError) syntax error
14:18:43 1: 1 errors
14:18:43 1: -- run_test.py: return code 1
14:18:43 1: -- run_test.py: verify result file 'C:/ci/ws/build/image_transport_py/test_results/image_transport_py/cppcheck.xunit.xml'
14:18:43 1/1 Test #1: cppcheck .........................***Failed    0.44 sec

The same check passes on Linux*, and the file compiles on Windows as well.

In point_cloud_transport_py's CMakeLists.txt you do not have ament_cppcheck.

Few options we have:

  1. I can disable ament_cppcheck entirely (just like in point_cloud_transport_py)
  2. Disable only on Windows
  3. Debug the issue on Windows

I prefer the second option but the first is fine too. Please suggest.

@tfoldi
Copy link
Contributor Author

tfoldi commented Oct 8, 2024

I disabled cpplint on Windows (or at least I tried). Would this work?

@ahcorde
Copy link
Collaborator

ahcorde commented Oct 9, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit c2952b0 into ros-perception:rolling Oct 9, 2024
2 checks passed
@tfoldi tfoldi deleted the feat_image_transport_py branch October 9, 2024 20:13
@ahcorde
Copy link
Collaborator

ahcorde commented Oct 31, 2024

https://github.com/Mergifyio backport jazzy humble iron

Copy link

mergify bot commented Oct 31, 2024

backport jazzy humble iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit c2952b0)
mergify bot pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit c2952b0)
mergify bot pushed a commit that referenced this pull request Oct 31, 2024
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
(cherry picked from commit c2952b0)
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.

3 participants