Fixing Hardware components logging spam in tests

[soham2560]

Brief

In this PR

• Used new RM constructors in tests

This Fixes #2662 partly (details at #2692 (comment))

[soham2560]

This Fixes #2662 partly

• It handles the Executor being passed to RM, so no more of

Executor is not available....

But I was questioning should we also take care of

`status_publish_rate` is set to 0.0, hardware status publisher will not be created.

As that message is not printed when we actually start up the publishers, do we want that to happen for every test?

[christophfroehlich]

`status_publish_rate` is set to 0.0, hardware status publisher will not be created.

As that message is not printed when we actually start up the publishers, do we want that to happen for every test?

Do we have a test in actually checking the status_publish_rate behavior?
maybe we should activate it per default, so that the tests are running with everything activated to see if there is any issue. But not sure what could be a possible issue :D

[saikishor]

status_publish_rate is set to 0.0, hardware status publisher will not be created.

We can remove the print and simply document it that unless a status_publish_rate is set to a valid value, nothing gets published

[soham2560]

status_publish_rate is set to 0.0, hardware status publisher will not be created.

We can remove the print and simply document it that unless a status_publish_rate is set to a valid value, nothing gets published

this also works, it is already documented at

ros2_control/hardware_interface/doc/writing_new_hardware_component.rst

Lines 215 to 225 in 6cbb5f0

	* **Enable in URDF**: Finally, to activate the publisher, add the ``status_publish_rate`` parameter to your ``<hardware>`` tag in the URDF. Setting it to 0.0 disables the feature.
	.. code-block:: xml
	<ros2_control name="MyHardware" type="system">
	<hardware>
	<plugin>my_package/MyHardware</plugin>
	<param name="status_publish_rate">20.0</param>
	</hardware>
	...
	</ros2_control>

, so ill just remove it for now, let me know if that works @christophfroehlich

[christophfroehlich]

shouldn't we avoid comparison with zero?

Suggested change

	if (publish_rate != 0.0)
	if (publish_rate < std::numeric_limits<T>::epsilon())

[soham2560]

was also considering not going down that route but since it is zero specifically (either assigned manually by us when user hasnt provided a value, or direct from config, so should not have any floating point issues) it seemed fine. but its more standard to go by the route you suggested. maybe ill add an fabs on publish rate too

[soham2560]

actually here particularly it might be better to just do

if (publish_rate > 0.0)

[christophfroehlich]

ah, the "lower" comparison was wrong anyways. And a negative value would also be valid parameter, right?

[saikishor]

Does it have to be a floating point? Can't it be an integer?

[soham2560]

also true, we can just move to integer

ah, the "lower" comparison was wrong anyways. And a negative value would also be valid parameter, right?

yes, even for integers, will just add a seperate condition for that

[christophfroehlich]

Don't you think that users might want to configure sub-Hz rate? there is no harm in using a double here, right?

[christophfroehlich]

I mean 0.1Hz for example.

[soham2560]

thats also true, I convinced myself by saying who would want the choice between 50hz and 50.5hz :), but yes the 0-1hz might be in need of granuality.

no strong opinion here, which one should I go for?

[saikishor]

Let's go with the current double approach

[codecov]

Codecov Report

❌ Patch coverage is 97.05882% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.48%. Comparing base (a2e56e4) to head (63e2902).

Files with missing lines	Patch %	Lines
...dware_interface/test/test_component_interfaces.cpp	98.27%	0 Missing and 1 partial ⚠️
...e/test/test_component_interfaces_custom_export.cpp	90.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2692      +/-   ##
==========================================
+ Coverage   89.46%   89.48%   +0.02%     
==========================================
  Files         152      152              
  Lines       17307    17344      +37     
  Branches     1434     1436       +2     
==========================================
+ Hits        15483    15521      +38     
+ Misses       1246     1244       -2     
- Partials      578      579       +1     

Flag	Coverage Δ
unittests	89.48% <97.05%> (+0.02%)	⬆️

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

Files with missing lines	Coverage Δ
...dware_interface/test/test_component_interfaces.cpp	97.84% <98.27%> (-0.05%)	⬇️
...e/test/test_component_interfaces_custom_export.cpp	91.91% <90.00%> (-0.34%)	⬇️

... 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.

[saikishor]

@soham2560 is this ready for review?

[mergify]

This pull request is in conflict. Could you fix it @soham2560?

[soham2560]

@soham2560 is this ready for review?

having trouble with some tests, will let you know once done