Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use of rcmap library #8307

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

peterbarker
Copy link
Contributor

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
Copy link
Contributor Author

Still need to do parameter conversion.

@Naterater
Copy link
Contributor

Looking for this so that #685 can be closed.

@peterbarker peterbarker force-pushed the die-rcmap-die branch 5 times, most recently from 345e976 to ee84145 Compare April 2, 2019 00:26
@peterbarker peterbarker force-pushed the die-rcmap-die branch 3 times, most recently from a56b3da to ce88a4b Compare April 5, 2019 05:32
@peterbarker peterbarker changed the title Remove use of rcmap library from Copter Remove use of rcmap librar Apr 10, 2019
@peterbarker peterbarker force-pushed the die-rcmap-die branch 2 times, most recently from dff68e4 to 5d9e9cf Compare April 16, 2019 11:39
Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed below it's still missing param conversion.

libraries/RC_Channel/RC_Channel.h Outdated Show resolved Hide resolved
libraries/RC_Channel/RC_Channels.cpp Outdated Show resolved Hide resolved
libraries/RC_Channel/RC_Channel.h Outdated Show resolved Hide resolved
ArduCopter/GCS_Mavlink.cpp Outdated Show resolved Hide resolved
ArduCopter/mode.cpp Outdated Show resolved Hide resolved
ArduPlane/Parameters.cpp Outdated Show resolved Hide resolved
ArduPlane/Parameters.cpp Outdated Show resolved Hide resolved
Tools/autotest/common.py Outdated Show resolved Hide resolved
APMrover2/system.cpp Outdated Show resolved Hide resolved
libraries/RC_Channel/RC_Channel.h Outdated Show resolved Hide resolved
@peterbarker peterbarker force-pushed the die-rcmap-die branch 5 times, most recently from 2c6e357 to 29409cd Compare February 15, 2024 00:39
@davidbuzz
Copy link
Collaborator

'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... ?

Blimp/radio.cpp Outdated Show resolved Hide resolved
RC_Channel::AUX_FUNC::THROTTLE,
};
for (uint8_t i=0; i<ARRAY_SIZE(funcs); i++) {
const RC_Channel *c = rc().find_channel_for_option(funcs[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you've inspected each place we use the new find_channel_for_option() to be sure it checks for nullptr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually an existing problem with the codebase; if you set a bad value in rcmap you're not going to go anywhere until you wipe your parameters by uploading a different vehicle. I've just checked in SITL, and it certainly behaves that way. This is because radio.cpp just unconditionaly sets the pointers from the rcmap values and nobody wants to check those RC channel pointers for nullptr every time they're looked at!

I don't think this PR makes the problem worse, except that the RCn_OPTIONS are much more in-your-face as a user.

I think we should address the problem, but it's a little tricky as we like to be able to reset RCn_OPTION parameters and have them generally start to work. And certainly want that behaviour for RC input channels IMO!

So what I propose is that we only update the RC input channel pointers when disarmed, and if they do go to nullptr then go to the config error loop. And for always-armed planes/rovers/trackers we do the assignments exactly once while armed.

@rmackay9
Copy link
Contributor

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

@peterbarker
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants