OccupancyMapMonitor tests using Dependency Injection #569

tylerjw · 2021-07-23T22:15:28Z

Description

Dependency Injection for OccupancyMapMonitor. More tests to come.

Checklist

Required by CI: Code is auto formatted using clang-format
Create tests, which fail without this PR reference
While waiting for someone to review your request, please help review another open pull request to support the maintainers

codecov · 2021-07-23T22:59:22Z

Codecov Report

Merging #569 (abba127) into main (31470cc) will increase coverage by 0.06%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
+ Coverage   54.22%   54.28%   +0.06%     
==========================================
  Files         190      191       +1     
  Lines       20063    20084      +21     
==========================================
+ Hits        10878    10901      +23     
+ Misses       9185     9183       -2

Impacted Files	Coverage Δ
...clude/moveit/occupancy_map_monitor/occupancy_map.h	`8.34% <ø> (+8.34%)`	⬆️
...veit/occupancy_map_monitor/occupancy_map_updater.h	`0.00% <ø> (ø)`
...or/src/occupancy_map_monitor_middleware_handle.cpp	`0.00% <0.00%> (ø)`
...ccupancy_map_monitor/src/occupancy_map_updater.cpp	`0.00% <ø> (ø)`
...ccupancy_map_monitor/src/occupancy_map_monitor.cpp	`18.90% <46.67%> (+18.90%)`	⬆️
...veit/occupancy_map_monitor/occupancy_map_monitor.h	`25.00% <50.00%> (+25.00%)`	⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	`74.72% <0.00%> (-1.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31470cc...abba127. Read the comment docs.

jliukkonen

My initial review. I think I want to leave another round of comments later when I have time to go through it more thoroughly.

moveit_ros/occupancy_map_monitor/test/occupancy_map_monitor_tests.cpp

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp

jliukkonen · 2021-07-23T23:26:53Z

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp

-
-OccupancyMapMonitor::OccupancyMapMonitor(const rclcpp::Node::SharedPtr& node, double map_resolution)
-  : map_resolution_(map_resolution), debug_info_(false), mesh_handle_count_(0), node_(node), active_(false)
+namespace


Nit: Should this namespace be outside the occupancy_map_monitor namespace for better readability? My personal preference is to have them be outside any namespace to underscore their "anonymousness".

I'm just copying a pattern I've seen in other parts of the MoveIt code base. I would prefer consistency, although I agree that having a separate anonymous namespace seems to be clearer. For consistency sake I want to leave this as is.

https://en.cppreference.com/w/cpp/language/namespace#Unnamed_namespaces

I ... think that the difference might be if someone wrote using namespace occupancy_map_monitor;, then another variable named LOGGER would conflict... I think.
While users shouldn't do that https://abseil.io/tips/153, we might consider not leaving a trap for them if my suspicion is correct.

but I guess all of this is in the cpp so... do they not export?

This is in the cpp so I would assume they wouldn't export

But do they?

Well, as cppreference points out, things in anonymous namespaces can be imported.
In your gtest, I would try to access something in your anon namespace defined in your cpp

cppreference:

Even though names in an unnamed namespace may be declared with external linkage, they are never accessible from other translation units because their namespace name is unique.

If that's what you mean by export here. But I guess you knew that before.
This only affects the individual cpp file and if anyone defines a second LOGGER there, you should change the names anyway to reflect the different meanings.

Usually I prefer anonymous namespaces inside file-wide namespaces to avoid having to spell out the file-wide namespace each time I need to access other symbols from there. While this is not used here, it makes some sense to keep the style consistent.

Yeah, it didn't seem likely that anything would be exported if declared in the cpp because of the external linkage. I guess it was a bit of sticker shock since I didn't know that anon-namespace anything could be exported.

file-wide namespace?

file-wide namespace?

It's not a technical term. Some maintainers introduced namespace declarations that span virtually the whole file for many packages in MoveIt (e.g., this one) to avoid the full namespace declaration for each symbol.

moveit_ros/occupancy_map_monitor/include/moveit/occupancy_map_monitor/occupancy_map_monitor.h

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp

moveit_ros/occupancy_map_monitor/CMakeLists.txt

moveit_ros/occupancy_map_monitor/include/moveit/occupancy_map_monitor/occupancy_map_monitor.h

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor_middleware_handle.cpp

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp

moveit_ros/occupancy_map_monitor/include/moveit/occupancy_map_monitor/occupancy_map_monitor.h

griswaldbrooks

Added comments. Also, not having the ability to resolve things is very frustrating 🚀

tylerjw · 2021-07-28T22:59:36Z

Added comments. Also, not having the ability to resolve things is very frustrating rocket

I agree, this seems like a github bug. https://twitter.com/weaveringrally/status/1420517755380436997 I couldn't find a github issue and found someone saying that tweeting at github has worked for them before at getting their attention.

I updated all the files I've touched with the same format for includes and consistent doxygen comments with descriptions as best as I understand.

tylerjw · 2021-07-29T19:20:47Z

It seems that re-ordering the includes broke the Foxy CI run. I've installed foxy locally and am working on figuring this one out.

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp

jliukkonen-pn · 2021-08-10T17:52:22Z

moveit_ros/occupancy_map_monitor/include/moveit/occupancy_map_monitor/occupancy_map_monitor.h

+  std::unique_ptr<MiddlewareHandle> middleware_handle_; /*!< The abstract interface to ros */
+  std::shared_ptr<tf2_ros::Buffer> tf_buffer_;          /*!< TF buffer */
+  Parameters parameters_;
+  std::mutex parameters_lock_; /*!< Mutex for synchronizing access to parameters */


I just realized that sometimes the access to parameters is protected by mutex but most of the time it isn't. Is that a bug or am I just reading the code wrong?

This mutex is never actually used to synchronize anything. I think someone had a plan to do that but didn't. I am separately working on implementing dynamic parameters for moveit2 and will clean this up in an unrelated change.

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor_middleware_handle.cpp

griswaldbrooks · 2021-08-05T04:02:18Z

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp

-
-OccupancyMapMonitor::OccupancyMapMonitor(const rclcpp::Node::SharedPtr& node, double map_resolution)
-  : map_resolution_(map_resolution), debug_info_(false), mesh_handle_count_(0), node_(node), active_(false)
+namespace


tylerjw requested a review from griswaldbrooks July 23, 2021 22:15

jliukkonen reviewed Jul 23, 2021

View reviewed changes

griswaldbrooks suggested changes Jul 27, 2021

View reviewed changes

tylerjw commented Jul 27, 2021

View reviewed changes

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp Show resolved Hide resolved

tylerjw force-pushed the occupancy_map_monitor_tests branch from 84aca38 to 3f6d539 Compare July 27, 2021 23:07

tylerjw requested review from jliukkonen and griswaldbrooks July 27, 2021 23:07

griswaldbrooks reviewed Jul 27, 2021

View reviewed changes

griswaldbrooks reviewed Jul 28, 2021

View reviewed changes

nbbrooks requested a review from griswaldbrooks July 28, 2021 14:26

tylerjw force-pushed the occupancy_map_monitor_tests branch 2 times, most recently from 993dd37 to 614c8e3 Compare July 28, 2021 22:58

tylerjw force-pushed the occupancy_map_monitor_tests branch 4 times, most recently from 0e29002 to 5b9fbae Compare July 28, 2021 23:21

tylerjw requested a review from v4hn July 28, 2021 23:30

tylerjw force-pushed the occupancy_map_monitor_tests branch 5 times, most recently from 3b3eb95 to 8637b7b Compare July 29, 2021 17:32

OccupancyMapMonitor tests using Dependency Injection

0489a5b

tylerjw force-pushed the occupancy_map_monitor_tests branch from 8637b7b to 0489a5b Compare July 29, 2021 21:02

henningkayser reviewed Jul 30, 2021

View reviewed changes

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp Outdated Show resolved Hide resolved

moveit_ros/occupancy_map_monitor/src/occupancy_map_monitor.cpp Outdated Show resolved Hide resolved

tylerjw and others added 2 commits July 30, 2021 22:30

Merge branch 'main' into occupancy_map_monitor_tests

3afd395

Parameter struct

f9d231c

tylerjw added 2 commits August 9, 2021 10:17

Merge branch 'main' into occupancy_map_monitor_tests

e10896c

Merge branch 'main' into occupancy_map_monitor_tests

4639571

jliukkonen-pn reviewed Aug 10, 2021

View reviewed changes

gbrooks2-pn approved these changes Aug 11, 2021

View reviewed changes

Merge branch 'main' into occupancy_map_monitor_tests

6f48d76

griswaldbrooks approved these changes Aug 11, 2021

View reviewed changes

tylerjw and others added 4 commits August 11, 2021 17:04

use emplace_back for sensor_plugins

6239ac5

Merge branch 'main' into occupancy_map_monitor_tests

d576379

Merge branch 'main' into occupancy_map_monitor_tests

2164a60

Add missing override annotation

abba127

henningkayser approved these changes Aug 25, 2021

View reviewed changes

henningkayser merged commit 75397fd into moveit:main Aug 25, 2021

henningkayser added this to Done in Galactic/Rolling - 2.3.0 - September 10 Aug 31, 2021

henningkayser mentioned this pull request Jun 9, 2022

Enable moveit2 kinematics framework within ros2 control loop #1329

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OccupancyMapMonitor tests using Dependency Injection #569

OccupancyMapMonitor tests using Dependency Injection #569

tylerjw commented Jul 23, 2021 •

edited

Loading

codecov bot commented Jul 23, 2021 •

edited

Loading

jliukkonen left a comment

jliukkonen Jul 23, 2021 •

edited

Loading

tylerjw Jul 26, 2021

griswaldbrooks Jul 27, 2021

tylerjw Jul 27, 2021

griswaldbrooks Jul 27, 2021

griswaldbrooks Jul 28, 2021

v4hn Jul 28, 2021

griswaldbrooks Jul 28, 2021

v4hn Jul 29, 2021

griswaldbrooks Aug 5, 2021

griswaldbrooks left a comment

tylerjw commented Jul 28, 2021

tylerjw commented Jul 29, 2021

jliukkonen-pn Aug 10, 2021

tylerjw Aug 10, 2021

griswaldbrooks Aug 5, 2021

OccupancyMapMonitor tests using Dependency Injection #569

OccupancyMapMonitor tests using Dependency Injection #569

Conversation

tylerjw commented Jul 23, 2021 • edited Loading

Description

Checklist

codecov bot commented Jul 23, 2021 • edited Loading

Codecov Report

jliukkonen left a comment

Choose a reason for hiding this comment

jliukkonen Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

griswaldbrooks left a comment

Choose a reason for hiding this comment

tylerjw commented Jul 28, 2021

tylerjw commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerjw commented Jul 23, 2021 •

edited

Loading

codecov bot commented Jul 23, 2021 •

edited

Loading

jliukkonen Jul 23, 2021 •

edited

Loading