[BUG] Passing a new target to move() is sometimes ignored

[sylque]

In BLDCMotor::move(), if you pass a new target as a parameter, it is sometimes ignored.

This is due to this code being at the wrong place. I think it should be placed at the very top of the function.

Indeed, if either (motion_cnt++ < motion_downsample) or (!enabled), the new_target parameter is ignored.

[runger1101001]

You're right... no one has ever reported this before, I think because it is generally assumed that you either

• always pass the target value to the move function
• or never pass the target value via move, and instead just set motor.target

To see the bug you're experiencing you must be passing the value sometimes, and sometimes not. As a workaround, I suggest just using motor.target = x; to change the target, rather than using the move function.

In the next release, I think we'll probably place the target setting at the top of the function like you suggest to solve this edge case.

In a future version of the API, I would remove the parameter to move() completely, as the semantics are confusing, and either just setting the member field directly or using a setter function like motor.setTarget() would be clearer, I feel.

[runger1101001]

Solved for BLDCMotor, StepperMotor and HybridStepperMotor (drivers repository). DCMotor was not affected.

Merged to dev branch.

[sylque]

In a future version of the API, I would remove the parameter to move() completely, as the semantics are confusing

Agreed.

[askuric]

Thanks for this. This one is an old and sneaky one!

The parameter for move was the initial way to set the target value. We have pivoted from this paradigm some time ago with the motor.target. The parameter to move stayed no to break the API at first. And it stayed a bit too long :D