ardupilot icon indicating copy to clipboard operation
ardupilot copied to clipboard

Remove use of rcmap library

Open peterbarker opened this issue 7 years ago • 21 comments

This is built on several the modeswitch-to-rcinput PR, #8170

There is still more to do to make everything nicely orthogonal, most notably the handling of RC6 specially in terms of deadzone handling, and the setting of ranges on only a selection of the auxillary channels.

The initialising of the ranges should probably be done based on the channel option assigned as part of init_aux_function() or init_aux_functions(). We should also rename that function given we're using it for the primary vehicle RC inputs!

One of the more interesting bits in here is the forcing of the 4 primary input channels to be defined. At the moment if you set RCMAP_THROTTLE to 89 and reboot your vehicle will have to wipe its parameters to make it functional again; we use these four channels pointers trustingly throughout the code. This force-setting is only one solution to the problem; we could also drop into something like the "you have a sensor problem" loop which BoardConfig has, or we could use a dummy RC_Channel and tell the user to fix things. Or we could check for nullptr here, there and everywhere.

Also of interest is the positioning of the throttle channel in the RC_Channels base class. Every vehicle has a throttle (coughAntennaTrackercough) - does it make sense to be in the base class? @WickedShell has been working on failsafes - would it help if it was? I'm currently leaning towards putting it back into RC_Channels_Copter.

Also not done is parameter conversion. PRs very welcome ;-)

peterbarker avatar May 05 '18 12:05 peterbarker

Still need to do parameter conversion.

peterbarker avatar Oct 23 '18 11:10 peterbarker

Looking for this so that https://github.com/ArduPilot/ardupilot/issues/685 can be closed.

Naterater avatar Jan 17 '19 23:01 Naterater

Tests:

  • [x] RCMAP_ parameters preserved across update (checkout master / set RCMAP_THROTTLE to 13 / reboot, make sure RC13_OPTION is set to 203)
  • [x] ensure rudder-only config works
  • [x] ensure IO processor mixer is programmed appropriately (set OVERRIDE_CHAN, use it to switch between mixer and ArduPilot use of RC input); surfaces should work the same as in manual.
  • [ ] ensure servo-channels upgrade works from ancient non-split code
  • [ ] ensure servo-channels upgrade works for modern-upgrade (i.e. setting default function values)
  • [x] ensure that if an RC channel isn't mapped then we can reboot and enter the config error loop

peterbarker avatar Dec 26 '19 04:12 peterbarker

RCMAP_ parameters preserved across update (checkout master / set RCMAP_THROTTLE to 13 / reboot, make sure RC13_OPTION is set to 203)

@peterbarker Was this tested in Copter?

OXINARF avatar Dec 26 '19 17:12 OXINARF

I think this change is going to cause a lot of pain. One obvious one is where users load an old parameter file and their RCn_OPTION values get reset to zero. We'll end up with very confused users.

tridge avatar Dec 26 '19 21:12 tridge

to me the most fundamental thing that isn't answered in this PR is what benefit it brings to our users? It will certainly bring pain, so where is the benefit that balances that?

tridge avatar Dec 26 '19 21:12 tridge

where is the benefit that balances that?

Consistency and fundamental logic in my opinion; RCMAP_ makes no fundamental sense since the addition of the RC_ parameters that more logically address the inputs to the system. Dividing the channel assignments between two separate sets of parameters that appear entirely unrelated is begging for misconfiguration.

For example, I somehow configured "flip" into an RC_Option for my throttle channel. Obviously it did nothing to the plane, but the separation of the parameters allowed it to happen, and at the time there were no checks to stop it. With every input being dictated by RC_ parameters, it is simpler and this makes total sense to me. Go to RC_ parameters for configuring inputs. Go to SERVO_ parameters to configure outputs.

I don't see how this is any more pain than the addition of the SERVO_ parameters back when plane 3.8 came out, but I also don't have a totally detailed understanding of what's happening here behind the scenes.

Naterater avatar Dec 26 '19 21:12 Naterater

RCMAP_ parameters preserved across update (checkout master / set RCMAP_THROTTLE to 13 / reboot, make sure RC13_OPTION is set to 203)

@peterbarker Was this tested in Copter?

I believe it was. I've now tested it as working in Copter, Rover and Plane.

peterbarker avatar Dec 27 '19 05:12 peterbarker

I think this change is going to cause a lot of pain. One obvious one is where users load an old parameter file and their RCn_OPTION values get reset to zero. We'll end up with very confused users.

This particular case could be avoided by a GCS checking for the presence of RCMAP_ parameters in the supported parameter list and not in the loaded-from-disk parameter list.

peterbarker avatar Dec 27 '19 05:12 peterbarker

I've tagged this for discussion on the dev call.

I suspect this change will cause some short-term pain but I also think it will ease the setup for the user in the medium and long term. As @Naterater says we current have a very non-obvious way of moving around the main control channels vs the auxiliary switches. If we make this change then the MP could have a nice setup page similar to what it has for the outputs. Users could then use this single screen to assign all the input channels whether that's roll, pitch, yaw, throttle, channel 6 tuning, auxiliary switches, etc.

I actually suspect that very few users know how to move around the main control channels.

rmackay9 avatar Dec 28 '19 02:12 rmackay9

At the moment if you set RC1_OPTION to 0 in Plane then we end up in the CPU failsafe code, which dereferences a null pointer for the roll channel. The board will reboot and end up in the config error loop before the CPU failsafe is installed and you can then reset the parameters. Not optimal. But better than master.

peterbarker avatar Dec 28 '19 06:12 peterbarker

TODO:

  • [ ] disallow arming without reboot when channel options are shifted around, or in some other way ensure angle vs range is correct. Potentially this could be a safety issue.

peterbarker avatar Jan 02 '20 14:01 peterbarker

Tests:

  • [x] RCMAP_ parameters preserved across update (checkout master / set RCMAP_THROTTLE to 13 / reboot, make sure RC13_OPTION is set to 203)
  • [x] ensure rudder-only config works
  • [x] ensure IO processor mixer is programmed appropriately (set OVERRIDE_CHAN, use it to switch between mixer and ArduPilot use of RC input); surfaces should work the same as in manual.
  • [x] ensure that if an RC channel isn't mapped then we can reboot and enter the config error loop

peterbarker avatar Jan 07 '20 00:01 peterbarker

Needs it's annual rebase. Would be nice to merge it and close the related issue too.

auturgy avatar Feb 08 '21 09:02 auturgy

On Mon, 8 Feb 2021, auturgy wrote:

Needs it's annual rebase. Would be nice to merge it and close the related issue too.

I'll push ahead with this post-4.1 - way too big a change at this point.

peterbarker avatar Feb 08 '21 09:02 peterbarker

Still need to do parameter conversion.

ive labeled it as wip due to this missing feature.

davidbuzz avatar Jul 21 '21 23:07 davidbuzz

I force-pushed this one again!

peterbarker avatar Feb 12 '24 06:02 peterbarker

I still very much like this change!

rmackay9 avatar Feb 12 '24 23:02 rmackay9

'convert_rcmap_parameters' - seems to have parameter conversion code now.... :-) I think Peter's gonna have to tell us how ready he thinks it is now... ?

davidbuzz avatar Feb 15 '24 23:02 davidbuzz

We should check that Mission Planner's RC calibration page is OK after this. I could probably do that.

rmackay9 avatar Feb 26 '24 23:02 rmackay9

We should check that Mission Planner's RC calibration page is OK after this. I could probably do that.

It's not - I've created this branch which compiled, but I haven't tested the created artifacts yet

https://github.com/ArduPilot/MissionPlanner/compare/master...peterbarker:MissionPlanner:die-rcmap-die?expand=1

peterbarker avatar Feb 27 '24 12:02 peterbarker