missing default `read` implementeation for `HardwareComponentInterface`

[christianrauch]

Describe the bug
HardwareComponentInterface::read is purely virtual while HardwareComponentInterface::write has a default implementation returning OK. This is inconsistent, as both should either be unimplemented or default to OK.

By the current design, an ActuatorInterface has to implement read, while a SensorInterface does not have to implement a write. In fact, SensorInterface::write overrides HardwareComponentInterface::write with the exact same behaviour of return return_type::OK;.

To Reproduce
Steps to reproduce the behavior:

1. implement a ActuatorInterface
2. you have to implement a ActuatorInterface::read that does nothing

Expected behavior
HardwareComponentInterface::read and HardwareComponentInterface::write should both be either be purely virtual or default implemented. ActuatorInterface::read should be default implemented either via HardwareComponentInterface::read if this has a default implementation, or by a custom default implementation similar to SensorInterface::write.

Screenshots
If applicable, add screenshots to help explain your problem.

Environment (please complete the following information):

• OS: Ubuntu
• Version jazzy

Additional context
Add any other context about the problem here, especially include any modifications to ros2_control that relate to this issue.

[christophfroehlich]

ros2_control/hardware_interface/include/hardware_interface/hardware_component_interface.hpp

Lines 655 to 667 in da81eb1

	/// Write the current command values to the hardware.
	/**
	* The physical hardware shall be updated with the latest value from
	* the exported command interfaces.
	*
	* \param[in] time The time at the start of this control loop iteration
	* \param[in] period The measured time taken by the last control loop iteration
	* \return return_type::OK if the read was successful, return_type::ERROR otherwise.
	*/
	virtual return_type write(const rclcpp::Time & /*time*/, const rclcpp::Duration & /*period*/)
	{
	return return_type::OK;
	}

vs

ros2_control/hardware_interface/include/hardware_interface/hardware_component_interface.hpp

Lines 607 to 617 in da81eb1

	/// Read the current state values from the hardware.
	/**
	* The data readings from the physical hardware has to be updated
	* and reflected accordingly in the exported state interfaces.
	* That is, the data pointed by the interfaces shall be updated.
	*
	* \param[in] time The time at the start of this control loop iteration
	* \param[in] period The measured time taken by the last control loop iteration
	* \return return_type::OK if the read was successful, return_type::ERROR otherwise.
	*/
	virtual return_type read(const rclcpp::Time & time, const rclcpp::Duration & period) = 0;

@soham2560, was there a specific reason for this decision? or was this the current state even before the unification?

[christophfroehlich]

By the current design, an ActuatorInterface has to implement read, while a SensorInterface does not have to implement a write. In fact, SensorInterface::write overrides HardwareComponentInterface::write with the exact same behaviour of return return_type::OK;.

This is intended as a Sensor has no command interfaces and thus no need for a write() method.

[christophfroehlich]

I'd even leave the default write overload here, because it might happen that a system_interface does not need a write() method

ros2_control/hardware_interface/include/hardware_interface/actuator_interface.hpp

Line 67 in da81eb1

return_type write(const rclcpp::Time & time, const rclcpp::Duration & period) override = 0;

ros2_control/hardware_interface/include/hardware_interface/system_interface.hpp

Line 69 in da81eb1

return_type write(const rclcpp::Time & time, const rclcpp::Duration & period) override = 0;

[soham2560]

@soham2560, was there a specific reason for this decision? or was this the current state even before the unification?

You've figured most of the stuff out but I'll clarify the thinking regardless

Before unification,

• System and Actuator Inteface
  • have to implement read and write (as you rightly quoted from da81eb1)
• Sensor Interface
  • have to implement read (write method is not provided)

So during unification we opted to make

• read compulsory for all
• and write optional for sensor, and then reimplement for the others with override = 0

Just to maintain conformity with the old system

this unification is just a stepping stone as the intention is to make do with the System, Sensor, Actuator paradigm and make it simpler for the end user to just write their interface. In the future, the job of whether to call read, write will be done on the basis of if the interface has state and command interfaces (basically only call read if state interface is there)

@christianrauch hope that clears some stuff up

[soham2560]

I'd even leave the default write overload here, because it might happen that a ros-controls/topic_based_hardware_interfaces#33

Ooh interesting usecase, using the default implementation might just be the way to go over here.

Ideally in fact later on when we remove the Actuator, System, Sensor paradigm completely, inheriting the deafult implementation for both would be the suggested way to go