move creation of stateinterfaces to systeminterface #9

mamueluth · 2023-12-07T09:04:52Z

Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:

Limited scope. Your PR should do one thing or one set of things. Avoid adding “random fixes” to PRs. Put those on separate PRs.
Give your PR a descriptive title. Add a short summary, if required.
Make sure the pipeline is green.
Don’t be afraid to request reviews from maintainers.
New code = new tests. If you are adding new functionality, always make sure to add some tests exercising the code and serving as live documentation of your original intention.

To send us a pull request, please:

Fork the repository.
Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
Commit to your fork using clear commit messages.
Send a pull request, answering any default questions in the pull request interface.
Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

setting/gettting

codecov-commenter · 2023-12-08T09:58:58Z

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

destogl · 2023-12-11T07:43:35Z

hardware_interface/include/hardware_interface/handle.hpp

@@ -116,6 +117,13 @@ class ReadWriteHandle : public ReadOnlyHandle
 class StateInterface : public ReadOnlyHandle
 {
 public:
+  explicit StateInterface(
+    const InterfaceDescription & interface_description, double * value_ptr = nullptr)


I am not sure if we actually need to extend this here. We are not storing InterfaceDescription, but extending this might be useful in the future to support other types. What we also don't need here is the third argument value_ptr. Did you plan to remove it in the future? I mean we require this for backward compatibility, but for new approach only InterfaceDescription should sufficient.

Looking into this, it might be useful to move the storage here, when using getter and setters, which means that we don't need this method, but more something like:

Suggested change

const InterfaceDescription & interface_description, double * value_ptr = nullptr)

const InterfaceDescription & interface_description)

This should for now store the variable with value, and later we move it into handles, for example:

const InterfaceDescription & interface_description) : ReadOnlyHandle(interface_description) // new method there see below) { value = std::limits<double>::quietNan(); // it might be that we can move this even in the ReadOnlyHandle, which is even better so we don't need to duplicate this code in the interfaces --> this means if there is no pointer, we create our own storage of values. value_ptr_ = &value; } // This method might be removed later when we remove the old functionality private: double value; // later we move this value into ... [this should go above in the `ReadOnlyHandle` explicit ReadOnlyHandle(const InterfaceDescription & interface_description) : prefix_name_(interface_description.prefix_name.), interface_name_(interface_description.interface_info.name), value_ptr_(nullptr) { }

Later changes within this file / handles are OK. Therefore, it is not so relevant where you move storage, but of course nicer the code the better. Only the changes of external API are critical.

working on this as a second step. Needs quite a lot of changes.

hardware_interface/include/hardware_interface/hardware_info.hpp

destogl · 2023-12-11T07:59:01Z

hardware_interface/include/hardware_interface/hardware_info.hpp

+ */
+struct InterfaceDescription
+{
+  InterfaceDescription(const std::string & prefix_name_in, const InterfaceInfo & interface_info_in)


This is a good idea to add this class, but please add also then type and size and so on. Maybe we have already something suitable here. What about InterfaceInfo structure?

Not sure what you mean, i already use InterfaceInfo but i created this since the unique name of the component is missing and ComponentInfo does store all command/state Interfaces which we don't want either. Or am i mistaken?

destogl · 2023-12-11T08:04:46Z

hardware_interface/include/hardware_interface/system_interface.hpp

    return CallbackReturn::SUCCESS;
  };

+  virtual void add_state_interface_descriptions()


Method name is not so good. Why don't you use the stateless method, this means doesn't access to the global variables? This parsing will repeat in all components, so maybe adding this into URDF parser or Hardware Info might be enough. Actually, we have this already in the InterfaceInfo class. OK, only sorting into different vectors is not there… Perhaps something be added on the parser level, as it is repeatable.

Cannot think of a better name. moved it to component parser

renamed it to import _<command/state>_interface_descriptions

destogl · 2023-12-11T08:05:07Z

hardware_interface/include/hardware_interface/system_interface.hpp

