Add filtering capability to ft_broadcaster

[OscarMrZ]

Good morning!

This is a continuation of #559 as it was inactive.

The changes are minimal:

1. Applied @saikishor suggestions

• Added sensor_filter_chain to the parameters, which serves as documentation.
• I removed all the logic about the additional frames. I agree with @saikishor that transforming to different frames should be handled by the user in their application. With this, no tf calls inside the RT are needed.
• No more publishing NaN if the update of the filter failed.

2. Added some relevant tests
   IMHO, the previous tests were testing the business logic of the filters themselves, that is, checking that the actual computations were correct, rather than the actual new logic for adding filters. I added three tests for configuring, activating/deactivating, and updating with a configured filter chain. Just one consideration: I changed a little bit the logic of SetUpFTSBroadcaster a little bit to accept a node name. This is because I added some parameters in the YAML that are needed for the broadcaster with filters. I cannot do set_parameter because the filter chain expects the parameters to be overrides.

Thanks a lot for your time!

Óscar

[OscarMrZ]

Also, an additional consideration about the logic:

Because of ros/filters#89, there's not really a way to tell if a chain is actually configured. So now, even if the user doesn't provide a chain config, it will be configured with an empty filter list. At the end, this just means that the filtered message will always be published, with output = input. If ros/filters#90 gets merged, the broadcaster will know no filter chain was configured and omit publishing the unnecessary message.

[christophfroehlich]

Thanks for picking this up.

This is a continuation of #559 as it was inactive.

It was not inactive, but it was missing more reviews ;)

2. Added some relevant tests
   IMHO, the previous tests were testing the business logic of the filters themselves, that is, checking that the actual computations were correct, rather than the actual new logic for adding filters. I added three tests for configuring, activating/deactivating, and updating with a configured filter chain. Just one consideration: I changed a little bit the logic of SetUpFTSBroadcaster a little bit to accept a node name. This is because I added some parameters in the YAML that are needed for the broadcaster with filters. I cannot do `set_parameter` because the filter chain expects the parameters to be overrides.

But now there is no test if any filtering is performed, or not? E.g., not calling the filter-update will not be covered by the tests?

Edit:
Please install pre-commit and fix the failing tests..

[codecov]

Codecov Report

❌ Patch coverage is 93.26923% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.23%. Comparing base (dc2ee70) to head (888a52f).

Files with missing lines	Patch %	Lines
...roadcaster/src/force_torque_sensor_broadcaster.cpp	87.17%	3 Missing and 2 partials ⚠️
...ce_torque_sensor_broadcaster/test/dummy_filter.cpp	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1814   +/-   ##
=======================================
  Coverage   85.23%   85.23%           
=======================================
  Files         142      143    +1     
  Lines       13734    13794   +60     
  Branches     1190     1194    +4     
=======================================
+ Hits        11706    11758   +52     
- Misses       1632     1636    +4     
- Partials      396      400    +4     

Flag	Coverage Δ
unittests	85.23% <93.26%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ster/test/test_force_torque_sensor_broadcaster.cpp	98.46% <100.00%> (+0.30%)	⬆️
...ce_torque_sensor_broadcaster/test/dummy_filter.cpp	80.00% <80.00%> (ø)
...roadcaster/src/force_torque_sensor_broadcaster.cpp	89.78% <87.17%> (-2.72%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[OscarMrZ]

Hello @christophfroehlich

Thanks for your comment!

True, my bad! I hope it's ok opening this new MR.

I had actually run it with pre-commit, but somehow pushed with some rules failing. It should be fixed now. Sorry for that!

But now there is no test if any filtering is performed, or not? E.g., not calling the filter-update will not be covered by the tests?

Yeah, you are right. I didn't know how to properly add it without including unnecessary logic checks for the underlying filters. Would you prefer re-adding a test with one of the simple filters or creating a dummy filter for testing?

P.D.: Sorry for the force push!

[christophfroehlich]

creating a dummy filter would be ok, but using an existing one is also fine as we have control_toolbox as dependency in this repo (but not in this package, right? this would speak for the dummy or one from ros/filters directly)

[saikishor]

creating a dummy filter would be ok, but using an existing one is also fine as we have control_toolbox as dependency in this repo (but not in this package, right? this would speak for the dummy or one from ros/filters directly)

Sounds good to me.

[OscarMrZ]

Good morning!

Yeah, there's no dependency for control_toolbox in the ft_broadcaster, but I think it's perfectly fine to use a filter from filters as we are introducing the dependency for the chain.

I added dummy_filter.cpp, which performs the template specialization for the WrenchStamped type of the increment filter update method. It will just add 1.0 to every field of the message.

I updated the tests accordingly and included an additional one to ensure the correct message is received. Everything seems to be working fine!

Thanks a lot!

[christophfroehlich]

This looks quite good already. Can you please update

• https://github.com/ros-controls/ros2_controllers/blob/master/force_torque_sensor_broadcaster/doc/userdoc.rst
• https://github.com/ros-controls/ros2_controllers/blob/master/doc/release_notes.rst

then this is good to be merged.

[christophfroehlich]

how to proceed here?

[OscarMrZ]

I would say until the new version of filters with my MR gets synced into Humble, it has to stay like it is. After that, we can check the length of the chain and act accordingly.

[christophfroehlich]

filters 2.2.2 is released, and only the sync to jazzy is missing. So we can update this PR already, and just hold an eventual backport to jazzy until jazzy got synced.

[saikishor]

Good job guys

[christophfroehlich]

Shouldn't we use get_length() now?

[OscarMrZ]

Good morning @christophfroehlich!

Sorry for the delay, things get slower in summer! 😄

Everything should be ready for merging. Please let me know if you think further changes may be needed.

Thanks a lot for your assistance!