ROS2: Port image_view

[JWhitleyWork]

Port is far from complete but I wanted to create a WIP PR to let others know that it is being worked on.

[SteveMacenski]

I'm sure as you've seen in the github queue, there are an awful lot of tickets complaining about image viewer not letting them save, freezing, etc. I don't want to make you deal with these in the port, but if you happen to be testing by running stuff and you do fix them, that would be great

[JWhitleyWork]

I'll see what I can do and back-port anything I find.

[SteveMacenski]

LMK when you want me to take a look

[JWhitleyWork]

@SteveMacenski - Could you possibly take a look at the build failure that's happening right now? I know there's something happening in the linking phase but I'm not sure why it is failing.

[SteveMacenski]

@JWhitleyAStuff try fixing

/root/project/src/image_pipeline/image_view/src/disparity_view_node.cpp: In constructor ‘image_view::DisparityViewNode::DisparityViewNode(const rclcpp::NodeOptions&)’:

/root/project/src/image_pipeline/image_view/src/disparity_view_node.cpp:69:8: warning: unused variable ‘autosize’ [-Wunused-variable]

   bool autosize = this->declare_parameter("autosize", false);

First, then I'll take a look. CI sometimes has a habit of not failing well so if you had warnings or something odd, downstream weird things happen.

if its still happening, ping me again

[JWhitleyWork]

@JWhitleyAStuff try fixing

/root/project/src/image_pipeline/image_view/src/disparity_view_node.cpp: In constructor ‘image_view::DisparityViewNode::DisparityViewNode(const rclcpp::NodeOptions&)’:

/root/project/src/image_pipeline/image_view/src/disparity_view_node.cpp:69:8: warning: unused variable ‘autosize’ [-Wunused-variable]

   bool autosize = this->declare_parameter("autosize", false);

First, then I'll take a look. CI sometimes has a habit of not failing well so if you had warnings or something odd, downstream weird things happen.

if its still happening, ping me again

I fixed this warning and I'm still getting the same two linker errors:

libimage_view_nodes.so: undefined reference to vtable for image_view::DisparityViewNode'`

and

libimage_view_nodes.so: undefined reference to vtable for image_view::DisparityViewNode'`

[JWhitleyWork]

@SteveMacenski - I figured it out. I had a function declared in disparity_view_node.hpp that was not declared in disparity_view_node.cpp. It's been fixed. Now I just need to test it and fix all the stuff that's broken!

[SteveMacenski]

Ah ok!

Errors are usually meaningful, but when I see them in CI and not locally my logic-sensor turns off and my wild-speculation-sensor turns on

[SteveMacenski]

No real substantive issues

[JWhitleyWork]

@SteveMacenski - Thanks for the feedback. I'm still cleaning up a lot of stuff so it's not quite ready for a review just yet. I'll hit you back when it's working and I've linted everything. Thanks!

[JWhitleyWork]

@SteveMacenski - OK, I think I got the majority of it working. image_view works as expected now. Definitely found some things that need to be cleaned up in melodic. Anyway, would you mind taking a look over the code now? Please ignore formatting for the moment - I'll add linting and static analysis in a separate MR since it'll be much harder to review with all of those formatting changes.

I tested image_view, and image_saver. The rest, I don't have the hardware to test.

Also, FYI, I used this driver and, other than having a really low default FPS, it works fine: https://github.com/klintan/ros2_usb_camera

[SteveMacenski]

I saw when i was cleaning up the filesystem someone added a usb capture to the publisher node. Maybe that should be pulled out again and in a new executable (like that)

[SteveMacenski]

what do you still need boost for?

[SteveMacenski]

LGTM. Any of the tickets you fixed, can you comment on their tickets saying resolved in ROS2? Keep track of the ROS2-needs

[JWhitleyWork]

what do you still need boost for?

https://stackoverflow.com/questions/23412978/c11-equivalent-to-boost-format

Essentially, there is no Standard Library, direct replacement for boost::Format. We could replace it with std::sprintf or an ostringstream but we'd have to change the format of the input parameter to match those methods or parse the meaning ourselves (since the user can input whatever they want for the "format" parameter). How do you want to handle this?

[SteveMacenski]

That's OK for me. Just curious.

Are you good for a review now?

[JWhitleyWork]

@SteveMacenski - Yeah, code should be ready for review, minus formatting (will be my next PR). When you say "That's OK for me", do you mean keeping Boost or using one of the other formatting methods?

[SteveMacenski]

Keeping Boost. No need to make you do everything all at once ;-) If you're so inclined to do something non-boost, power to you, but I'm not pushing it.

[SteveMacenski]

Small things - mostly good

[SteveMacenski]

Is this still a fix need?

[JWhitleyWork]

Instead of trying to "fix" this, I just removed the request_start_end parameter. Having topics or services appear or not appear based on parameters is bad form. I just have both services available all the time and people can choose how to use the node (whether they use the save service or the start and end services).

[SteveMacenski]

So dynamic reconfigure is never coming back. We have dynamic parameters, where RQT reconfigure can just tag new parameter values and you can register parameter callback on change ros-navigation/navigation2#1370

[JWhitleyWork]

Yeah, I forgot about this. I'll work on adding them.

[JWhitleyWork]

Done.

[JWhitleyWork]

@SteveMacenski - Sorry for the late entry. I was going back over your comment about there being several tickets regarding image_view and I realized that #221 reports a very annoying issue, which I finally fixed. I'll fix it in melodic too but there is some clean-up needed first (there is a separate implementation of image_view as a node vs a nodelet).