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

Rot mech fix pr #3180

Closed
wants to merge 2 commits into from
Closed

Rot mech fix pr #3180

wants to merge 2 commits into from

Conversation

tmldeponti
Copy link
Contributor

This is to fix the depndency of the rot controller servo module (NPS) from INDI related variables. The IDX of the servo of the rotation mechanism can (and has) to be defined in the airframe, not dependent on assumptions regarding the number of INDI actuators.

@dewagter
Copy link
Member

@tmldeponti Thanks for the proposed update. This is indeed a problem!

I think that the existing code is writing outside the array (fortunately only in simulation, but still)!

Why can the simulator not simply use the same actuator IDX as the autopilot code? This way it would first of all not be outside the array (crucial), be similar to the flight code, but also not require extra defines.

@tmldeponti
Copy link
Contributor Author

@dewagter Indeed manually specifying an index without any checks can result in out of bounds. I also agree that the ap can be coded to use actuator_pprz[whatever_index_the_rot_mech_is] instead of wing_rotation_controller.servo_pprz_cmd as currently coded in the 3b airframe. I could though see some problems with that:

  1. How do we make sure that control codes that loop through the number of actuators do not consider the index of the rotation mech? Maybe something like looping through ACT_NUM -1 if rot_mech exists and assuming that it is the last actuator (which might not be the case)?
  2. The number of actuators still has to be defined as something independent of INDI (or my oneloop controller). I belive there should be a distinction between the total number of actuators and the actual number of actuator used in control allocation. Maybe, building on what i have done in the oneloop andi controller (see https://github.com/tudelft/paparazzi/blob/395ddc3df232cf3eaa4bc67e0f3dab3d454d38bc/conf/airframes/tudelft/rot_wing_v3c_oneloop_simulation.xml#L113), actuator number should have subcategories like :
    ANDI_NUM_REAL_ACT = 4; // four quad motors
    ANDI_NUM_VIRTUAL_ACT = 2; // Phi and Theta
    ANDI_NUM_PASSIVE_ACT = 1; // Rot Mech
    ANDI_NUM_ACT = ANDI_NUM_REAL_ACT + ANDI_NUM_VIRTUAL_ACT; // For control allocation loops
    ANDI_NUM_ACT_TOT = ANDI_NUM_ACT + ANDI_NUM_PASSIVE_ACT; // For definition of important arrays like actuator_pprz

ANDI can of course be removed from the name.
Let me know what you think and i can code a solution and commit it for further review.

@dewagter
Copy link
Member

Addresses #3179

@dewagter
Copy link
Member

I agree there must be a way to have any combination of

  • actuators set by control: e.g. INDI
  • actuators set by modules
  • actuators set by RC directly
  • virtual actuators

To me, in paparazzi, this list is called COMMANDS which in paparazzi already is super flexible and can do everything mentioned above.

I just think that adding defines about which element of the array to use is so terribly error prone it should not be the way to go. Here the index must match the first place after the actuators set by control, but should still fit in the array. This is all not checked and will lead to bugs.

One possible alternative is suggested in #3203 ... and if that is not working out we will need another way.

@dewagter dewagter closed this Dec 15, 2023
@dewagter dewagter deleted the rot_mech_fix_PR branch December 15, 2023 09:46
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.

None yet

2 participants