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

Add tracing to image_pipeline and instrument two ROS 2 components #717

Merged
merged 6 commits into from
Jan 24, 2022

Conversation

vmayoral
Copy link
Contributor

  • Fork tracetools and shape it as tracetools_image_pipeline to avoid external dependencies.
  • Tracing first examples: image_proc::RectifyNode and image_proc::ResizeNode

Add an LTTng tracing provider wrapper for image_pipeline
metapackage. Include tracepoints for two initial ROS 2
components: image_proc::RectifyNode and ResizeNode

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
@vmayoral
Copy link
Contributor Author

vmayoral commented Dec 20, 2021

Ping @christophebedard, I'd like you to confirm you're fine with this.

Note that I've forked tracetools and instead, this PR proposes to create a new ROS 2 package tracetools_image_pipeline which includes only the TRACEPOINTS for this meta-package and which can be maintained more easily (instead of having to send PRs to tracetools and iterate in longer cycles).

The rationale behind this comes from the the experience of having worked for a few months with the ROS 2 tracing infrastructure. To me, it eventually become rather cumbersome to have to maintain a single tracetools repo with all tracepoints (I experienced this directly with an extension of tracetools_acceleration we keep internally). Altogether this led me to realize that it might be better to simply add a tracetools_<whatever-ros2-stack/package>.

@vrabaud and @JWhitleyWork, note that this contribution adds the new package proposed as a REQUIRED dependency, if LTTng is not installed into the system, it should nicely inform that tracing is disabled but packages should build still fine. For now only image_proc::RectifyNode and image_proc::ResizeNode have been instrumented but if this approach receives a 👍, happy to instrument a few more components of the pipeline.

A simple launch file to test it:

"""Launch a raw image publisher, pipelined with rectify and resize"""

import os
import rclpy
import launch
from rclpy.node import Node
from std_msgs.msg import String
from sensor_msgs.msg import CameraInfo, Image
from launch_ros.actions import ComposableNodeContainer, Node
from launch_ros.descriptions import ComposableNode
from tracetools_launch.action import Trace
from tracetools_trace.tools.names import DEFAULT_EVENTS_ROS


def generate_launch_description():
    """Generate launch description with multiple components."""

    # # An image_raw publisher
    # image_raw_publisher = Node(
    #     package="perception_2nodes",
    #     executable="image_raw_publisher",
    #     name="image_raw_publisher",
    #     remappings=[("image_raw", "image")],
    # )

    # Trace
    trace = Trace(
        session_name="raw_rectify_resize_pipeline",
        events_ust=[
            "ros2_image_pipeline:*",
        ]
        + DEFAULT_EVENTS_ROS,
    )

    perception_container = ComposableNodeContainer(
        name="perception_container",
        namespace="",
        package="rclcpp_components",
        executable="component_container",
        composable_node_descriptions=[
            ComposableNode(
                package="image_proc",
                plugin="image_proc::RectifyNode",
                name="rectify_node",
            ),
            ComposableNode(
                namespace="resize",
                package="image_proc",
                plugin="image_proc::ResizeNode",
                name="resize_node",
                remappings=[
                    ("camera_info", "/camera_info"),
                    ("image", "/image_rect"),
                    ("resize", "resize"),
                ],
                parameters=[
                    {
                        "scale_height": 2.0,
                        "scale_width": 2.0,
                    }
                ],
            ),
        ],
        output="screen",
    )
    # return launch.LaunchDescription([image_raw_publisher, perception_container, trace])
    return launch.LaunchDescription([perception_container, trace])

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
@christophebedard
Copy link

christophebedard commented Dec 20, 2021

Ping @christophebedard, I'd like you to confirm you're fine with this.

Note that I've forked tracetools and instead, this PR proposes to create a new ROS 2 package tracetools_image_pipeline which includes only the TRACEPOINTS for this meta-package and which can be maintained more easily (instead of having to send PRs to tracetools and iterate in longer cycles).

The rationale behind this comes from the the experience of having worked for a few months with the ROS 2 tracing infrastructure. To me, it eventually become rather cumbersome to have to maintain a single tracetools repo with all tracepoints (I experienced this directly with an extension of tracetools_acceleration we keep internally). Altogether this led me to realize that it might be better to simply add a tracetools_<whatever-ros2-stack/package>.

Happy to see people extending/building on top of this @vmayoral! 😃

This looks good to me. For reference, a similar rationale wrt the ROS 2 core itself was brought up a few months ago here: ros2/rclcpp#1738 (comment). If these were tracepoints for the ROS 2 core (i.e., currently rclcpp* and below), I'd prefer to avoid forking tracetools/putting the tracepoints in other packages to avoid the usual fragmentation of open source code, but this isn't really the case. It's more of an extension, i.e., additional instrumentation for application-level layers above the core, so I think this is the way to go for these use-cases. Besides, there is nothing about LTTng that requires instrumentation (or, rather, tracepoints) to be centralized like it is currently for the ROS 2 core, and the interactions with the Trace action are not really affected by this choice.

