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

Fix uncalibrated RC input being used #21543

Merged
merged 3 commits into from
May 23, 2023
Merged

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented May 3, 2023

Solved Problem

When testing for #21506 I often reset all parameters to go through a fresh setup and found that my RC input data from a crossfire receiver was used even though I didn't calibrate it yet. The values were all static zero because it was uncalibrated but still assuming to have pilot input even though there is none is a safety concern.

Solution

  • (Refactor topic object naming in RCInput.)
  • Start using the valid flag in the manual_control_input topic to signal that the input is not actually calibrated and shouldn't be considered valid and used. The RC is considered calibrated if the channel count parameter RC_CHAN_CNT is set and at least one of the stick axes roll, pitch, yaw or throttle is mapped to an RC channel. Further the input is only considered valid if the number of channels in it actually matches what was calibrated (RC_CHAN_CNT).

Changelog Entry

For release notes:

Bugfix: Uncalibrated remote input is now considered invalid

Alternatives

What's not ideal in my eyes is the reporting to the user. He cannot screw up anymore by accidentally using uncalibrated stick input but he's not informed about the details about why the stick input does not get used. QGC shows a red remote icon but feedback could be better. Should I duplicate all these checks into the RC calibration check here?

I considered that but then we need to change the workflow of default RC configuration because it will immediately fail and you will not be able to fly without RC without disabling it. I think this is the way to go but I don't feel comfortable introducing this change for 1.14 since we don't have UI changes and RC disabled by default to go with it yet.

Test coverage

I did bench testing using the same setup that I found the issue with. On a fresh setup the manual_control_input is published with the uncalibrated values but flagged as invalid and therefore does not get selected/used.

Once the RC is successfully calibrated it gets flagged valid and is normally used.

@MaEtUgR MaEtUgR requested a review from bkueng May 3, 2023 10:17
@MaEtUgR MaEtUgR self-assigned this May 3, 2023
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-community-q-a-may-03-2023/31965/1

@MaEtUgR MaEtUgR force-pushed the maetugr/rc-uncalibrated-fix branch from 57cbcfb to aa728ef Compare May 8, 2023 07:59
@MaEtUgR
Copy link
Member Author

MaEtUgR commented May 8, 2023

For now I removed the validity check for the channel count of every sample matching the calibration parameter. (#21543 (comment))
And fixed the manual control selector unit tests. I also added a new test that goes through invalid and valid flagged input to verify what changed.

@@ -155,6 +154,12 @@ void RCUpdate::parameters_updated()

update_rc_functions();

_rc_calibrated = _param_rc_chan_cnt.get() > 0
&& (_param_rc_map_throttle.get() > 0
Copy link
Member

Choose a reason for hiding this comment

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

What about a rover?

What it be enough to start with just RC_CHAN_CNT > 0 and RC_MAP_THROTTLE?

Copy link
Member Author

@MaEtUgR MaEtUgR May 9, 2023

Choose a reason for hiding this comment

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

I thought about that case. Just one of the 4 main functions needs to be mapped:
(channel count > 0) and (throttle or roll or pitch or yaw mapped)
Did I do a mistake?

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-maintainers-call-may-09-2023/32047/1

@MaEtUgR MaEtUgR force-pushed the maetugr/rc-uncalibrated-fix branch from 07bed33 to 4b76c3d Compare May 10, 2023 08:27
@MaEtUgR
Copy link
Member Author

MaEtUgR commented May 10, 2023

I rebased again in the hope Mac CI is more successful. In my eyes this pr is mergeable. Any objections?

@MaEtUgR MaEtUgR force-pushed the maetugr/rc-uncalibrated-fix branch from 4b76c3d to f573b6a Compare May 17, 2023 12:50
@MaEtUgR
Copy link
Member Author

MaEtUgR commented May 17, 2023

I rebased again. If no one objects I'll merge this later.

@MaEtUgR MaEtUgR merged commit c903288 into main May 23, 2023
@MaEtUgR MaEtUgR deleted the maetugr/rc-uncalibrated-fix branch May 23, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants