Bug in Mavlink MOUNT_CONTROL handling

[olliw42]

even though it is crystal clear that ArduPilot does not have any bug, these lines are wrong:

https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Mount/AP_Mount_Backend.cpp#L42

-> control(0.01f*(float)packet.input_a, 0.01f*(float)packet.input_b, 0.01f*(float)packet.input_c, _state._mode);

https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Mount/AP_Mount_Backend.cpp#L45

-> void AP_Mount_Backend::control(float pitch_or_lat, float roll_or_lon, float yaw_or_alt, MAV_MOUNT_MODE mount_mode)

https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Mount/AP_Mount_Backend.cpp#L58

-> set_angle_targets(roll_or_lon, pitch_or_lat, yaw_or_alt);

these changes imply some follow-up changes higher-up, which are easily found, and thus not listed

[peterbarker]

@olliw42 I'm feeling a bit slow ATM.

Are you suggesting that the call control(...) do the division rather than control(...) itself? (the line number you reference doesn't look like the line below it, so I'm guessing by -> you mean "should be")

AFAICS, with the current strange interface where we pass in either angles or lat/lon precludes us from pre-dividing these numbers.

[olliw42]

first off, experimentally one of the commands yields 100 times too low gimbal control (that's how I stumbled across it). I believe it is the COMMAND_LONG.
I'm away of this since a while, so my memory is weak, not sure I get it right now, but what I see is:

control() is called from COMMAND_LONG, which appears to be the new official to-be used mavlink, and the other is marked deprecated in the code, I thus would find it natural that control() provides the according interface, i.e. floats in degrees. The _GPS_POINT needs to be factored out, one would e.g. introduce a method to be called from COMMAND_LONG, which has floats too, which I would find natural too to do.
These methods would be used from the deprecated handlers, which would imply moving the division in the angles up.
The alternative would be to just multiply the angles by 100 in the COMMAND_LONG call of control(), but I would consider that a bit weird, since you would go float->int with *100 and then int->float with /100. And moreover one would give the deprecated the preference. So I suggested the former path in the above.

yes, the "->" point to what I think it should become

hope that helps a bit

Note, the issue wasn't there in AC3.5.x. It - so it appears - was introduces without properly considering the side-effects and without proper testing, and without following the "bread crumps". Maybe a general lesson to be learned here.
(and maybe a lesson for other mount stuff too ;) experimental facts are experimental facts ;) ).

[peterbarker]

On Mon, 29 Oct 2018, olliw42 wrote: first off, experimentally one of the commands yields 100 times too low

Ah ha! That's the key. I shall stare at the code paths - and, more usefully, write tests for each of them. Thanks! [many useful details snipped]

[olliw42]

once pointed at it I considered it obvious from the code
sorry if I wasn't clear

[amilcarlucas]

@peterbarker what is the status on this?

[IamPete1]

Looks like this was fixed by #9288, @peterbarker ?

[peterbarker]

@IamPete1 AFAIK this problem is as yet unsolved and we can't get to there from here. IIRC its basically a degrees vs centidegrees issue

[rmackay9]

@olliw42,

Maybe we can close this now? Again, sorry it took so long.

[olliw42]

closing it since resolved :)