search

ArduPilot / ardupilot

ArduPlane, ArduCopter, ArduRover, ArduSub source
http://ardupilot.org/
GNU General Public License v3.0
10.19k stars 16.77k forks source link

Sub: SITL rc channels initialized wrong #8961

Closed jaxxzer closed 4 years ago

jaxxzer commented 5 years ago

We are using values from plane or something, everything should be initialized to 1500.

jaxxzer commented 4 years ago

The issue is here: https://github.com/ArduPilot/ardupilot/blob/Sub-3.5/libraries/AP_HAL_SITL/SITL_State.cpp#L466

These values only make sense for certain vehicles.

ArduSub is overwriting these initial values soon after startup here, so the effect is unnoticed as long as we have this call, but the issue with sitl rc channel initialization still stands: https://github.com/ArduPilot/ardupilot/blob/cb88bc7f53ad5aa27e9bb32d2121e320f724d198/ArduSub/control_manual.cpp#L11

I think the proper fix is to abstract the initial values here so each vehicle type can provide it's own.

peterbarker commented 4 years ago

On Tue, 10 Sep 2019, Jacob Walser wrote:

The issue is here: https://github.com/ArduPilot/ardupilot/blob/Sub-3.5/libraries/AP_HAL_SITL/SITL_State.cpp#L466

These values only make sense for certain vehicles.

ArduSub is overwriting these initial values soon after startup here, so the effect is unnoticed as long as we have this call, but the issue with sitl rc channel initialization still stands: https://github.com/ArduPilot/ardupilot/blob/cb88bc7f53ad5aa27e9bb32d2121e320f724d198/ArduSub/control_manual.cpp#L11

I think the proper fix is to abstract the initial values here so each vehicle type can provide it's own.

Where/when are those values being used?

Shouldn't those values be overwritten before they are used?

jaxxzer commented 4 years ago

Shouldn't those values be overwritten before they are used?

Not necessarily, but as written that's basically the requirement. If this is the case, why not just use zero? Initializing them with these values threw me off in the past when ardusub was not overwriting these values as you say.

I imagine if one were to adjust some initialization code, or change the default flight mode, debugging could ensue.

peterbarker commented 4 years ago

On Tue, 10 Sep 2019, Jacob Walser wrote:

Not necessarily, but as written that's basically the requirement. If this is the case, why not just use zero? Initializing them with these values threw me off in the past when ardusub was not overwriting these values as you say.

Yes, I've no idea why they're non-zero, and I can understand your confusion.

Are these being used somewhere before being overwritten?

jaxxzer commented 4 years ago

Are these being used somewhere before being overwritten?

Not so far as I've seen, but I'm only really up on what ardusub is doing in particular.

I presume the rc input driver is taking care of it for the other vehicles.

peterbarker commented 4 years ago

On Tue, 10 Sep 2019, Jacob Walser wrote:

  Are these being used somewhere before being overwritten?

Not so far as I've seen, but I'm only really up on what ardusub is doing in particular.

I presume the rc input driver is taking care of it for the other vehicles.

Yep. RC shouldn't be healthy before these are read. So perhaps we ought to remove the confusing code and put in (uint16_t)-5 or something in their places as a weird flag value?

jaxxzer commented 4 years ago

I think zero is the most suitable, unless that's a valid value or somehow not going to work.

peterbarker commented 4 years ago

On Tue, 10 Sep 2019, Jacob Walser wrote:

I think zero is the most suitable.

Flag values mean if somebody does use them then there's a chance they'll notice something odd.

jaxxzer commented 4 years ago

Well https://github.com/ArduPilot/ardupilot/pull/12278 was a flop.

Are these being used somewhere before being overwritten?

yes by plane sitl apparently (yet travis passed).

I don't know if this qualifies as an issue after all. It appears to be working according to @tridge expectation. Close this at will.