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

[tf2] Output canTransform msg only on state change #491

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

aprotyas
Copy link
Member

The "frame does not exist" error message in BufferCore::validateFrameId
is noisy and not warranted if a frame never existed to begin with, so
that message is only outputted once a state change has occurred - i.e.
any frame was found.

Fixes #358.

Signed-off-by: Abrar Rahman Protyasha [email protected]

@clalancette clalancette self-assigned this Jan 13, 2022
@audrow audrow changed the base branch from ros2 to rolling June 28, 2022 14:18
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Seems fine to me - sorry for that noisy output

@aprotyas
Copy link
Member Author

aprotyas commented Dec 6, 2022

It's been a while since I've done this... can anyone run CI please?

@emersonknapp
Copy link
Contributor

Gist: https://gist.githubusercontent.com/emersonknapp/db372988ab86273a8125101ee006afb5/raw/e4913ecebdf892a47f59a89cb0cafa3265432f5a/ros2.repos
BUILD args: --packages-above-and-dependencies tf2
TEST args: --packages-above tf2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11230

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Separately from the static scoping issue. I don't think that this is a good heuristic to add.

If you make the assumption that the frame will become available it will reduce the number of warnings on startup when you're querying before the buffer has been filled. However this will also potentially suppress all warnings for users who fail to create transform publishers and they will potentially sit with a running system without warnings, yet it cannot run correctly so they should be getting warnings.

An advanced user can choose to suppress the warnings in various ways. But we should not make it silently fail to warn the user for an warning message that they asked for (by passing in the error_msg non-null pointer.

This can lead to very unanticipated behaviors such as the system is silent for some period of time and suddenly it starts publishing warnings when a tf publisher comes online because it is publishing unrelated content. Causing the user to believe that adding the publisher caused the warnings, instead of knowing that this has been an issue all along, and continues to be an issue.

@@ -113,8 +113,13 @@ CompactFrameID BufferCore::validateFrameId(
return 0;
}

// Only check that a frame is non-existent once a frame was found
static bool init_state = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

A static variable here is not correct. It will not correctly interact with potentially multiple BufferCore instances and doesn't deal with resetting the core.

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.

Every call to canTransform implementation throws Warning message print until transform is received.
4 participants