Extend interfaces melodic #395

bmagyar · 2019-12-02T09:02:24Z

Adds absolute_position and torque_sensor data types to JointStateHandle and ActuatorStateHandle, as well as the corresponding transformations to Transmissions
Changes the way velocity joint saturation limits are calculated: It now uses previous commanded velocity rather than measured velocity.
Exposes raw data pointers in JointStateHandle and JointCommandHandle. Previously only available in actuator handles.
Adds a new JointModeHandle for switching a hardware interface between different controller modes

matthew-reynolds

These changes should all be run through an auto-formatter. There are a lot of stylistic differences between the new changes and the rest of the codebase.

I realized while going through some of the comments I had were about established patterns. This is already a fairly large PR, so perhaps it makes more sense to stick with the established patterns and those changes should be pulled out into follow-ups (ie. replacing 0 with nullptr, adding consts all over the place, etc.)

hardware_interface/include/hardware_interface/actuator_state_interface.h

hardware_interface/include/hardware_interface/internal/resource_manager.h

transmission_interface/include/transmission_interface/differential_transmission.h

transmission_interface/include/transmission_interface/transmission.h

transmission_interface/include/transmission_interface/transmission_interface_loader.h

transmission_interface/src/differential_transmission_loader.cpp

bmagyar · 2019-12-30T00:56:42Z

@matthew-reynolds could you please give it another pass? I think it should be ready now

The feedback from the controller is way too slow to be used on an actual robot. A robot that had 15 rad.s^-2 on each wheel as an acceleration limit could not even reach 2 rad.s^-2 This is in line with ros_controllers#23

matthew-reynolds

There's a breaking change in the DifferentialTransmission constructor that should be fixed up. Once that's fixed up, there are a couple of style nitpicks but LGTM.

Beyond the code itself, I've got a couple questions about the usages and need for these changes, but I suspect you've already thought them through and I'll understand better once some more documentation is present. Don't want to hold up the PR while I'm getting conceptually on the same page, since the code itself looks good.

We should definitely get some documentation going that explains:

effort vs torque_sensor and pos vs absolute_position
The usage of the JointModeHandle (Once it's fleshed out)
What ignore_transmission_for_absolute_encoders does in DifferentialTransmission and how the loader can read it

Also, for changelogs down the road, it should be pointed out that this mega-PR:

Adds absolute_position and torque_sensor data types to JointStateHandle and ActuatorStateHandle, as well as the corresponding transformations to Transmissions
Changes the way velocity joint saturation limits are calculated (Now uses prev commanded vel rather than measured vel).
Exposes raw data pointers in JointStateHandle and JointCommandHandle (Previously only in actuator handles)
Adds a new JointModeHandle "for switching a hardware interface between different controller modes"

(Might be worth updating the PR description so that people reading this in the future don't need to go digging through comments)

Also, a final comment: Why torque_sensor rather than torque?

hardware_interface/include/hardware_interface/joint_mode_interface.h

hardware_interface/include/hardware_interface/joint_state_interface.h

transmission_interface/include/transmission_interface/differential_transmission_loader.h

matthew-reynolds · 2019-12-30T01:24:20Z

transmission_interface/include/transmission_interface/differential_transmission.h

+   * \pre Actuator, joint position and torque sensor vectors must have the same size.
+   */


Could be missing something, but is there a reason we don't have the same "To call this method it is not required that all other data vectors contain valid data, and can even remain empty" blurb here?

my mistake, fixed!

transmission_interface/include/transmission_interface/differential_transmission.h

transmission_interface/include/transmission_interface/simple_transmission.h

transmission_interface/include/transmission_interface/transmission.h

transmission_interface/src/differential_transmission_loader.cpp

hardware_interface/include/hardware_interface/joint_mode_interface.h

transmission_interface/include/transmission_interface/differential_transmission.h

bmagyar · 2019-12-31T01:04:51Z

Also, a final comment: Why torque_sensor rather than torque?

For lack of a better alternative, this is how we try to show that this is a read-only value, not something ppl can use for commanding.

…ters

bmagyar · 2019-12-31T01:20:52Z

I think I did all the requested changes, back to you ;)

matthew-reynolds

Thanks for reworking the new DifferentialTransmission constructors, with that squared away looks good to me!

Looks good to merge, a couple tiny outstanding things:

this comment about actuatorToJointTorqueSensor()'s function comment was not answered
this and this comment were marked resolved but the code wasn't updated. Not a big deal, especially if we plan on running a formatter soonish.

Also, a final comment: Why torque_sensor rather than torque?

For lack of a better alternative, this is how we try to show that this is a read-only value, not something ppl can use for commanding.

Got it. Feels a bit inconsistent with absolute_pos, since they're both read-only sensors, but absolute_position_sensor is certainly quite wordy.

…djust tests Add pointer accessors for torque sensor and absoute position encoders

bmagyar · 2020-01-02T18:52:43Z

The outstanding ones should be pushed, thanks for spotting them!

matthew-reynolds · 2020-01-02T20:53:53Z

👍 Looks great!

bmagyar · 2020-01-03T00:16:01Z

Thanks a lot for the reviews!

matthew-reynolds · 2020-02-07T23:46:08Z

Looking back at this again:

Did something change since pal-robotics-forks/ros_controllers#41 (comment)? From @adolfo-rt in that thread, Feb 2015:

We've come to the conclusion that having controllers for handling joint mode switches is suboptimal

I'm wondering if there has been further discussion since then that prompted the joint_mode_interface to be merged in here, or if it should be pulled it back out.

bmagyar · 2020-02-08T17:35:23Z

It wasn't an accident. After that conclusion, we also came to the realization that companies working with ros_control shouldn't keep internal forks solving this problem which forces them to ship their robots with custom, partially upstream-compatible ROS distributions.

Suboptimal is better than no solution at all.

matthew-reynolds · 2020-02-08T17:47:11Z

Got it - Thanks for clarifying :)