+    {
+      for (const auto & state_interface : joint.state_interfaces)
+      {
+        joint_states_descr_.emplace_back(InterfaceDescription(joint.name, state_interface));


Just write the full name, it is clearer.

Suggested change

joint_states_descr_.emplace_back(InterfaceDescription(joint.name, state_interface));

joint_states_description_.emplace_back(InterfaceDescription(joint.name, state_interface));

destogl · 2023-12-11T08:13:14Z

hardware_interface/include/hardware_interface/system_interface.hpp

+  double joint_state_get_value(const InterfaceDescription & interface_descr) const
+  {
+    return joint_states_.at(interface_descr.get_name());
+  }
+
+  void joint_state_set_value(const InterfaceDescription & interface_descr, const double & value)
+  {
+    joint_states_[interface_descr.get_name()] = value;
+  }
+
+  double joint_command_get_value(const InterfaceDescription & interface_descr) const
+  {
+    return joint_commands_.at(interface_descr.get_name());
+  }
+
+  void joint_command_set_value(const InterfaceDescription & interface_descr, const double & value)
+  {
+    joint_commands_[interface_descr.get_name()] = value;
+  }
+
+  double sensor_state_get_value(const InterfaceDescription & interface_descr) const
+  {
+    return sensor_states_.at(interface_descr.get_name());
+  }
+
+  void sensor_state_set_value(const InterfaceDescription & interface_descr, const double & value)
+  {
+    sensor_states_[interface_descr.get_name()] = value;
+  }
+
+  double gpio_state_get_value(const InterfaceDescription & interface_descr) const
+  {
+    return gpio_states_.at(interface_descr.get_name());
+  }
+
+  void gpio_state_set_value(const InterfaceDescription & interface_descr, const double & value)
+  {
+    gpio_states_[interface_descr.get_name()] = value;
+  }
+
+  double gpio_command_get_value(const InterfaceDescription & interface_descr) const
+  {
+    return gpio_commands_.at(interface_descr.get_name());
+  }
+
+  void gpio_commands_set_value(const InterfaceDescription & interface_descr, const double & value)
+  {
+    gpio_commands_[interface_descr.get_name()] = value;
+  }


This looks good!

mergify · 2024-01-08T13:10:50Z

This pull request is in conflict. Could you fix it @mamueluth?

mamueluth added 2 commits December 7, 2023 09:33

move creation of stateinterfaces to systeminterface

a8a3ab0

store description in state_interface and provide functions for

9931106

setting/gettting

destogl requested changes Dec 11, 2023

View reviewed changes

mamueluth added 6 commits December 11, 2023 10:20

return correct name for InterfaceDescription

88f744f

move parsing of interface description to component parser

8f73a83

adjusted actuator- and sensor_interface as well

c49735d

add first tests

0782cdb

adjust sensor_interface getting/setting and add tests

17f7a8f

add more tests

7d4d939

mamueluth mentioned this pull request Dec 19, 2023

Change interface export step 1 StoglRobotics-forks/ros2_control_demos#9

Draft

mamueluth closed this Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move creation of stateinterfaces to systeminterface #9

move creation of stateinterfaces to systeminterface #9

mamueluth commented Dec 7, 2023

codecov-commenter commented Dec 8, 2023

destogl Dec 11, 2023

destogl Dec 11, 2023

mamueluth Dec 12, 2023

destogl Dec 11, 2023

mamueluth Dec 11, 2023

destogl Dec 11, 2023

mamueluth Dec 11, 2023

mamueluth Dec 12, 2023

destogl Dec 11, 2023

destogl Dec 11, 2023

mergify bot commented Jan 8, 2024

	const InterfaceDescription & interface_description, double * value_ptr = nullptr)
	const InterfaceDescription & interface_description)

	joint_states_descr_.emplace_back(InterfaceDescription(joint.name, state_interface));
	joint_states_description_.emplace_back(InterfaceDescription(joint.name, state_interface));

move creation of stateinterfaces to systeminterface #9

move creation of stateinterfaces to systeminterface #9

Conversation

mamueluth commented Dec 7, 2023

codecov-commenter commented Dec 8, 2023

Welcome to Codecov 🎉

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jan 8, 2024