Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: effective matrix calculation from module to lib #24270

Closed
wants to merge 4 commits into from

Conversation

Perrrewi
Copy link
Contributor

@Perrrewi Perrrewi commented Jan 29, 2025

Solved Problem

Refactoring of control allocation started here.
This PR enable the tests and will unlock the computeEffectivenessMatrix in SIH.

Note that the refactoring should not affect the performance, only make it accessible as a library.

Test coverage

  • Unit/integration test: Yes
  • SIH test with QGC and AMC 34.12

@Perrrewi Perrrewi self-assigned this Jan 29, 2025
@Perrrewi Perrrewi requested a review from sfuhrer January 29, 2025 17:02
@Perrrewi Perrrewi marked this pull request as draft January 29, 2025 17:02
@Perrrewi Perrrewi requested a review from Jaeyoung-Lim January 29, 2025 17:03
Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

This is undoing the changes that were done in the previous PR

ActuatorEffectivenessUUV.cpp
ActuatorEffectivenessUUV.hpp
RpmControl.hpp
RpmControl.cpp
Copy link
Member

Choose a reason for hiding this comment

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

These were intentionally left as part of the control_allocator module, such that the control allocation library can stay generic.

In theory, there is no reason for the actuator effectiveness to be tied to vehicle types. The reason that this exists, is tied to how the control_allocator module is implemented. This is why I wanted to leave this out of the control_allocation library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed the description was wrong, it should have been computeEffectivenessMatrix that needs to be accessible for SIH.

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim Jan 30, 2025

Choose a reason for hiding this comment

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

I think this doesn't change my comment on the PR. Specific implementations of effectiveness matrix should not be part of the library. Only the base class should be part of the common lib. If SIH depends on specific versions of the actuatoreffectiveness implementation, it should use the VehicleActuatorEffectiveness library, which is built with control_allocator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I made some changes.

Only moved the most generic code to the library and let the vehicle specific stay in the VehicleActuatorEffectiveness module library.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why these changes are needed for enabling SIH. For me it seems as you are just moving the location of the implementation without any added value. The actuatoreffectiveness classes you have moved are still specific for the control_allocator, and I would prefer if they don't propagate to the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that there are other libraries inside modules, but in general, shouldn't we place shared content in a non-module library?

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim Jan 31, 2025

Choose a reason for hiding this comment

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

The reason I did not copy over the entire library was because the actuator effectiveness was not just actuator effectiveness semantically, but more of an implementation for the control allocator. Therefore I have only copied over the base class so that the control_allocation library is reusable without depending on how control_allocator works.

The main intention for this is that we want to be able to write our own control allocators e.g. metric control allocation.

If youtr intention is to fix the pseudo inverse test, we need to rewrite the test to not use to actuatoreffectivenessrotors.

If your intention is to use airframe specific control allocators in SIH, you should just use the library that contains that implementation, not move the module specific implementation to a generic library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I will close this PR and proposed a fix for fixing the unit test for now. Thanks for the review!

@Perrrewi Perrrewi force-pushed the pernilla/refactor-effective-matrix-calculation branch from 9a5aac0 to 44bce91 Compare January 30, 2025 17:41
@Perrrewi Perrrewi marked this pull request as ready for review January 31, 2025 09:01
@Perrrewi Perrrewi closed this Feb 3, 2025
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