Added a new implementation of the RealtimeBox with added best effort behaviour

[firesurfer]

Hi, I realized the current implementation of the RealtimeBox does not have a "try write/try read" access method. This im important from my point of view. Therefore I implemented a RealtimeBoxBestEffort which per default trys to call try_lock for read and write operations but also implements the previous behaviour which waits until it can aquire a lock.

As already stated in #138 this implementation is not safe to be used with pointers from my point of view. (In the tests I added an example which shows how to use a pointer type with the box but well..for pointers not using this kind of box might be better or we could specialize the template to provide a different behaviour)

Furthermore I took the chance to provide some more modern c++ "magic". For example it is possible to have "emplace behaviour" - basically construct in place or if the underlying type allows it to directly pass an initilizer list.

Note: This implementation requires at least c++17 which is the current default target for ROS2 if I am informed correctly.

Perhaps we could even figure out a way how to replace the current RealTimeBox with an updated interface (#81 says that there are plans to update the API in general)

[bmagyar]

@christophfroehlich here we have a new contender for C++17 support. Do you know how Debian compilation would be affected?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.15%. Comparing base (8995b49) to head (6499036).

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #139       +/-   ##
===========================================
+ Coverage   53.64%   77.15%   +23.51%     
===========================================
  Files          18        7       -11     
  Lines         755      232      -523     
  Branches      375       98      -277     
===========================================
- Hits          405      179      -226     
+ Misses         56       26       -30     
+ Partials      294       27      -267     

Flag	Coverage Δ
unittests	77.15% <92.59%> (+23.51%)	⬆️

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

Files	Coverage Δ
include/realtime_tools/realtime_box_best_effort.h	92.59% <92.59%> (ø)

... and 12 files with indirect coverage changes

[christophfroehlich]

@christophfroehlich here we have a new contender for C++17 support. Do you know how Debian compilation would be affected?

it compiles and the test runs successfully on debian11

[firesurfer]

@christophfroehlich I reworked the PR and you might want to test if it still compiles on older platforms.

1. I restructured the interface. It should now be compatible with the existing RealtimeBox.
2. I added a way to safely use the Box with pointer types. (If you come up with a better way let me know). See the last two tests for an example.
3. For pointer types the normal get/set methods are disabled via std::enable_if as they are not safe to use.

As further steps I would suggest:

1. I would replace the existing RT Box with the new implementation and see if the existing tests work fine with it.
   EDIT: Just did that and seems to work fine!
2. Add some documentation about the usage (Is there any prefered place in the repository to store that?)

[christophfroehlich]

I can't give proper feedback to your implementation, but it still compiles on debian11. And I made a mistake pushing something of your PR to the master (that's why the conflicts now appeared)

btw: I propose that we fix the RHEL image and add maybe another CI for the supported versions of debian to our CIs

[firesurfer]

Then given that it still compiles for you on an older system perhaps @bmagyar might want to review it ?

I am btw not sure why the formatting step fails. I am pretty sure I ran clang-format on all files I touched.

EDIT: Perhaps you might want to note at a prominent place that you have additional requirements for this package when it comes to build platforms. As far as I know the base line for rolling/iron is Ubuntu 22.04 and C++17 (correct me if I am wrong)

[christophfroehlich]

I am btw not sure why the formatting step fails. I am pretty sure I ran clang-format on all files I touched.

The copyright notice is missing in the files, and maybe you have different clang settings. Please install pre-commit in this repo and run it (pre-commit run --all)

EDIT: Perhaps you might want to note at a prominent place that you have additional requirements for this package when it comes to build platforms. As far as I know the base line for rolling/iron is Ubuntu 22.04 and C++17 (correct me if I am wrong)

The mentioned combination was never a problem so far, but gcc on debian11 (which is Tier 3 for humble) has not implemented all of the c++17 standard. This is why std::from_chars is not supported, what caused problems with ros2_control.

[saikishor]

The changes seem fine to me.

I have a comment regarding the style of the methods. Do we follow the ROS standard or the Google style?

[firesurfer]

@saikishor Do you mean the style of the methods themselves or of the comments?
@christophfroehlich Would it be possible for you to resolve the merge conflicts? I am not sure what exactly the best way would be to resolve them.

Is there anything else required to get this merged?

I would additionally add another PR to replace the existing RealtimeBox implementation with this one. I already checked locally on my system if all RTBox Tests also run fine with this implementation.

[saikishor]

@saikishor Do you mean the style of the methods themselves or of the comments?

I meant the methods naming style

[christophfroehlich]

@firesurfer please check if the merge was successful, and why the format job fails again.

[firesurfer]

@christophfroehlich Should be fixed now. The formatter just added a whitespace at the end of some comments...

I think cppcheck can't cope wit the std::enable_if_t at two points which is why the ament_cppcheck fails

 /usr/bin/bash -c source /opt/ros/rolling/setup.sh && ament_cppcheck $(colcon list --packages-select realtime_tools -p) --language=c++