bmagyar force-pushed the extend-interfaces-melodic branch 5 times, most recently from 31aa5e8 to 9e3e40e Compare December 12, 2019 08:50

martiniil mentioned this pull request Dec 20, 2019

Extend joint state interface by commanded position #400

Open

matthew-reynolds reviewed Dec 20, 2019

View reviewed changes

bmagyar force-pushed the extend-interfaces-melodic branch 5 times, most recently from a7fbbd2 to 60d67db Compare December 30, 2019 00:14

bmagyar force-pushed the extend-interfaces-melodic branch from 60d67db to 550190f Compare December 30, 2019 01:13

Paul Mathieu and others added 4 commits December 30, 2019 01:24

joint_limits: use an open-loop policy for velocity staturation

402bb94

The feedback from the controller is way too slow to be used on an actual robot. A robot that had 15 rad.s^-2 on each wheel as an acceleration limit could not even reach 2 rad.s^-2 This is in line with ros_controllers#23

Created new hardware interface for switching between controller modes

c1a69bc

hardware_interface: fix initialization order

63c6ff8

Fix argument types to use enum

551b2d5

bmagyar force-pushed the extend-interfaces-melodic branch from 550190f to 7e76196 Compare December 30, 2019 01:24

matthew-reynolds reviewed Dec 30, 2019

View reviewed changes

matthew-reynolds mentioned this pull request Dec 30, 2019

Use nullptr #405

Merged

bmagyar force-pushed the extend-interfaces-melodic branch from 7e76196 to 78112b9 Compare December 31, 2019 01:02

Modified structures to have absolute encoder and torque sensor parame…

133d768

…ters

bmagyar force-pushed the extend-interfaces-melodic branch from 78112b9 to ebb481e Compare December 31, 2019 01:18

matthew-reynolds approved these changes Dec 31, 2019

View reviewed changes

Hilario Tome added 2 commits January 2, 2020 18:51

Add torque sensor and absolute encoder support to transmissions and a…

9592ad6

…djust tests Add pointer accessors for torque sensor and absoute position encoders

Extend joint mode interface

49512bf

bmagyar force-pushed the extend-interfaces-melodic branch from ebb481e to 49512bf Compare January 2, 2020 18:52

bmagyar merged commit f57bf3f into ros-controls:melodic-devel Jan 3, 2020

matthew-reynolds mentioned this pull request Jan 3, 2020

Use #pragma once #406

Merged

matthew-reynolds mentioned this pull request Feb 7, 2020

Something is wrong with the controller switching (tests) #418

Open

jordan-palacios mentioned this pull request Apr 8, 2020

Add joint interfaces and transmission design draft ros-controls/roadmap#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend interfaces melodic #395

Extend interfaces melodic #395

bmagyar commented Dec 2, 2019 •

edited

Loading

matthew-reynolds left a comment

bmagyar commented Dec 30, 2019

matthew-reynolds left a comment •

edited

Loading

matthew-reynolds Dec 30, 2019

bmagyar Jan 2, 2020

bmagyar commented Dec 31, 2019 •

edited

Loading

bmagyar commented Dec 31, 2019

matthew-reynolds left a comment

bmagyar commented Jan 2, 2020

matthew-reynolds commented Jan 2, 2020

bmagyar commented Jan 3, 2020

matthew-reynolds commented Feb 7, 2020

bmagyar commented Feb 8, 2020

matthew-reynolds commented Feb 8, 2020

		* \pre Actuator, joint position and torque sensor vectors must have the same size.
		*/

Extend interfaces melodic #395

Extend interfaces melodic #395

Conversation

bmagyar commented Dec 2, 2019 • edited Loading

matthew-reynolds left a comment

Choose a reason for hiding this comment

bmagyar commented Dec 30, 2019

matthew-reynolds left a comment • edited Loading

Choose a reason for hiding this comment

matthew-reynolds Dec 30, 2019

Choose a reason for hiding this comment

bmagyar Jan 2, 2020

Choose a reason for hiding this comment

bmagyar commented Dec 31, 2019 • edited Loading

bmagyar commented Dec 31, 2019

matthew-reynolds left a comment

Choose a reason for hiding this comment

bmagyar commented Jan 2, 2020

matthew-reynolds commented Jan 2, 2020

bmagyar commented Jan 3, 2020

matthew-reynolds commented Feb 7, 2020

bmagyar commented Feb 8, 2020

matthew-reynolds commented Feb 8, 2020

bmagyar commented Dec 2, 2019 •

edited

Loading

matthew-reynolds left a comment •

edited

Loading

bmagyar commented Dec 31, 2019 •

edited

Loading