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

image_publisher_node not read camera_info_url on initialization #965

Closed
Kotochleb opened this issue Apr 17, 2024 · 3 comments
Closed

image_publisher_node not read camera_info_url on initialization #965

Kotochleb opened this issue Apr 17, 2024 · 3 comments

Comments

@Kotochleb
Copy link
Contributor

Kotochleb commented Apr 17, 2024

As in the title, image_publisher_node doesn't account for camera_info_url on the initialization when launched as a standalone node on Humble. For some reason, it skips reading it. When the same parameter is changed dynamically after the node started, everything works just fine.
The simplest walk-round I found was to change return result with break in the pram_change_callback like so:

if (parameter.get_name() == "filename") {
    filename_ = parameter.as_string();
    RCLCPP_INFO(get_logger(), "Reset filename as '%s'", filename_.c_str());
    ImagePublisher::onInit();
-   return result;
+   break;
    } else if (parameter.get_name() == "flip_horizontal") {
    flip_horizontal_ = parameter.as_bool();
    RCLCPP_INFO(get_logger(), "Reset flip_horizontal as '%d'", flip_horizontal_);
    ImagePublisher::onInit();
-   return result;
+   break;
  } else if (parameter.get_name() == "flip_vertical") {
    flip_vertical_ = parameter.as_bool();
    RCLCPP_INFO(get_logger(), "Reset flip_vertical as '%d'", flip_vertical_);
    ImagePublisher::onInit();
-   return result;
+   break;

Yet, I feel like there should be a better solution to this, and I do not feel confident enough with the original implementation to submit a good enough bugfix PR.

@mikeferguson
Copy link
Member

I'm not sure why there is a "return result" in any of those cases - I would think we should iterate through all the parameter settings by completing the for loop (and then there is a return after the reconfigure callback is called). I think we should actually remove the lines entirely (not change to a break).

@Kotochleb
Copy link
Contributor Author

I'm not sure why there is a "return result" in any of those cases

@mikeferguson this is precisely what caused my confusion and said lack of said confidence in making changes. If you think removing the return entirely is a fine fix I can submit PR soon.

ahcorde pushed a commit that referenced this issue May 29, 2024
…on startup (#983)

As described in
#965 camera info
is not loaded from the file on node initialization, but only when the
parameter is reloaded.

This PR resolves this issue and should be straightforward to port it to
`Humble`, `Iron` and `Jazzy`.
mergify bot pushed a commit that referenced this issue May 29, 2024
…on startup (#983)

As described in
#965 camera info
is not loaded from the file on node initialization, but only when the
parameter is reloaded.

This PR resolves this issue and should be straightforward to port it to
`Humble`, `Iron` and `Jazzy`.

(cherry picked from commit 847920b)

# Conflicts:
#	image_publisher/src/image_publisher.cpp
mergify bot pushed a commit that referenced this issue May 29, 2024
…on startup (#983)

As described in
#965 camera info
is not loaded from the file on node initialization, but only when the
parameter is reloaded.

This PR resolves this issue and should be straightforward to port it to
`Humble`, `Iron` and `Jazzy`.

(cherry picked from commit 847920b)

# Conflicts:
#	image_publisher/src/image_publisher.cpp
mergify bot pushed a commit that referenced this issue May 29, 2024
…on startup (#983)

As described in
#965 camera info
is not loaded from the file on node initialization, but only when the
parameter is reloaded.

This PR resolves this issue and should be straightforward to port it to
`Humble`, `Iron` and `Jazzy`.

(cherry picked from commit 847920b)

# Conflicts:
#	image_publisher/src/image_publisher.cpp
@mikeferguson
Copy link
Member

Looks like fix is merged to Rolling, backports in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants