Introduce ROS interface abstraction to improve testability #156
Conversation
There was a problem hiding this comment.
I have some requested changes, but I agree with the direction:
- mock-able abstraction classes which inherit from pure virtual interfaces > free functions
- dependency injection of mock-able abstraction classes
- using interfaces in the code, rather than the child class
- using
unique_ptr's for storage that automatically cleans up
| namespace ros_integration | ||
| { | ||
|
|
||
| class RosNodeAbstraction : public RosNodeAbstractionIface |
There was a problem hiding this comment.
I think this class is conflating two things, storage of nodes and the abstraction of nodes. Ideally, the storage of the nodes would be not public, whereas the abstraction for nodes would be public and would also not reference ROS 2 API directly (which the storage does since it takes and returns rclcpp::Node's).
| * | ||
| * \param node_name name to be used as the key for the rclcpp node | ||
| * \param node the rclcpp node to be stored | ||
| */ |
There was a problem hiding this comment.
Probably best to not duplicate the documentation, instead pick one place and document it there. I would guess the non-Iface class is best so long as it is public. It's possible you'd want to document both, but the content should be materially different, otherwise one should just reference the other.
| TopicDisplayWidget::TopicDisplayWidget( | ||
| const std::string & node_name, | ||
| std::unique_ptr<rviz_common::ros_integration::RosNodeAbstractionIface> node_abstraction) | ||
| : node_name_(node_name), node_abstraction_(std::move(node_abstraction)) |
There was a problem hiding this comment.
If you're going to pass in a ros node abstraction, just get the name from that rather than also passing the node name along and storing it.
| @@ -70,6 +74,7 @@ private Q_SLOTS: | |||
| QTimer * continue_timer_; | |||
| VisualizationFrame * frame_; | |||
| std::string node_name_; | |||
| std::unique_ptr<rviz_common::ros_integration::RosClientAbstractionIface> ros_client_; | |||
There was a problem hiding this comment.
nitpick: this name ros_client_ leaves out "abstraction", but in the other file you name the variable node_abstraction leaving our "ros". I don't mind leaving out ros or abstraction (though my default preference would be to include both in both variable names), but it would be best to keep them at least consistent, e.g. client_abstraction_ or ros_node_.
wjwwood
added
in progress
|
First try with the ros ci:
|
wjwwood
added
in review
|
The ci build failures are due to the |
|
Next ci try (ros2 master is green again)
|
- move all public free function of ros_integration namespace to interface - add test for correct use of ros shutdown in VisualizerApp
…erface - also replaces the free function in get_topic_names_and_types
- a RosNodeAbstraction shall encapsulate a rclcpp::Node in the future and thus keeps track of its name (this simplifies a few functions) - for testing the rest of rviz the public api from the ros_integration namespace should be mocked - the optional storage parameter of RosNodeAbstraction is intended for unit testing of RosNodeAbstraction only and should not be needed otherwise
anhosi
force-pushed
the
feature/ros_interface_abstraction
branch
from
f77ab00
to
e9412d7
Compare
|
Next try with ci 😄
|
| * \return name of the resulting ROS node | ||
| */ | ||
| std::string | ||
| init(int argc, char ** argv, const std::string & name, bool anonymous_name) override; |
There was a problem hiding this comment.
Should this return a RosNodeAbstraction instead?
There was a problem hiding this comment.
👍 That would be a nicer interface.
There was a problem hiding this comment.
Will you do that now or should I go ahead and merge this and you can change it later/follow up pr?
There was a problem hiding this comment.
I will do that now. It should be a small change.
There was a problem hiding this comment.
This is more involved than I first thought. Currently we use the node name as a key to easily get access to an RosNodeAbstraction instance. The lifetime of this node abstraction has no effect on the stored rclcpp::Node inside the RosNodeStorage but maybe should have (?). What should be the behavior is the backing rclcpp::Node corresponding to a RosNodeAbstraction is clean up? ...
Any suggestions/ thoughts on that @wjwwood ?
I still agree that returning "some kind" of node from init is preferable but I am yet not sure how to do that properly. So I would prefer to do this in a follow up pr.
There was a problem hiding this comment.
I'd probably not make the node abstraction object scoped, just because you cannot "cleanup" a ROS 1 node, so maybe we don't need to worry about it.
| * | ||
| * \param node_name name of the node to create | ||
| */ | ||
| explicit RosNodeAbstraction(const std::string & node_name); |
There was a problem hiding this comment.
I sort of think it would be better to have the constructor private and only generate these instances via a get_or_create_node_by_name() (or some other name, just trying to be specific here) method of the RosClientAbstrction class.
There was a problem hiding this comment.
Introducing a static factory function does not allow mocking and thus kind of defeats the purpose of these changes. I agree that the current constructor solution is not ideal. I will try to expore using a separate factory.
There was a problem hiding this comment.
Ok, well don't spend too much time on it, if you come up with something that works better but still lets you mock it then great, otherwise we'll take it as-is.
The other thing to think about is how do you destroy nodes that have been created? In the case of ROS 2 (at least) you can make more than one node and you can remove them too. Maybe the RosClientAbstraction needs a destroy node method which is implicitly called on shutdown() if not already called for each node (typically just the one created during init()). Or the RosNodeAbstraction could be a scoped object (could be a shared_ptr with the node storage keeping only a weak_ptr). Dunno, just something to think about.
* add dependency between rviz2 and rviz_default_plugins (#149) * Move pointcloud displays to rviz_default_plugins (#153) * Move pointcloud displays to rviz_default_plugins * Move pointcloud tests to rviz_default_plugins * Adjust namespaces for pointcloud display files * Fix namespaces and import for pointcloud tests * Rename transformers folder * Fix linting errors * Move pointcloud files between CMakeLists, remove display from display_factory * Add pointcloud displays to plugins_description.xml * fix includes * add missing test dependencies * uncrustify * Revert "uncrustify" This reverts commit 03ed305. * disable uncrustify on long include statements * Fixed missing break in switch-case (#158) * Fall-through is not intended: listAppendNew sets the type to List while the call to setValue after the fall-through resets the type to Value. * Remove now obsolete function (#163) - was made obsolete by pr #136 which removed the memcopy * Reenable and fix config loading (#167) * Switch to assimp version 4.1.0 (#169) This should fix the dynimcally linked assimp build on Windows. * Feature/grid display refactoring (#165) * Move billboard_line to rviz2 for use in grid * Restore billboard functionality in Grid * Uncrustify + cpplint * Add tests for billboard_line - need to expose getChains for testing - one test documents faulty behaviour of Ogre in test * Refactoring of billboard_line - extract functions - fix functionality for huge numbers of elements per chain - add TODO comment to use internal Ogre functionality when fixed * Add tests for grid - need to expose functions for testing * Refactoring of grid * Propose additional newLine() possible in BillboardLine for ease of use - this gets rid of awkward "if(first element) don't add a new line" - simplifies usage - allows further refactoring of grid (for instance) * Minor refactoring of GridDisplay * Disable tests for now * Add functional header and std:: for "bind" * Introduce ROS interface abstraction to improve testability (#156) * Introduce interface for ros abstraction - move all public free function of ros_integration namespace to interface - add test for correct use of ros shutdown in VisualizerApp * Document recommendation for testable code * Move free functions from rclcpp_node_storage into RosAbstraction class * Rename RosAbstraction to RosClientAbstraction * Introduce RosNodeAbstraction for the node-centric part of the ros interface - also replaces the free function in get_topic_names_and_types * Cleanup: remove default arguments for virtual function * Refactor towards proper usage of RosNodeAbstraction * nitpick: alphabetical ordering * Comment only non interface classes (avoid duplicate documentation) * Cleanup: use consistent variable naming * Extract storage functionality from RosNodeAbstraction * Refactor RosNodeAbstraction towards a replacement of rclcpp::Node - a RosNodeAbstraction shall encapsulate a rclcpp::Node in the future and thus keeps track of its name (this simplifies a few functions) - for testing the rest of rviz the public api from the ros_integration namespace should be mocked - the optional storage parameter of RosNodeAbstraction is intended for unit testing of RosNodeAbstraction only and should not be needed otherwise * Fix gmock dependency * Migrate ImageDisplay (#164) * Reintroduce RosTopicProperty to MessageFilterDisplays - we can't correctly filter for message types. Added ToDo - Allow empty names in RosTopicProperty for now Refactor MessageFilterDisplay to correctly handle exceptions * Move ros_image_texture to default_plugins - Does mostly Ogre things, so definitely not rviz_common - Fix linters in RosImageTexture * Show image display in GUI and render default image Fix linters in rviz_rendering Improve naming of RenderWindow::setupSceneAfterInit() Move RenderPanel header to include folder * Let ImageDisplay extend MessageFilterDisplay to receive ROS image messages * Fix uncrustify and cpplint for ImageDisplay * Rename MessageFilterDisplay to RosTopicDisplay Remove the message filter parts currently commented out. Add comments regarding the missing message filter display * Add reliability profile to RosTopicDisplay * Introduce QueueSizeProperty * Put node from Display into _RosTopicDisplay * Initial move of ImageDisplay * Make ImageDisplay compile (except for missing image_display_base) * Reintroduce QueueSizeProperty to displays - Both ImageDisplay and PointCloud had the property previously * Offer generic way to change QoS objects * Expose ogre testing enviroment header in rviz_rendering * Add tests for ROSImageTexture * Use override instead of virtual where applicable * Extract scene setup lambda into separate method * Fix typo * Adapt interface of normalize and add tests * Interface abstraction for ROSImageTexture, replace raw pointers * Add onInitialize(string topic_name) to RosTopicDisplay This onInitialize automatically sets the default topic name to listen to * Move Image display to rviz_default_plugins * Add first tests for image display * Refactor ros_image_texture * Remove misleading onInitialize method from RosTopicDisplay * Disable tests for now * Implement review comments * Remove unnecessary check * fix memory leak (remake) (#173) * fix memory leak Signed-off-by: Chris Ye <[email protected]> * remove vestigial example scene * use unique_ptr instead of shared_ptr and add comment * cpplint * Fix Windows build (#175) * wip * Fix visibility in rviz_common - needs to export some more symbols from rviz_common - small uncrustify changes * Fix stl_loader * Use dynamically linked OgrePlugins Don't link against Plugins anymore (loaded dynamically) Set ogre_plugin_dir to load dynamically Release and Debug need different behaviour * Fix yaml-cpp dynamic linking see jbeder/yaml-cpp#332 * Install rviz_rendering dlls * Reenable Windows tests * Move test folder to correct place - this is now consistent with rviz_common and rviz_default_plugins - fixup location of ogre_testing_environment - ogre_testing_environment needs to be exported * Fix linking against Ogre * Add zlib to be build on Windows * Change targets to unknown instead of shared * Fix billboard_line tests for Windows - need to expose symbols of billboard line for Windows - copy constructor of billboard line is not generated on Windows * Use rviz_common::UniformStringStream instead of std::stringstream * Fix linking of tests on Windows - just like rviz2.exe, tests need dll directories in %PATH% - add local install to environment variables for testing - in isolated builds, ogre resources cannot be found otherwise - enable tests in rviz_default_plugins on Windows but disable display * Fix grid_test * Linking to exported template function is problematic on Windows - this does not work with RVIZ_DEFAULT_PLUGINS_PUBLIC - delete tests for templated function - hide templated function again * Fix image_display tests * Introduce visibility control in rviz_default_plugins * run windeployqt on rviz2 binary to make it runnable after installation * extend PATH on Windows with location of libraries for vendor packages * Add missing visibility_control.hpp and fix license * log build output of dependencies to file * Fix Windows warnings * delete visibility from rviz_default_plugins * Disable deprecation warnings in Ogre * Fix dllexport warning in ros abstraction * fix linking warnings in rviz_rendering * fix linking errors * Fix warning in rviz_rendering * Reintroduce destructor to fix test * TF Display migration (#182) * Move tf_display * Make tf_display compile * Dissect tf_display into several classes * C++14 style adjustments of rviz_default_plugin tf classes - frame_selection_handler - frame_info - tf_display * Fix infinite loop when adding a display by topic * Rename tf_display folder to just tf * Expose functions for testability - BoundingRadius because it might need refactoring - getRenderOperation to allow updating Add tests to movable_text - Some tests already show bugs * Refactor movable_text - delete duplicate code in _setupGeometry() Further refactoring * Fix a couple of bugs - Line spacing could be set but did not have an effect - Using HorizontalAlignment::H_CENTER and text with spaces did not work correctly - Using VerticalAlignment::V_ABOVE did not work correctly * Extract more functions and refactoring * Set reasonable default value for spacing from the start * Refactoring of file style - Remove unnecessary variables - Delete non-virtual override that is protected - Remove duplicate method to getBoundingBox - Remove using namespace Ogre * Refactoring of tests - Expose update functionality instead of getRenderOp for testing - Change tests to approximate compare * Split methods in tf_display * Move some methods from tf_display to frame_info * Add tf_display to plugins_description * Change inheritance of MovableText to avoid visibility problems * Update Ogre to 1.10.11 (#181) * Use Ogre 1.10.11 * Fix deprecation warnings * Fix billboard_line - Ogre::BillboardLine is fixed, so we can remove manual counting - newLine() really finishes the line, thus rename to finishLine() * Fix cmake on Linux not appending _d any more * Migrate camera display (#183) * Initial move * Move display_group_visibility_properties * Make camera display compile * Add camera display to default plugins * Quickfix Segfault * First try * First working camera display * Shows the main scene from a different perspective * Misses visibility bits * Ignores caminfo status * Adds queue size property, visibility masks, listener removal on destruct Also includes a dummy caminfo for testing * Does not crash on startup, without visibility property * Reenable visibility properties * Add subscriber for CameraInfo messages * Fix linter issues * Reenable exact time mode * Implementing review comments * Extract smaller methods from large methods, cleanup Cleanup such as * correct float conversion * use lambda instead of bind * remove old comments * More method extraction and cleanup * Clean up and refactor display visibility properties * Split onInitialize method, remove obsolete comments * Fix visibility of BitAllocator * Fix Windows visibility * Default initialize forced_hidden (#187) * point to the source and bugtracker used in ros2 rather than the upstream one (#190) * Fix pointcloud2 display not accepting valid points (#189) * Fix rviz crash when removing a display (#191) The crash occurred when adding a camera display and then deleting any display that was created before adding the camera display. * print message to stdout instead of stderr (#193) * fix possible null pointer is dereferenced (#178) Signed-off-by: Chris Ye <[email protected]> * Migrate polygon display (#194) * Initial move * Add to plugins_description.xml * Make display compile * Uncrustify * Change from uint32_t to size_t to silence warning on Windows. * Minor refactoring. * Enable alpha blending for the Polygon Display. * Let Ogre delete its render windows (#195) * Fix warnings from pluginlib (#196) * Update rviz2 installation to run it using `ros2 run` (#198) * disable RobotModel until it can be refactored to not use hard coded paths * Disable anti aliasing on Windows (#199) This fixes rendering issues on Windows when opening two or more render windows.
The free functions in the ros_integration namespace cannot be mocked in tests. This PR introduces two interfaces, a generic ros client interface (init, shutdown, ok) and a ros node interface for all the mode-centric functions.