[Admittance] multiple state/command interfaces

[MarcoMagriDev]

This PR aims to address the following issues:

1. Fix wrong has flags used in write and read methods
2. Fix wrong fields of state_commanded used in write_state_to_hardware
3. Fix that multiple state interfaces are configured only the position state interface is considered due to else if statement.

Moreover:

1. Makes position state interface mandatory, otherwise the controller will not be able to perform computations. The controller will not be configured if not provided.
2. Adds compatibility with velocity command interface without position command interface.

[MarcoMagriDev]

Hi everyone, just a gentle reminder to review this PR when you get a chance. Let me know if anything needs clarification. Thanks!

[saikishor]

Hello!

Sorry for the late review. I have a comment on a change. Rest seems fine to me.
Is this fix tested on any hardware?

[MarcoMagriDev]

Hello!

Sorry for the late review. I have a comment on a change. Rest seems fine to me. Is this fix tested on any hardware?

At that time, I tested it on a UR10e robot using both the position command interface and the velocity command interface separately. That’s why I didn’t notice the missing pos_ind +. You’re absolutely right--let me fix this in a new commit.

[christophfroehlich]

There exists a newer PR #1434 addressing similar issues. Does it make sense to fix everything at once, and add proper tests for the multiple interfaces?

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.33%. Comparing base (ae6efeb) to head (e13863c).

Files with missing lines	Patch %	Lines
...dmittance_controller/src/admittance_controller.cpp	85.71%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
+ Coverage   84.27%   84.33%   +0.06%     
==========================================
  Files         123      123              
  Lines       11359    11390      +31     
  Branches      961      969       +8     
==========================================
+ Hits         9573     9606      +33     
+ Misses       1470     1466       -4     
- Partials      316      318       +2     

Flag	Coverage Δ
unittests	84.33% <91.66%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...nce_controller/test/test_admittance_controller.cpp	100.00% <100.00%> (ø)
...nce_controller/test/test_admittance_controller.hpp	94.79% <ø> (ø)
...dmittance_controller/src/admittance_controller.cpp	76.17% <85.71%> (+1.45%)	⬆️

... and 1 file with indirect coverage changes

[MarcoMagriDev]

There exists a newer PR #1434 addressing similar issues. Does it make sense to fix everything at once, and add proper tests for the multiple interfaces?

@christophfroehlich If you'd like, I can integrate those commits into my branch and push them to this PR. Let me know what you think!

[christophfroehlich]

There exists a newer PR #1434 addressing similar issues. Does it make sense to fix everything at once, and add proper tests for the multiple interfaces?

@christophfroehlich If you'd like, I can integrate those commits into my branch and push them to this PR. Let me know what you think!

If you approve the changes and they are related, yes please merge them into your PR. Do you have an idea for adding a proper test for this use-case?

[MarcoMagriDev]

Yes I think that they are somehow related to this PR since it deals with interfaces.

---

Planned Tests for This PR:

• Verify that if only the velocity state_interface is exported, the controller fails during configuration.
• Ensure that if only the velocity command_interface is exported, there are no indexing errors in the vectors.

Planned Tests for the Other PR (After Integrating Changes):

• Check that if a single reference_interface is exported, no errors are returned.

@christophfroehlich Do you have any other tests in mind that we should consider?

[christophfroehlich]

sounds good. if the error you have encountered and fixed is tested, we already have improved it

[christophfroehlich]

Thanks for adding tests and the other fixes. Failing tests are not related to this PR.

[saikishor]

LGTM
Just a minor comment
Thanks for adding tests

[saikishor]

Now, LGTM

[christophfroehlich]

one_init_parameter_is_missing is failing now

[MarcoMagriDev]

one_init_parameter_is_missing is failing now

one_init_parameter_is_missing was failing because we now define a default value for chainable_command_interfaces, whereas the test expected the controller to crash when this parameter was not explicitly provided.

I just pushed a new commit that removes this parameter from the list of tested ones in the test suite.