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

fix: fix parameters and clean up node handles #136

Closed
wants to merge 3 commits into from

Conversation

DerKurt
Copy link

@DerKurt DerKurt commented Aug 10, 2023

Public API Changes
None

Description
Parameters where not usable before.
private_nh isn't needed.

@DerKurt
Copy link
Author

DerKurt commented Aug 21, 2023

See issue #137

@styczen
Copy link

styczen commented May 27, 2024

@rctoris Do we know why this PR is not merged? This fix is quite helpful.

async_web_server_cpp::HttpReply::stock_reply(async_web_server_cpp::HttpReply::not_found))
{
declare_parameter("port", rclcpp::PARAMETER_INTEGER);
declare_parameter("verbose", rclcpp::PARAMETER_BOOL);
declare_parameter("my_doaddressuble_array", rclcpp::PARAMETER_STRING);
Copy link

Choose a reason for hiding this comment

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

Typo

Suggested change
declare_parameter("my_doaddressuble_array", rclcpp::PARAMETER_STRING);
declare_parameter("address", rclcpp::PARAMETER_STRING);

Copy link

Choose a reason for hiding this comment

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

fixedfixed

Copy link

@chameau5050 chameau5050 left a comment

Choose a reason for hiding this comment

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

Good job @DerKurt, do you think you could make the change soon so everybody can use this nice upgrade ? Or maybe @styczen or @rctoris you could make those minor changes. I would do it myself If I had the write permission.

@@ -2,7 +2,7 @@
#define WEB_VIDEO_SERVER_H_

#include <rclcpp/rclcpp.hpp>
#include <cv_bridge/cv_bridge.h>
#include <cv_bridge/cv_bridge.hpp>

Choose a reason for hiding this comment

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

In my case, using humble the cv_bridge.hpp is not found by the compilator

Suggested change
#include <cv_bridge/cv_bridge.hpp>
#include <cv_bridge/cv_bridge.h>

Copy link

Choose a reason for hiding this comment

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

In my case, using humble the cv_bridge.hpp is not found by the compilator

The cv_bridge.h header seems to be deprecated:
#warning This header is obsolete, please include cv_bridge/cv_bridge.hpp instead

Since we are compiling using -werror, this breaks compilation for us.
Is there a compile time switch that we can use to check for the ros2 version?

Choose a reason for hiding this comment

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

Yay, I just notice the root cause of the problem. It is because the humble branch of cv_bridge has not been updated with the new header file. Still, we can fix it here at compile time by detecting ROS distro at compile time by doing something like that.

Suggested change
#include <cv_bridge/cv_bridge.hpp>
#ifdef USE_CV_BRIDGE_H
#include <cv_bridge/cv_bridge.h>
#else
#include <cv_bridge/cv_bridge.hpp>
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been fixed in #149

@@ -1,5 +1,5 @@
#include "web_video_server/image_streamer.h"
#include <cv_bridge/cv_bridge.h>
#include <cv_bridge/cv_bridge.hpp>

Choose a reason for hiding this comment

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

same problem here

Suggested change
#include <cv_bridge/cv_bridge.hpp>
#include <cv_bridge/cv_bridge.h>

Choose a reason for hiding this comment

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

same here

Suggested change
#include <cv_bridge/cv_bridge.hpp>
#ifdef USE_CV_BRIDGE_H
#include <cv_bridge/cv_bridge.h>
#else
#include <cv_bridge/cv_bridge.hpp>
#endif

@@ -117,32 +127,26 @@ WebVideoServer::WebVideoServer(rclcpp::Node::SharedPtr &nh, rclcpp::Node::Shared
}

Choose a reason for hiding this comment

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

This print was removed, but I found it really useful to check that everything is launching correctly

Suggested change
}
RCLCPP_INFO(nh_->get_logger(), "Waiting For connections on %s:%d", address_.c_str(), port_);
}

@DerKurt
Copy link
Author

DerKurt commented Aug 14, 2024

@chameau5050 I don´t have access to the branch anymore, but I notified some people that have and hopefully they can do the changes.

@chameau5050
Copy link

@arneboe also add this code after this line and compile issue should be fix for humble and above :

if("$ENV{ROS_DISTRO}" STREQUAL "humble")
 target_compile_definitions(${PROJECT_NAME} PRIVATE USE_CV_BRIDGE_H)
endif()

@bjsowa
Copy link
Collaborator

bjsowa commented Oct 2, 2024

Parameters have been reworked in #154 and #156

@bjsowa bjsowa closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants