Refactor driver library to follow patterns from PickNikRobotics/ros2_epick_gripper #34

kineticsystem · 2023-09-05T11:14:17Z

The monolithic code has been broken apart to introduce tests.

The serial connection creation has been moved from the driver constructor to the hardware interface configure method.

A lot of code has been refactored with the intention of bringing the Epick gripper into this same repository and reusing some common code.

codecov · 2023-09-05T11:24:01Z

Codecov Report

Merging #34 (f5d7b7a) into main (743d20f) will increase coverage by 20.77%.
Report is 6 commits behind head on main.
The diff coverage is 21.34%.

❗ Current head f5d7b7a differs from pull request most recent head 4c3fd18. Consider uploading reports for the commit 4c3fd18 to get more accurate results

@@            Coverage Diff             @@
##            main      #34       +/-   ##
==========================================
+ Coverage   0.00%   20.77%   +20.77%     
==========================================
  Files          5       20       +15     
  Lines        352      597      +245     
  Branches       0      240      +240     
==========================================
+ Hits           0      124      +124     
+ Misses       352      341       -11     
- Partials       0      132      +132

Flag	Coverage Δ
unittests	`20.77% <21.34%> (+20.77%)`	⬆️

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

Files	Coverage Δ
.../include/robotiq_driver/default_driver_factory.hpp	`100.00% <100.00%> (ø)`
.../include/robotiq_driver/default_serial_factory.hpp	`100.00% <100.00%> (ø)`
robotiq_driver/include/robotiq_driver/serial.hpp	`100.00% <100.00%> (ø)`
robotiq_driver/src/crc_utils.cpp	`100.00% <100.00%> (ø)`
.../tests/test_robotiq_gripper_hardware_interface.cpp	`33.33% <33.33%> (ø)`
robotiq_driver/tests/mock/mock_driver.hpp	`40.00% <40.00%> (ø)`
robotiq_driver/tests/test_crc_utils.cpp	`14.28% <14.28%> (ø)`
robotiq_driver/tests/test_data_utils.cpp	`45.45% <45.45%> (ø)`
...driver/include/robotiq_driver/driver_exception.hpp	`0.00% <0.00%> (ø)`
robotiq_driver/tests/mock/mock_serial.hpp	`0.00% <0.00%> (ø)`
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

moriarty · 2023-09-05T12:25:06Z

robotiq_driver/tests/CMakeLists.txt

+find_package(ament_cmake_gtest REQUIRED)
+find_package(ament_cmake_gmock REQUIRED)
+find_package(ament_lint_auto REQUIRED)
+find_package(ros2_control_test_assets REQUIRED)


This needs to be added to the package.xml as a <test_depend>

Done. Thanks.

moriarty · 2023-09-05T13:01:26Z

From the logs it looks like the test assets were installed, but that file still not found..

https://github.com/PickNikRobotics/ros2_robotiq_gripper/actions/runs/6084786368/job/16507440934

