Suppress spam from calling canTransform by gonzodepedro · Pull Request #529 · ros2/geometry2

gonzodepedro · 2022-06-01T18:56:46Z

Based on #502

Closes #501

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

jacobperron · 2022-06-01T20:27:41Z

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);

The user has few options to workaround potential console spam from canTransform. If we want, it would be better to update the API to optional return the error string. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2022-06-20T15:44:52Z

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 · 2022-06-23T13:32:24Z

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

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.
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 · 2022-06-23T13:36:12Z

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 · 2022-08-03T17:13:57Z

@clalancette friendly ping 🙂

clalancette · 2022-08-19T15:00:28Z

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 · 2022-08-20T17:19:41Z

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

vincentrou · 2022-10-17T09:36:53Z

@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

* suppress spam in canTransform * Uncrustify Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Suppress console spam in other calls to canTransform The user has few options to workaround potential console spam from canTransform. If we want, it would be better to update the API to optional return the error string. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> Signed-off-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: Grigoriy Lipenko <gnlipenko@sberautotech.ru> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

leander2189 · 2023-07-21T10:16:46Z

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

clalancette · 2023-07-24T14:19:46Z

@Mergifyio backport humble

mergify · 2023-07-24T14:19:53Z

backport humble

✅ Backports have been created

Details

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

* suppress spam in canTransform * Uncrustify Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Suppress console spam in other calls to canTransform The user has few options to workaround potential console spam from canTransform. If we want, it would be better to update the API to optional return the error string. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> Signed-off-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: Grigoriy Lipenko <gnlipenko@sberautotech.ru> Co-authored-by: Jacob Perron <jacob@openrobotics.org> (cherry picked from commit 8c2aa5a)

* suppress spam in canTransform * Uncrustify Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> * Suppress console spam in other calls to canTransform The user has few options to workaround potential console spam from canTransform. If we want, it would be better to update the API to optional return the error string. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar> Signed-off-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: Grigoriy Lipenko <gnlipenko@sberautotech.ru> Co-authored-by: Jacob Perron <jacob@openrobotics.org> (cherry picked from commit 8c2aa5a) Co-authored-by: Gonzo <42421541+gonzodepedro@users.noreply.github.com>

Grigoriy Lipenko and others added 2 commits June 1, 2022 15:40

suppress spam in canTransform

3946b6e

Uncrustify

b35dc1a

Signed-off-by: Gonzalo de Pedro <gonzalo@depedro.com.ar>

clalancette self-requested a review June 16, 2022 17:05

clalancette assigned jacobperron Jun 16, 2022

Suppress console spam in other calls to canTransform

00fa842

The user has few options to workaround potential console spam from canTransform. If we want, it would be better to update the API to optional return the error string. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron changed the title ~~Grishalipenko/ros2~~ Suppress spam from calling canTransform Jun 20, 2022

clalancette reviewed Jun 20, 2022

View reviewed changes

audrow changed the base branch from ros2 to rolling June 28, 2022 14:18

jacobperron requested a review from clalancette July 13, 2022 21:55

clalancette approved these changes Aug 19, 2022

View reviewed changes

jacobperron merged commit 8c2aa5a into rolling Aug 23, 2022

delete-merged-branch bot deleted the grishalipenko/ros2 branch August 23, 2022 17:50

jacobperron mentioned this pull request Aug 23, 2022

suppress spam in canTransform #502

Closed

clalancette mentioned this pull request Oct 19, 2022

Make warn message debug so the spam will stop #556

Closed

mergify bot mentioned this pull request Jul 24, 2023

Suppress spam from calling canTransform (backport #529) #615

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress spam from calling canTransform#529

Suppress spam from calling canTransform#529
jacobperron merged 3 commits intorollingfrom
grishalipenko/ros2

gonzodepedro commented Jun 1, 2022 •

edited by jacobperron

Loading

Uh oh!

jacobperron commented Jun 1, 2022

Uh oh!

jacobperron commented Jun 20, 2022

Uh oh!

clalancette left a comment

Uh oh!

jacobperron commented Jun 23, 2022 •

edited

Loading

Uh oh!

jacobperron commented Jun 23, 2022 •

edited

Loading

Uh oh!

jacobperron commented Aug 3, 2022

Uh oh!

clalancette commented Aug 19, 2022

Uh oh!

jacobperron commented Aug 20, 2022 •

edited

Loading

Uh oh!

vincentrou commented Oct 17, 2022

Uh oh!

leander2189 commented Jul 21, 2023

Uh oh!

clalancette commented Jul 24, 2023

Uh oh!

mergify bot commented Jul 24, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

gonzodepedro commented Jun 1, 2022 • edited by jacobperron Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron commented Jun 1, 2022

Uh oh!

jacobperron commented Jun 20, 2022

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

jacobperron commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobperron commented Aug 3, 2022

Uh oh!

clalancette commented Aug 19, 2022

Uh oh!

jacobperron commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentrou commented Oct 17, 2022

Uh oh!

leander2189 commented Jul 21, 2023

Uh oh!

clalancette commented Jul 24, 2023

Uh oh!

mergify bot commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gonzodepedro commented Jun 1, 2022 •

edited by jacobperron

Loading

jacobperron commented Jun 23, 2022 •

edited

Loading

jacobperron commented Jun 23, 2022 •

edited

Loading

jacobperron commented Aug 20, 2022 •

edited

Loading

mergify bot commented Jul 24, 2023 •

edited

Loading