ardupilot
ardupilot copied to clipboard
Remove use of rcmap library
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 ;-)
Still need to do parameter conversion.
Looking for this so that https://github.com/ArduPilot/ardupilot/issues/685 can be closed.
Tests:
- [x]
RCMAP_parameters preserved across update (checkout master / setRCMAP_THROTTLEto 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
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 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.
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?
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.
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.
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.
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.
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.
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.
Tests:
- [x]
RCMAP_parameters preserved across update (checkout master / setRCMAP_THROTTLEto 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
Needs it's annual rebase. Would be nice to merge it and close the related issue too.
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.
Still need to do parameter conversion.
ive labeled it as wip due to this missing feature.
I force-pushed this one again!
I still very much like this change!
'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... ?
We should check that Mission Planner's RC calibration page is OK after this. I could probably do that.
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