2023-09-05T12:51:27.9022317Z �[1mexecuting command [apt-get install -y -qq ros-rolling-ament-cmake-copyright]�[0m
2023-09-05T12:51:27.9022908Z �[1mexecuting command [apt-get install -y -qq ros-rolling-ament-cmake-lint-cmake]�[0m
2023-09-05T12:51:27.9023473Z �[1mexecuting command [apt-get install -y -qq ros-rolling-ament-cmake-gmock]�[0m
2023-09-05T12:51:27.9024043Z �[1mexecuting command [apt-get install -y -qq ros-rolling-ros2-control-test-assets]�[0m
2023-09-05T12:51:27.9024623Z Setting up ros-rolling-ros2-control-test-assets (3.16.0-1jammy.20230711.194957) ...
2023-09-05T12:51:27.9025474Z �[1mexecuting command [apt-get install -y -qq ros-rolling-gripper-controllers]�[0m
2023-09-05T12:51:27.9025954Z Setting up python3-typeguard (2.2.2-1.1) ...
2023-09-05T12:51:27.9026404Z Setting up ros-rolling-tl-expected (1.0.2-3jammy.20230711.195032) ...
2023-09-05T12:51:27.9026836Z Setting up python3-markupsafe (2.0.1-2build1) ...
2023-09-05T12:51:27.9027224Z Setting up python3-tz (2022.1-1ubuntu0.22.04.1) ...
2023-09-05T12:51:27.9027645Z Setting up python-babel-localedata (2.8.0+dfsg.1-7) ...
2023-09-05T12:51:27.9028097Z Setting up ros-rolling-tcb-span (1.0.2-3jammy.20230711.195007) ...
2023-09-05T12:51:27.9028578Z Setting up ros-rolling-realtime-tools (2.5.0-2jammy.20230711.230518) ...
2023-09-05T12:51:27.9029006Z Setting up libelf-dev:amd64 (0.186-1build1) ...
2023-09-05T12:51:27.9029374Z Setting up liblzma-dev:amd64 (5.2.5-2ubuntu1) ...
2023-09-05T12:51:27.9029756Z Setting up python3-babel (2.8.0+dfsg.1-7) ...
2023-09-05T12:51:27.9030202Z Setting up ros-rolling-control-toolbox (3.1.0-1jammy.20230711.231139) ...
2023-09-05T12:51:27.9030620Z Setting up libdw-dev:amd64 (0.186-1build1) ...
2023-09-05T12:51:40.8430488Z Setting up ros-rolling-rsl (0.2.2-1jammy.20230711.232011) ...
2023-09-05T12:51:40.8430952Z Setting up python3-jinja2 (3.0.3-1) ...
2023-09-05T12:51:40.8441174Z Setting up ros-rolling-parameter-traits (0.3.3-1jammy.20230711.232510) ...
2023-09-05T12:51:40.8442021Z Setting up ros-rolling-backward-ros (1.0.2-4jammy.20230711.194802) ...
2023-09-05T12:51:40.8442572Z Setting up ros-rolling-generate-parameter-library-py (0.3.3-1jammy.20230711.194824) ...
2023-09-05T12:51:40.8443177Z Setting up ros-rolling-generate-parameter-library (0.3.3-1jammy.20230711.232811) ...
2023-09-05T12:51:40.8443734Z Setting up ros-rolling-gripper-controllers (3.12.0-1jammy.20230718.185445) ...
2023-09-05T12:51:40.8444330Z �[1mexecuting command [apt-get install -y -qq ros-rolling-joint-state-broadcaster]�[0m
2023-09-05T12:51:40.8444912Z Setting up ros-rolling-joint-state-broadcaster (3.12.0-1jammy.20230718.185413) ...
2023-09-05T12:51:40.8445798Z ##[endgroup]
2023-09-05T12:51:40.8446214Z �[32m'install_target_dependencies' returned with code '0' after 2 min 3 sec�[0m
2023-09-05T12:51:40.8446586Z 
2023-09-05T12:51:40.8448241Z ##[group]�[34mbuild_target_workspace�[0m
2023-09-05T12:51:40.8448434Z 
2023-09-05T12:51:40.8449251Z �[1m$ ( source /home/runner/work/ros2_robotiq_gripper/ros2_robotiq_gripper/.work/upstream_ws/install/setup.bash && cd /home/runner/work/ros2_robotiq_gripper/ros2_robotiq_gripper/.work/target_ws && colcon build --event-handlers desktop_notification- status- terminal_title-; )�[0m
2023-09-05T12:51:40.8449890Z Starting >>> robotiq_controllers
2023-09-05T12:51:40.8450174Z Starting >>> robotiq_description
2023-09-05T12:51:40.8450476Z Finished <<< robotiq_description [0.81s]
2023-09-05T12:51:40.8454764Z Starting >>> robotiq_driver
2023-09-05T12:51:40.8455270Z Finished <<< robotiq_controllers [2.93s]
2023-09-05T12:51:40.8455640Z --- stderr: robotiq_driver
2023-09-05T12:51:40.8456545Z �[01m�[K/home/runner/work/ros2_robotiq_gripper/ros2_robotiq_gripper/.work/target_ws/src/ros2_robotiq_gripper/robotiq_driver/tests/test_robotiq_gripper_hardware_interface.cpp:41:10:�[m�[K �[01;31m�[Kfatal error: �[m�[Kros2_control_test_assets/components_urdfs.hpp: No such file or directory
2023-09-05T12:51:40.8457502Z    41 | #include �[01;31m�[K<ros2_control_test_assets/components_urdfs.hpp>�[m�[K
2023-09-05T12:51:40.8457956Z       |          �[01;31m�[K^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~�[m�[K

This reverts commit f26c1f3e0fffd9cd1cc603adde5b3265962f96f8.

This reverts commit 4bf404b2d78818853054658aefc81247ee7f69d7.

…GTests.

…r to robotiq_description.

moriarty

This is a huge PR so I just took a first pass and left some comments.

IIRC we shouldn't set the VERSION of a ROS package in the CMakeLists.txt as this is set in the package.xml and it should match, with the version in the package.xml being the source of truth

.github/workflows/prerelease-check.yml

moriarty · 2023-09-08T17:58:54Z

.github/workflows/prerelease-check.yml

+          - humble
+          - iron
+          - master


This must be an old/outdated file or one that was autogenerated or accidentally copied while setting up this repo.

This repo doesn't have humble/iron/master branches only main

moriarty · 2023-09-08T18:03:05Z

robotiq_description/urdf/robotiq_gripper.ros2_control.xacro

@@ -13,6 +13,10 @@
        <ros2_control name="${name}" type="system">
            <!-- Plugins -->
            <hardware>
+
+                <!-- Set use_dummy to true to connect to a dummy driver for testing purposes. -->
+                <param name="use_dummy">false</param>


Should this be use_mock I thought there was a discussion to unify the language in another project

robotiq_driver/CMakeLists.txt

robotiq_driver/tests/CMakeLists.txt

Co-authored-by: Alex Moriarty <[email protected]>

moriarty

I hope github saves my files viewed when i finish this review and leave the comments so far.

moriarty · 2023-09-26T00:21:10Z

.github/workflows/ci-ros-lint.yml

+        with:
+          distribution: rolling
+          linter: cpplint
+          arguments: "--linelength=121 --filter=-whitespace/newline"


why is this 121 ? that seems strange.

There is a bug in the clang formatter. Sometimes a line is cut at 121 and this causes this linter to fail.
I experienced this bug in one single line of code: hardware_interface.cpp line 73.
If you have any suggestions let me know, maybe I'm missing something.

moriarty · 2023-09-26T00:43:00Z

robotiq_description/CMakeLists.txt

@@ -10,10 +10,11 @@ find_package(ament_cmake REQUIRED)

 install(
  DIRECTORY
-    urdf
+    config


Why is the controller configuration being moved from the robotiq_driver/config -> robotiq_description/config ?

And why is the controller the launch for the controllers also being moved into the robotiq_description?

I think the description should have only the urdf but now that I look at the package.xml in robotiq_description I see that we've already messed up the dependencies and seperation a bit because the description depends on rviz2 which it should not.

I need to think about this.

What I want to avoid is the driver itself depending on any gui or simulation dependencies... which already seems to not be the case 😞 I didn't notice this when originally taking over this repo and opening it... but it would be great if someone could install the driver without needing to install rviz.

So, in my opinion, the driver (robotiq_driver) should not know who controls it, that's why I moved the controller config out of it.

On the other hand, the description package (robotiq_description) which contains the robot launch file should know both about the driver and the controller (robotiq_controllers), because you want to use it to start a working robot.

I also think that the urdf in the description package should be of a robot example, not a real robot which may differ a lot from the example. Because the description package knows about the robot and contains the launch file, it seemed natural to add a dependency to rviz too.

moriarty · 2023-09-26T00:45:05Z

robotiq_description/launch/robotiq_control.launch.py

@@ -67,6 +64,11 @@ def generate_launch_description():
            description="Absolute path to rviz config file",
        )
    )
+    args.append(
+        launch.actions.DeclareLaunchArgument(
+            name="launch_rviz", default_value="false", description="Launch RViz?"


see my previous comment maybe we should remove this, or stick it somewhere else so that rviz is not a transitive dependency of the driver?

We need to find an agreement or check with somebody else in the team. To me, the driver is the one in the robotiq_driver package. The robotiq_description describes how to use the driver.

moriarty · 2023-09-26T00:47:23Z

robotiq_driver/CMakeLists.txt

+  include/robotiq_driver/fake/fake_driver.hpp
+  include/robotiq_driver/crc_utils.hpp
+  include/robotiq_driver/data_utils.hpp
+  include/robotiq_driver/default_driver.hpp
+  include/robotiq_driver/default_driver_factory.hpp
+  include/robotiq_driver/default_serial.hpp
+  include/robotiq_driver/default_serial_factory.hpp
+  include/robotiq_driver/driver.hpp
+  include/robotiq_driver/driver_exception.hpp
+  include/robotiq_driver/driver_factory.hpp
+  include/robotiq_driver/hardware_interface.hpp
+  include/robotiq_driver/serial.hpp
+  include/robotiq_driver/serial_factory.hpp
+  include/robotiq_driver/visibility_control.hpp
+  src/crc_utils.cpp
+  src/data_utils.cpp
  src/hardware_interface.cpp
-  src/robotiq_gripper_interface.cpp
+  src/default_driver.cpp
+  src/default_driver_factory.cpp
+  src/fake/fake_driver.cpp
+  src/default_serial.cpp
+  src/default_serial_factory.cpp


I take it the files listed here are the meat of this PR and the rest of the changes in the PR are actually minor formatting changes and style changes but it's very hard to tell with such a large PR

I haven't changed the logic of how the driver works, I broke it apart into smaller pieces to allow testing without being connected to the hardware. Originally it was a monolithic project with no tests and with the hard requirement of having hardware at hand.

I intentionally followed the structure of the Epick Gripper so that later we can merge the two drivers into the same repo and extract common code. For example, all the serial communication is common to both.

I definitely made the mistake of formatting the code later which makes the PR hard to read.

Sorry this was just a note to self when I was reviewing I wanted to check the main changes parts in more detain.

I definitely agree we should get he Epick Grippper merged into one repo that would be great.

moriarty · 2023-09-26T00:56:19Z

robotiq_driver/include/robotiq_driver/fake/fake_driver.hpp

+ * <!-- Set use_dummy to true to connect to a dummy driver. -->
+ * <param name="use_dummy">true</param>
+ */
+class FakeDriver : public Driver


Here there is a mismatch between the param an the thing Fake vs Dummy. I think Fake should be used instead of Dummy but you also have a tests/mock/mock_driver.hpp so is this a Fake or a Mock driver?

From the point of view of the client, the HardwareInterface, a Fake is a simplified replacement for the real thing. A Mock is a class in which I want to test its expectations.

"use_fake" would be probably more accurate. The reason I used "use_dummy" is because it is used in the Dynamixel ROS2 drivers: https://github.com/dynamixel-community/dynamixel_hardware/blob/d84511db57094e3aeb5c09181517649e40172a67/dynamixel_hardware/include/dynamixel_hardware/dynamixel_hardware.hpp#L106

moriarty · 2023-09-26T01:02:16Z

robotiq_hardware_tests/package.xml

+<package format="3">
+  <name>robotiq_hardware_tests</name>
+  <version>0.0.1</version>
+  <description>ROS2 driver for the Robotiq gripper.</description>


What does this package actually do? who is the intended user? should it be robotiq_hardware_utils is it simple commands for checking that hardware is functioning as expected? or is it tests that don't need to be distributed via a ros package but could just be internal to the robotiq_driver package for development but not packages and installed for end users?

They are commands (to be run in the terminal) to check that the hardware is properly connected and working as expected.
If you think "robotiq_hardware_utils" is more appropriate I do not mind renaming it.

Co-authored-by: Alex Moriarty <[email protected]>

moriarty · 2023-10-13T13:22:11Z

robotiq_hardware_tests/CMakeLists.txt

+# See https://docs.ros.org/en/foxy/How-To-Guides/Ament-CMake-Documentation.html
+
+cmake_minimum_required(VERSION 3.8)
+project(robotiq_hardware_tests VERSION 0.0.1 LANGUAGES CXX)


This VERSION number should be set in the package.xml and IIRC it should match the other versions else the ros release tools complain.

Refactoring for testability.

6b782ba

kineticsystem added 2 commits September 5, 2023 13:32

Added simple test.

d32b44e

Pre-commits.

1a8aeeb

moriarty reviewed Sep 5, 2023

View reviewed changes

Added missing packages for testing.

ddda502

kineticsystem added 23 commits September 5, 2023 15:02

In the on_configure method we try to connect to the hardware.

79dfec4

Added missing package.

f371468

Refactoring.

c11d657

Removed not existing package.

76c5497

Removed ros-linter. Reformatted code as per Studio conventions.

8788593

Revert "Auxiliary commit to revert individual files from 8788593"

d7cfc1b

This reverts commit f26c1f3e0fffd9cd1cc603adde5b3265962f96f8.

Linting packages.

46b8b58

Remove ros linting clashing with .clang-format.

92eefce

Revert "Auxiliary commit to revert individual files from 92eefce"

19569f8

This reverts commit 4bf404b2d78818853054658aefc81247ee7f69d7.

Updated CMakeLists.txt

dad1114

Removed connection to the serial port in the contructor.

de63c83

Refactoring for testing.

312ab08

Pre-commits.

c1dc459

Added copyright.

4c3eafc

Increased max line size to 120.

e1089ad

Fixed ROS lints.

d283f18

Try to exclude tests from linting.

2239dd9

Removed cppcheck linter because it has problem recognizing macros in …

011df4b

…GTests.

Trying to add ros2_control_test_assets to test CMakeLists.txt

7414c37

Tryint to build everything on Ubuntu 22.04.

56ddcd4

Moved gripper_interface_test command into its own package.

51e98f8

Some refactoring.

dfe6f79

Some refactoring.

21dc2c7

kineticsystem added 5 commits September 6, 2023 23:50

Removed bringup package.

91a08d0

Added fake driver for testing. config folder moved from robotiq_drive…

03e3701

…r to robotiq_description.

Hardware interface and driver refactoring.

bcd5fe6

Driver refactoring.

7301c64

Fixed pre-commits.

d719ed2

moriarty suggested changes Sep 8, 2023

View reviewed changes

moriarty reviewed Sep 8, 2023

View reviewed changes

robotiq_driver/tests/CMakeLists.txt Show resolved Hide resolved

kineticsystem and others added 6 commits September 8, 2023 22:52

Update .github/workflows/prerelease-check.yml

a434edf

Co-authored-by: Alex Moriarty <[email protected]>

Update .github/workflows/prerelease-check.yml

fb01131

Co-authored-by: Alex Moriarty <[email protected]>

Update .github/workflows/prerelease-check.yml

a5a9468

Co-authored-by: Alex Moriarty <[email protected]>

Update .github/workflows/prerelease-check.yml

21d1b4f

Co-authored-by: Alex Moriarty <[email protected]>

Update .github/workflows/prerelease-check.yml

c5819d8

Co-authored-by: Alex Moriarty <[email protected]>

Adding ros2_control_test_assets to CMakeLists.txt

1658cdf

kineticsystem marked this pull request as ready for review September 8, 2023 21:24

kineticsystem added 3 commits September 8, 2023 23:38

Removed unecessary dependencies.

9f0a100

Added CRC-16/MODBUS test.

660eec5

Unit tests.

0f41ce9

moriarty reviewed Sep 26, 2023

View reviewed changes

Update robotiq_driver/CMakeLists.txt

f5d7b7a

Co-authored-by: Alex Moriarty <[email protected]>

cmelliott2 assigned kineticsystem Oct 9, 2023

schornakj changed the title ~~Refactoring for testability.~~ Refactor driver library to follow patterns from PickNikRobotics/ros2_epick_gripper Oct 11, 2023

schornakj approved these changes Oct 11, 2023

View reviewed changes

kineticsystem enabled auto-merge (squash) October 11, 2023 19:37

moriarty reviewed Oct 13, 2023

View reviewed changes

kineticsystem and others added 2 commits October 27, 2023 21:19

Removed version from CMakeLists.txt file.

4c3fd18

Merge branch 'main' into pr-refactoring

b4b73e6

moriarty approved these changes Oct 31, 2023

View reviewed changes

kineticsystem merged commit 29ce398 into main Oct 31, 2023

delete-merged-branch bot deleted the pr-refactoring branch October 31, 2023 13:41

moriarty mentioned this pull request Dec 1, 2023

Potential use with Robotiq 3f? #39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor driver library to follow patterns from PickNikRobotics/ros2_epick_gripper #34

Refactor driver library to follow patterns from PickNikRobotics/ros2_epick_gripper #34

kineticsystem commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

moriarty Sep 5, 2023

kineticsystem Sep 5, 2023

moriarty commented Sep 5, 2023

moriarty left a comment

moriarty Sep 8, 2023

moriarty Sep 8, 2023

moriarty left a comment

moriarty Sep 26, 2023

kineticsystem Sep 28, 2023

moriarty Sep 26, 2023

kineticsystem Sep 28, 2023

moriarty Sep 26, 2023

kineticsystem Sep 28, 2023

moriarty Sep 26, 2023

kineticsystem Sep 28, 2023 •

edited

Loading

moriarty Sep 28, 2023

moriarty Sep 26, 2023

kineticsystem Sep 28, 2023

moriarty Sep 26, 2023

kineticsystem Sep 28, 2023

moriarty Oct 13, 2023

kineticsystem Oct 27, 2023

Refactor driver library to follow patterns from PickNikRobotics/ros2_epick_gripper #34

Refactor driver library to follow patterns from PickNikRobotics/ros2_epick_gripper #34

Conversation

kineticsystem commented Sep 5, 2023 • edited Loading

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moriarty commented Sep 5, 2023

moriarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moriarty left a 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

kineticsystem Sep 28, 2023 • 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

kineticsystem commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

kineticsystem Sep 28, 2023 •

edited

Loading