SpinningBodyOneDOFStateEffector doesn't support locking multiple instances

[ascended121]

Describe the bug Basilisk provides an ArrayEffectorLockMsgPayload which has an array of flags, one for each of the maximum number of effectors. Presumably, each one of these flags is intended to correspond to a unique effector (ie, setting the first flag to 1 would lock the first effector, setting the second flag to 1 would lock the second effector, etc).

This does not appear to be the case. Every instance of spinningBodyOneDOFStateEffector appears to be locked by the first flag.

To reproduce Steps to reproduce the behavior:

1. Setup modules

solar_array1 = SpinningBodyOneDOFStateEffector()
...
sc_obj.addStateEffector(solar_array1)
sim.AddModelToTask(task_name, solar_array1)

solar_array2 = SpinningBodyOneDOFStateEffector()
...
sc_obj.addStateEffector(solar_array2)
sim.AddModelToTask(task_name, solar_array2)

lockArray = messaging.ArrayEffectorLockMsgPayload()
lockArray.effectorLockFlag = [1, 0, 0]
lockMsg = messaging.ArrayEffectorLockMsg().write(lockArray)
solar_array1.motorLockInMsg.subscribeTo(lockMsg)
solar_array2.motorLockInMsg.subscribeTo(lockMsg)

2. Add the following print to SpinningBodyOneDOFStateEffector::updateContributions:

std::cout << "Effector " << this->moduleID << ": " << this->lockFlag << std::endl;

3. Simulate.
4. See the following output:

Effector 1: 1
Effector 2: 1

Expected behavior The Lock message should've resulted in solar array 1 being locked, and solar array 2 being unlocked.

Screenshots N/A

Desktop (please complete the following information):

• OS: Ubuntu
• Version 22.04
• Python version 3.10.12

Additional context The problem appears to occur here. Every instance of spinningBodyOneDOFStateEffector looks at the first lock flag.

Instead, it appears each instance of spinningBodyOneDOFStateEffector needs to understand which effector it is, so that it can index the proper flag. Perhaps effectorID was intended for this purpose, though its not currently used for it.

[ascended121]

If a maintainer can confirm that the effector should indeed be using effectorID to determine which effector it is, then I'll be happy to submit a PR to make that change.

[joaogvcarneiro]

Hey @ascended121, the intended functionality is to have one message per effector, hence why only the first element is pulled each time. See how in the two-degree-of-freedom effector, we also only use one message but grab the first and second elements. This might become even more obvious when we introduce an N-degree-of-freedom effector.

I agree that the code might be a bit misleading, given that the length of the array corresponds to the maximum number of effectors, not the maximum number of degrees of freedom. I think that's something we can improve on.

[ascended121]

I agree that the code might be a bit misleading, given that the length of the array corresponds to the maximum number of effectors, not the maximum number of degrees of freedom. I think that's something we can improve on.

@joaogvcarneiro If you can suggest a change I can look into putting up a PR. Perhaps that variable should be called MAX_EFF_DOF? If so, some guidance on which modules/messages should be changed would be helpful.

When I see that variable in a message like MTBCmdMsgPayload I think of it as MAX_EFF_CNT MTB effectors, but perhaps you guys think of that as an MTB with MAX_EFF_CNT DOFs?

[joaogvcarneiro]

Whether or not it is a MAX_EFF_CNT or MAX_DOF_CNT is module-dependent. The original intent of using the maximum effector count is because some effectors, such as reaction wheels and coarse sun sensors, usually come in clusters. Therefore, these modules represent multiple effectors. However, they still use one input message per module, such as ArrayMotorTorqueMsgPayload, which is where the MAX_EFF_CNT comes from.

In the spinning bodies modules, this is not quite the case. Each module represents one effector, which can have multiple degrees of freedom. However, we still need a motor torque input message, which is why we also use ArrayMotorTorqueMsgPayload and why ArrayEffectorLockMsgPayload follows the same convention.

As it stands, it's an imperfect system. Reaction wheels and coarse sun sensors are special in that they represent clusters, but every other effector behaves differently. It feels like we're using this exceptional behavior for everything.

I don't know what the best approach is. We may need to create separate torque and lock messages containing only one variable, and we'd use N of them for effectors with N degrees of freedom. @patkenneally @schaubh, any thoughts?