Suppress spam from calling canTransform

[gonzodepedro]

Based on #502

Closes #501

[jacobperron]

Do you mind also updating these other calls (from my other comment #502 (comment)):

geometry2/tf2_ros/src/buffer.cpp

Line 94 in ef2dd14

canTransform(target_frame, source_frame, lookup_time, timeout);

geometry2/tf2_ros/src/buffer.cpp

Line 117 in ef2dd14

canTransform(target_frame, target_time, source_frame, source_time, fixed_frame, timeout);

[jacobperron]

• Linux
• Linux-aarch64
• Windows

[clalancette]

Not to derail this, but I will point out that suppressing these error messages unconditionally basically constitutes a large behavior change in tf2_ros from previous versions. It also means that we won't get notified of real errors in the tf2 tree (like loops).

I'm not quite sure what we want to do here. Maybe it would be a better idea to rate-limit this instead of suppressing it completely, as we are going to be losing a lot of debugging information this way.

[jacobperron]

The code is a bit tangled, but I think these are the two behavior changes caused by this PR:

1. Users calling tf2_ros::Buffer::canTransform with a timeout and errstr argument (that is not nullptr) now do not see a wall full of console warnings (3946b6e). Instead, errstr is set to any warning.
2. Users calling t2_ros::Buffer::lookupTransform with a timeout argument now do not see a wall of console warnings (from the internal canTransform() invocation) (00fa842).

Correct me if I'm wrong, but I don't think (1) is disagreeable.

Regarding (2), the code is a bit hard to follow, but in both the previous and current behavior an exception is thrown from lookupTransform if there is an issue validating frames:

geometry2/tf2/src/buffer_core.cpp

Lines 124 to 145 in 51a7f24

	CompactFrameID BufferCore::validateFrameId(
	const char * function_name_arg,
	const std::string & frame_id) const
	{
	if (frame_id.empty()) {
	std::string error_msg = "Invalid argument \"" + frame_id + "\" passed to " + function_name_arg +
	" - in tf2 frame_ids cannot be empty";
	throw tf2::InvalidArgumentException(error_msg.c_str());
	}
	if (startsWithSlash(frame_id)) {
	std::string error_msg = "Invalid argument \"" + frame_id + "\" passed to " + function_name_arg +
	" - in tf2 frame_ids cannot start with a '/'";
	throw tf2::InvalidArgumentException(error_msg.c_str());
	}
	CompactFrameID id = lookupFrameNumber(frame_id);
	if (id == 0) {
	std::string error_msg = "\"" + frame_id + "\" passed to " + function_name_arg +
	" does not exist. ";
	throw tf2::LookupException(error_msg.c_str());
	}

called from lookupTransform's implementation:

geometry2/tf2/src/buffer_core.cpp

Lines 654 to 655 in 51a7f24

	CompactFrameID target_id = validateFrameId("lookupTransform argument target_frame", target_frame);
	CompactFrameID source_id = validateFrameId("lookupTransform argument source_frame", source_frame);

The only difference this PR introduces is that we don't get the (more or less) same message repeated to the console before the exception ultimately occurs. AFAICT, we're only calling canTransform inside lookupTransform as a lazy way to implement the timeout. Both canTransform and lookupTransform do frame ID validation, but the former prints warnings to console and the latter throws an exception.

I agree that the underlying issue of tf2::canTransform printing to console at an unholy rate should probably be fixed.
However, IMO, it doesn't make sense to print warnings to console while we're waiting for a transform to become available with lookupTransform, especially since we're already raising an exception with the same message if there's still a problem when the timeout is reached.

[jacobperron]

If I'm not mistaken, this change doesn't hide errors in the TF tree since lookupTransform still throws an exception with the same error message that was previously printed to console.

[jacobperron]

@clalancette friendly ping 🙂

[clalancette]

Correct me if I'm wrong, but I don't think (1) is disagreeable.

Agreed, this is desirable. The part of the patch that changes canTransform looks good.

Regarding (2), the code is a bit hard to follow, but in both the previous and current behavior an exception is thrown from lookupTransform if there is an issue validating frames:

And now looking at it in more detail, I agree with you. canTransform is basically being reused as a way to "wait" for the transform to come in, and as such should not print any warnings. Once that expires and we get to the lookupTransform API (which actually is inherited from the underlying tf2::BufferCore class), then if that transform still doesn't exist we'll throw an exception. That seems like sufficient warning to the user.

So with all of that said, I'm going to approve this. Thanks for the detailed explanation on what is going on here.

[jacobperron]

• Linux
• Linux-aarch64
• Windows (unrelated cmake warning from osrf_testing_tools_cpp)

[vincentrou]

@jacobperron
Thanks for merging this PR.
Is it possible to backport this to humble ?
Since it is the last LTS and there is a lot of related issues : #358

[leander2189]

Was this feature ever ported to humble? I am getting lots of messages in my tests, they make debugging a lot more difficult

[clalancette]

@Mergifyio backport humble

[mergify]

backport humble

✅ Backports have been created

• #615 Suppress spam from calling canTransform (backport #529) has been created for branch humble