Skip to content

Conversation

Basiljamal1
Copy link

Summary

This pull request removes the check in export_reference_interfaces that compared the size of the reference_interfaces_

I might be wrong but it seems to me this check is not necessary. Removing it simplifies the code and avoids redundant runtime errors. All other interface export and validation logic remains unchanged.

For example, in my trapezoidal controller here, I previously had to resize the reference interfaces vector solely to satisfy this check, even though it was not functionally necessary. With this change, reference interfaces can be exported regardless of the vector size, simplifying controller implementations.

Please take a look and provide feedback 🙏

saikishor

This comment was marked as outdated.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about my earlier comment, It is expected that the reference_interfaces_ variable is used for exporting the reference, that's why this check is added.
Let's see what others say about this check

@Basiljamal1
Copy link
Author

Sorry about my earlier comment, It is expected that the reference_interfaces_ variable is used for exporting the reference, that's why this check is added. Let's see what others say about this check

Yup, I think using reference_interfaces_ is one valid way to export a reference. Similarly, in the hardware interface, systems can export a command interface either through the xacro file or directly within export_command_interfaces() by adding elements to std::vector<hardware_interface::CommandInterface>.

That said, I feel that throwing an exception here and terminating the application might be a bit too harsh. Throwing should generally be reserved for situations where the program encounters an unrecoverable error and doesn’t know how to proceed. Here, leaving reference_interfaces_ without resizing does not cause any side effects and the program is able to run normally without issues:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants