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

AfWindows #14

Closed
wants to merge 6 commits into from
Closed

AfWindows #14

wants to merge 6 commits into from

Conversation

Woojin-Crive
Copy link

@Woojin-Crive Woojin-Crive commented Oct 28, 2023

#13

Avoid AfWindows parameter to work properly.
Raspberry pi 4 b with latest os and rapicam v3

Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this PR does not fix the original problem. It is just ignoring a setting. Additionally, it is not well documented in the commit message what is actually happening and why these changes are required.

Comment on lines +397 to +398
if (id->name() == "AfWindows")
continue;
Copy link
Owner

Choose a reason for hiding this comment

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

You cannot just ignore a parameter. What about other users that want to make use of it? And the commit message does not describe what you change and why.

Comment on lines +167 to +180
auto qos_override_options = rclcpp::QosOverridingOptions({
rclcpp::QosPolicyKind::Depth,
rclcpp::QosPolicyKind::History,
rclcpp::QosPolicyKind::Reliability,
rclcpp::QosPolicyKind::Durability,
});

rclcpp::PublisherOptions pub_options;
pub_options.qos_overriding_options = qos_override_options;
// publisher for raw and compressed image
pub_image = this->create_publisher<sensor_msgs::msg::Image>("~/image_raw", 1);
pub_image = this->create_publisher<sensor_msgs::msg::Image>("~/image_raw", 1, pub_options);
pub_image_compressed =
this->create_publisher<sensor_msgs::msg::CompressedImage>("~/image_raw/compressed", 1);
pub_ci = this->create_publisher<sensor_msgs::msg::CameraInfo>("~/camera_info", 1);
this->create_publisher<sensor_msgs::msg::CompressedImage>("~/image_raw/compressed", 1, pub_options);
pub_ci = this->create_publisher<sensor_msgs::msg::CameraInfo>("~/camera_info", 1, pub_options);
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you changing the default QoS settings? If you need to change the QoS settings, you can also do this via parameters. See qos_overrides in the full list of parameters: ros2 param list.

Comment on lines +63 to +68
# install the launch directory
install(DIRECTORY
launch
DESTINATION share/${PROJECT_NAME}/
)

Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer not to add a launch file in this repo since the node already uses sensible defaults provided by libcamera.

Comment on lines 10 to 11
DeclareLaunchArgument("width", default_value="1280", description="Camera image width"),
DeclareLaunchArgument("height", default_value="720", description="Camera image height"),
Copy link
Owner

Choose a reason for hiding this comment

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

These defaults may not work for every camera.

Comment on lines 13 to 30
# You can add other launch arguments as needed.
# Run the camera_node with the specified parameters.
Node(
package="camera_ros",
executable="camera_node",
name="camera",
output="screen", # Adjust the output as needed.
parameters=[
{"width": LaunchConfiguration("width")},
{"height": LaunchConfiguration("height")},
{"qos_overrides./camera/image_raw.publisher.reliability": "best_effort"},
{"qos_overrides./camera/image_raw/compressed.publisher.reliability": "best_effort"},
{"qos_overrides./camera/camera_info.publisher.reliability": "best_effort"},
],
remappings=[], # Add remappings if necessary.
arguments=["--log-level", LaunchConfiguration("log_level")],
),
# You can add additional nodes or actions here as needed.
Copy link
Owner

Choose a reason for hiding this comment

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

Most of these comments are redundant as they apply to generic launch file configurations, which are already documented in the official documentation.

@Woojin-Crive
Copy link
Author

Oh sorry for the dirty code.
I was just testing for private use.

@christianrauch
Copy link
Owner

You seem to add unrelated commits to this PR. Are you intending to address your issue with the AfWindows control in this PR?

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.

2 participants