I'm not entirely sure whether or not these "instrumentation extension" packages should use the same TRACETOOLS_* CMake options, but I think it does make sense to use the same option name for all of them and have globals options. Besides, a package-specific option can always be added later on.

@vmayoral
Copy link
Contributor Author

Happy new year folks!

I'm bumping this ticket so that it doesn't get stalled. @vrabaud and @JWhitleyWork, you guys are listed as maintainers, @jacobperron you seem to have contributed recently as well, anything else needed from our side on this PR? With it, we should be able to add additional probes.

Copy link
Contributor

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Looks okay to me. I didn't test it, but I think @JWhitleyWork should give the final approval.

tracetools_image_pipeline/CHANGELOG.rst Outdated Show resolved Hide resolved
tracetools_image_pipeline/package.xml Outdated Show resolved Hide resolved
image_proc/CMakeLists.txt Outdated Show resolved Hide resolved
@JWhitleyWork
Copy link
Collaborator

Please address the 3 changes above (mine and @jacobperron's) and then I'm good to merge.

See ros-perception#717 (comment)

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
tracetools_image_pipeline included in package.xml and
fetched by ament_auto_find_build_dependencies().

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
@vmayoral
Copy link
Contributor Author

All done from my side @jacobperron and @JWhitleyWork, back to you.

vmayoral added a commit to ros-acceleration/image_pipeline that referenced this pull request Jan 11, 2022
See ros-perception#717 (comment)

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
@vmayoral
Copy link
Contributor Author

Friendly ping @JWhitleyWork

@JWhitleyWork JWhitleyWork merged commit 1b6f868 into ros-perception:ros2 Jan 24, 2022
JWhitleyWork pushed a commit that referenced this pull request Jan 24, 2022
See #717 (comment)

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
knzo25 added a commit to tier4/image_pipeline that referenced this pull request Mar 4, 2022
* Update camera_calibration setup.cfg to use underscores (ros-perception#688)

Fixes a deprecation warning.

* trimmed whitespace at line endings

* implemented fisheye mono and stereo calibration based on the melodic branch

* rebase change

* fixes to pass tests

* update pytest.ini

* added missing imports

* revert back

* Add retry video capture feature with timeout

Retry onInit() if the loading image fails or if the image is empty.
This is useful if the stream is lost for a while or if the stream
is not ready in the beginning.

* Warning instead of fatal error when frames are differents

Signed-off-by: Francisco Martin Rico <fmrico@gmail.com>

* Fix tiny error in comment

Signed-off-by: Francisco Martin Rico <fmrico@gmail.com>

* Use RCLCPP_WARN_THROTTLE (10 secs) to avoid terminal spam

Signed-off-by: Francisco Martín Rico <fmrico@gmail.com>

* add xyzrgb radial node

* use unique_ptrs, remove unused code, add back in missing initMatrix call

Signed-off-by: Evan Flynn <evan.flynn@apex.ai>

* bring over ros1 fix for missing roi resize

Signed-off-by: Evan Flynn <evan.flynn@apex.ai>

* Add tracetools_image_pipeline package to the pipeline

Add an LTTng tracing provider wrapper for image_pipeline
metapackage. Include tracepoints for two initial ROS 2
components: image_proc::RectifyNode and ResizeNode

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* LTTng instrument image_proc::RectifyNode and image_proc::ResizeNode

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* Deal with uncrustify and cpplint

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* Omit changelogfile

See ros-perception#717 (comment)

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* tracetools_image_pipeline version consistent with repo

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* Remove unnecessary find_package

tracetools_image_pipeline included in package.xml and
fetched by ament_auto_find_build_dependencies().

Signed-off-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>

* Fixed the occupation percetnage drawings
Fixed the deadlock caused by using GUI calls in different threads by moving all those calls to the main thread
The calibrator was missing the major part of the frames and the window did not get updated due to the threading model. Test indicates that just by properly changing the model to a classic consumer/producer solves the particular issue without any side effects

* Fixed the 'proper' camera calibration part.
This time using the bag recorded on the 01/31, the results seem coherent, but have not been tested

* Changed the locks by deques to match the original code in style.
These structures are thread-safe so they replace the manual locks

* Fixed typo in pointcloudxyz launch file

* Fixed the code from the upstream (this branch also has tieriv code)

* Fixed import error during merge

Co-authored-by: jaiveersinghNV <84546269+jaiveersinghNV@users.noreply.github.com>
Co-authored-by: Gabor Soros <gabor.soros@nokia-bell-labs.com>
Co-authored-by: Patrick Musau <pmusau13ster@gmail.com>
Co-authored-by: Ashwin Sushil <ashwinsushil@yahoo.com>
Co-authored-by: Francisco Martin Rico <fmrico@gmail.com>
Co-authored-by: Evan Flynn <evanflynn.msu@gmail.com>
Co-authored-by: Evan Flynn <evan.flynn@apex.ai>
Co-authored-by: Víctor Mayoral Vilches <v.mayoralv@gmail.com>
Co-authored-by: Harshal Deshpande <hardesh1deshpande@gmail.com>
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