Add simple transmission class by bmagyar · Pull Request #245 · ros-controls/ros2_control

bmagyar · 2020-11-19T20:57:02Z

This is me porting over SimpleTransmission from ROS1.
I haven't updated the documentation but it should be mostly relevant.
Updates licenses to Apache2 from original.

Question:
Should I add a JointInterface and an ActuatorInterface class to transmission_interface which transmissions can be used with?

I added them

Should communicate clearly what "handle" is intended to be used where. If you have other suggestions, fire away.

hardware_interface/include/hardware_interface/handle.hpp

transmission_interface/include/transmission_interface/simple_transmission.h

bmagyar · 2020-11-23T09:07:24Z

@Karsten1987 @destogl this should fix one half of the failures we are getting from #236 , please let me know your feeling about this updated version.

destogl

It doesn`t look bad. I am not happy with the used nomenclature, but I also do not have any better ideas.

I will check tests to be finished to approve it.

transmission_interface/include/transmission_interface/simple_transmission.h

Karsten1987

I don't see any changes here.

hardware_interface/include/hardware_interface/handle.hpp

Karsten1987 · 2020-11-23T18:53:11Z

transmission_interface/CMakeLists.txt

 target_include_directories(transmission_parser PUBLIC include)
 ament_target_dependencies(transmission_parser hardware_interface tinyxml2_vendor TinyXML2)
-ament_export_dependencies(hardware_interface tinyxml2_vendor TinyXML2)
+ament_export_dependencies(transmission_parser tinyxml2_vendor TinyXML2)


consider moving this further down next to ament_export_include_directories

I belive @Karsten1987 meant to move ament_export_dependencies to the end of the file

Karsten1987 · 2020-11-23T18:53:29Z

transmission_interface/CMakeLists.txt

+#add_library(transmission_plugins SHARED
+#  include/transmission_interfrace/simple_transmission.h
+#)
+#target_include_directories(transmission_plugins PUBLIC include)
+#ament_target_dependencies(transmission_plugins hardware_interface tinyxml2_vendor TinyXML2)
+#ament_export_dependencies(transmission_plugins hardware_interface tinyxml2_vendor TinyXML2)
+#target_compile_definitions(transmission_plugins PRIVATE "TRANSMISSION_INTERFACE_BUILDING_DLL")
+


Can this be deleted?

transmission_interface/include/transmission_interface/simple_transmission.h

Karsten1987 · 2020-11-23T18:55:20Z

transmission_interface/test/simple_transmission_test.cpp

+  EXPECT_EQ(-1.0, trans.get_joint_offset());
+}
+
+// #ifndef NDEBUG


what's up with these tests?

jordan-palacios · 2020-11-24T11:00:49Z

transmission_interface/include/transmission_interface/simple_transmission.h

+
+    ActuatorHandle actuator_position = {"", "", nullptr};
+    ActuatorHandle actuator_velocity = {"", "", nullptr};
+    ActuatorHandle actuator_effort = {"", "", nullptr};


Please end your member variables with a "_".

jordan-palacios · 2020-11-24T11:05:42Z

transmission_interface/include/transmission_interface/simple_transmission.h

+    const std::vector<ActuatorHandle> & actuator_handles)
+  {
+    // check that joint / act names are identical
+    // check that joint interfaces and actuator interfaces match


Also check the actual handle is valid: handle name and interface are not empty.

jordan-palacios · 2020-11-24T11:07:34Z

transmission_interface/include/transmission_interface/simple_transmission.h

+    }
+
+//   *jnt_data.absolute_position[0] = *act_data.absolute_position[0] / reduction_ + jnt_offset_;
+//   *jnt_data.torque_sensor[0] = *act_data.torque_sensor[0] * reduction_;


Leftover comments?

wasn't sure what to do with these

jordan-palacios · 2020-11-24T11:17:46Z

transmission_interface/include/transmission_interface/simple_transmission.h

+  inline void SimpleTransmission::joint_to_actuator()
+  {
+    if (joint_effort && actuator_effort) {
+      actuator_effort.set_value(joint_effort.get_value() / reduction_);


I know this was present in the original implementation, but I'm wondering in which cases should the reduction be applied to the effort interface. E.g., it does not make sense if you are using current as effort.

What should we do here instead? Acceleration? Torque? same handling for both?

API

v-lopez · 2021-01-05T09:37:29Z

transmission_interface/include/transmission_interface/simple_transmission.hpp

+  /**
+   * \brief Transform \e effort variables from actuator to joint space.
+   * \param[in]  act_data Actuator-space variables.
+   * \param[out] jnt_data Joint-space variables.
+   * \pre Actuator and joint effort vectors must have size 1 and point to valid data.
+   *  To call this method it is not required that all other data vectors contain valid data, and can even remain empty.
+   */
+  void actuator_to_joint() override;
+
+  /**
+   * \brief Transform \e effort variables from joint to actuator space.
+   * \param[in]  jnt_data Joint-space variables.
+   * \param[out] act_data Actuator-space variables.
+   * \pre Actuator and joint effort vectors must have size 1 and point to valid data.
+   *  To call this method it is not required that all other data vectors contain valid data, and can even remain empty.
+   */
+  void joint_to_actuator() override;


It is a bit confusing that these methods have documented param[in] and param[out] args, but they have no params, I have to assume that they operate on the handles provided in the configure, maybe we should mention this explicitely.

Good spotting, thanks!

v-lopez · 2021-01-05T09:38:19Z

transmission_interface/include/transmission_interface/simple_transmission.hpp

+  void configure(
+    const std::vector<JointHandle> & joint_handles,
+    const std::vector<ActuatorHandle> & actuator_handles) override;


We should document that it is expected that you can provide multiple Handles but only of different interfaces of the same joint. I did not understand it until I got to the implementation of this method.

Can we check this somehow to protect users from making errors?

@destogl it is checked and exceptions are thrown accurately reporting the issue. I'll add docs to reinforce

destogl

Sorr to be very picky. It seems there are some obsolete comments.

destogl · 2021-01-02T08:48:36Z

transmission_interface/CMakeLists.txt

 target_include_directories(transmission_parser PUBLIC include)
 ament_target_dependencies(transmission_parser hardware_interface tinyxml2_vendor TinyXML2)
-ament_export_dependencies(hardware_interface tinyxml2_vendor TinyXML2)
+ament_export_dependencies(transmission_parser tinyxml2_vendor TinyXML2)


I belive @Karsten1987 meant to move ament_export_dependencies to the end of the file

destogl · 2021-01-07T14:41:26Z

transmission_interface/include/transmission_interface/simple_transmission.hpp

+  void configure(
+    const std::vector<JointHandle> & joint_handles,
+    const std::vector<ActuatorHandle> & actuator_handles) override;


Can we check this somehow to protect users from making errors?

destogl · 2021-01-07T14:44:18Z

transmission_interface/include/transmission_interface/simple_transmission.hpp

+   * \pre Nonzero reduction value.
+   */
+  SimpleTransmission(
+    const double reduction,


Disclaimer: I am not a transmission expert, but...

What does "Reduction ratio" means? Is this ratio "actuator/joint" or "joint/actuator". this should be at least documented.

I think a better variable name would be (if correct):

Suggested change

const double reduction,

const double actuator_to_joint_ratio,

There is plenty of this on the class documentation but I also renamed the argument. For brevity, the member variable name didn't change.

destogl · 2021-01-07T14:45:16Z

transmission_interface/include/transmission_interface/simple_transmission.hpp

+   * \param[in]  act_data Actuator-space variables.
+   * \param[out] jnt_data Joint-space variables.


Where are those parameters?

Nowhere, removed the offending doc

destogl · 2021-01-07T14:48:19Z

transmission_interface/include/transmission_interface/simple_transmission.hpp

+: reduction_(reduction),
+  jnt_offset_(joint_offset)
+{
+  if (0.0 == reduction_) {


This is rather an unusual syntax.

Suggested change

if (0.0 == reduction_) {

if (reduction_ == 0.0) {

indeed, flipped

destogl · 2021-01-07T14:51:16Z

transmission_interface/include/transmission_interface/transmission.hpp

+   * \param[in]  act_data Actuator-space variables.
+   * \param[out] jnt_data Joint-space variables.


this is probably copied from earlier code?

Suggested change

* \param[in] act_data Actuator-space variables.

* \param[out] jnt_data Joint-space variables.

destogl · 2021-01-07T14:51:26Z

transmission_interface/include/transmission_interface/transmission.hpp

+  * \param[in]  jnt_data Joint-space variables.
+  * \param[out] act_data Actuator-space variables.


this is probably copied from earlier code?

Suggested change

* \param[in] jnt_data Joint-space variables.

* \param[out] act_data Actuator-space variables.

bmagyar · 2021-01-08T08:32:42Z

Thanks guys for the comments, I believe I should have addressed them all now. Let me know if there's anything else.

destogl

It looks good now.

* Remove compile warnings. * correct formatting.

bmagyar requested a review from Karsten1987 November 19, 2020 20:57

Karsten1987 reviewed Nov 19, 2020

View reviewed changes

bmagyar requested a review from destogl November 23, 2020 09:07

destogl reviewed Nov 23, 2020

View reviewed changes

Karsten1987 reviewed Nov 23, 2020

View reviewed changes

bmagyar force-pushed the add_simple_transmission branch from 5d49a43 to 1931f10 Compare November 23, 2020 20:00

jordan-palacios reviewed Nov 24, 2020

View reviewed changes

bmagyar force-pushed the add_simple_transmission branch from 1931f10 to 4640a8e Compare December 26, 2020 22:10

bmagyar added 6 commits December 26, 2020 22:12

Add SimpleTransmission and unit tests

986062d

Create local JointHandle and ActuatorHandle to be used by transmissions

2da070e

API

fix include order

689da76

add strange indents uncrustify wants but cpplint will hate

1908bfa

add validation for handles, extend testing

e44dc80

remove commented lib

0ad15e8

bmagyar force-pushed the add_simple_transmission branch from 4640a8e to 9b348db Compare December 26, 2020 22:23

end the war between uncrustify and cpplint

2448b7e

bmagyar force-pushed the add_simple_transmission branch from 9b348db to baf4d1e Compare January 1, 2021 18:40

bmagyar requested review from destogl and jordan-palacios January 1, 2021 18:40

Update tests & add some more error handling

5135346

bmagyar force-pushed the add_simple_transmission branch from baf4d1e to 5135346 Compare January 1, 2021 18:41

v-lopez reviewed Jan 5, 2021

View reviewed changes

destogl requested changes Jan 7, 2021

View reviewed changes

Clarify documentation and naming re reduction direction

24e9f63

bmagyar requested review from destogl and v-lopez January 8, 2021 08:32

v-lopez approved these changes Jan 8, 2021

View reviewed changes

destogl approved these changes Jan 8, 2021

View reviewed changes

bmagyar merged commit 5bbd16c into ros-controls:master Jan 8, 2021

bmagyar mentioned this pull request Jan 28, 2021

Implement basic Transmission loading and SimpleTransmission #86

Closed

destogl added a commit to b-robotized-forks/ros2_control that referenced this pull request Aug 11, 2022

Remove compile warnings. (ros-controls#245)

554995e

* Remove compile warnings. * correct formatting.

	const double reduction,
	const double actuator_to_joint_ratio,

		* \param[in] act_data Actuator-space variables.
		* \param[out] jnt_data Joint-space variables.

		* \param[in] jnt_data Joint-space variables.
		* \param[out] act_data Actuator-space variables.

Conversation

bmagyar commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmagyar commented Nov 23, 2020

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Karsten1987 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

destogl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bmagyar commented Nov 19, 2020 •

edited

Loading