Error: [include/realtime_tools/realtime_box_best_effort.h:141]: (error: missingReturn) Found a exit path from function with non-void return type that has missing return statement
Error: [include/realtime_tools/realtime_box_best_effort.h:170]: (error: missingReturn) Found a exit path from function with non-void return type that has missing return statement

[christophfroehlich]

@christophfroehlich Should be fixed now. The formatter just added a whitespace at the end of some comments...

I think cppcheck can't cope wit the std::enable_if_t at two points which is why the ament_cppcheck fails

 /usr/bin/bash -c source /opt/ros/rolling/setup.sh && ament_cppcheck $(colcon list --packages-select realtime_tools -p) --language=c++
Error: [include/realtime_tools/realtime_box_best_effort.h:141]: (error: missingReturn) Found a exit path from function with non-void return type that has missing return statement
Error: [include/realtime_tools/realtime_box_best_effort.h:170]: (error: missingReturn) Found a exit path from function with non-void return type that has missing return statement

This seems to be fixed and should be part of cppcheck 2.10, but this is not released for Ubuntu jammy. Nothing we can do here?

format job is still complaining:
include/realtime_tools/realtime_box_best_effort.h:204: carefull ==> careful, carefully

[firesurfer]

I fixed the format job now (hopefully). Sorry that this took so long. For some reason I get way more errors when I run pre-commit run --all locally. Especially ament_cpplint complains about quite a lot of formatting but I guess this is disabled in the ci job?

[firesurfer]

@christophfroehlich Apparently now the ament_cpplint fails.

I am a bit puzzled because at least half of the issues there should be fixed by clang-format as they are purely formatting related. What is the best way to fix them without going manually through each of them.

I already tried ament_uncrustify but then the clang-format check from pre-commit fails again.

EDIT:
I fixed it by hand now apart from one issue:

#include <optional>
#include <rcpputils/pointer_traits.hpp>
#include <utility>

When I run pre-commit run --all clang format automatically sorts it into this order. Afterwards ament_cpplint complains about

/workspace/src/realtime_tools/include/realtime_tools/realtime_box_best_effort.h:37: Found C++ system header after other header. Should be: realtime_box_best_effort.h, c system, c++ system, other. [build/include_order] [4]

[firesurfer]

@christophfroehlich All checks seem to pass now. I simply suppressed the two cppcheck warnings regarding the wrong return type.

[firesurfer]

@christophfroehlich So all checks have passed. What is needed to get this merged?

[christophfroehlich]

A review from @bmagyar or @destogl ;)

[firesurfer]

@bmagyar or @destogl would you mind to review the changes?

[destogl]

This looks good! Just a few comments about formatting and style

[firesurfer]

@destogl I tried to resolve your comments wherever possible. Some of the formatting comments are not possible to resolve because clang-format decides to format the comments that way.

[firesurfer]

So anyone interested in clicking on merge ;) As far is can tell there should be no remaining issue?

[firesurfer]

@destogl @christophfroehlich Is there any remaining issue left in order to get this merged?

[saikishor]

Few minor suggestions.

Thank you! :)

[firesurfer]

@saikishor Thanks for your comments. I implemented most of them as appropriate. (E.g. we can not remove the additional function template parameter). The only open point is if we shall rename / remove is_ptr_or_smart_ptr. Just see the conversation above.

[bmagyar]

Only waiting on CI and a thumbs up from @saikishor

[firesurfer]

@saikishor
If constexpr auto is_ptr_or_smart_ptr = rcpputils::is_pointer<T>::value; is the only thing keeping this back from getting merged I can replace it. Would be defining an alias like constexpr auto is_pointer_v = rcpputils::is_pointer<T>::value; ok for you. I think it would keep the code more readable in general.

[saikishor]

@saikishor If constexpr auto is_ptr_or_smart_ptr = rcpputils::is_pointer<T>::value; is the only thing keeping this back from getting merged I can replace it. Would be defining an alias like constexpr auto is_pointer_v = rcpputils::is_pointer<T>::value; ok for you. I think it would keep the code more readable in general.

@firesurfer If others are okay with it, then it is fine for me and can be merged. For me, it is not with the naming is_ptr_or_smart_ptr, if it is the same trait doing the same why redefine/rename it and use it differently instead of using the original rcpputils::is_pointer.

This is just nitpicking, apart from that everything else looks good to me.

[saikishor]

Apart from the nitpicking discussion we are having, the logic and the idea of this PR look good to me.

If it is fine how it is, then I think it is good to be merged

[firesurfer]

So if I understood it correctly we are just waiting for a comment on the is_ptr_or_smart_ptr alias from @christophfroehlich and @destogl ?

[saikishor]

So if I understood it correctly we are just waiting for a comment on the is_ptr_or_smart_ptr alias from @christophfroehlich and @destogl ?

Hello @firesurfer!

As I have mentioned it is just a nitpicking comment, the changes in the PR look good to me. If they are okay with it, it can be merged.

[christophfroehlich]

Thanks for you contribution (and perseverance).