-
Notifications
You must be signed in to change notification settings - Fork 1
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
Manual Control refactor #62
Conversation
…ethod return type
…alid RC), or vanilla
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## v1.13.1_dev #62 +/- ##
==============================================
Coverage ? 58.92%
==============================================
Files ? 958
Lines ? 83110
Branches ? 0
==============================================
Hits ? 48975
Misses ? 34135
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed I haven't reviewed exhaustively but this looks fine and I can't see any issues from a reasonable review of the changes. Also you've tested on the bench and the behaviour seems as intended - including timeouts etc. all working.
Next steps:
- You'll have a quick look at Manual Control tests - see if they can be extended easily to test our functionality.
- And before merging you'll do a test flight on the real drone in the air.
msg/sees_manual_control_data.msg
Outdated
uint64 timestamp # time since system start (microseconds) | ||
|
||
uint8 sees_desired_control_source # desired type of manual control source, RC (1) or Mav (2) | ||
uint64 valid_mavlink_setpoint_count # number of Mavlink connections providing setpoints (implying joystick connected). Should be < 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need uint64
if the expected count is less than 2? I think uint8
, similar to sees_desired_control_source
, should be sufficient.
msg/sees_manual_control_data.msg
Outdated
|
||
uint8 sees_desired_control_source # desired type of manual control source, RC (1) or Mav (2) | ||
uint64 valid_mavlink_setpoint_count # number of Mavlink connections providing setpoints (implying joystick connected). Should be < 2. | ||
uint64 valid_rc_setpoint_count # number of RC connections providing setpoints. Should be < 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to valid_mavlink_setpoint_count
, it might be good to change the type to uint8
if the integer value is not large so the message size is not too large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spots, thanks!
This PR is interlinked with the following SeesInterface2 and MAVSDK PRs:
Testing for this PR was completed here.
Problem(s)
This PR includes fixes and improvements for the following bugs:
Solution(s)
4467 - The QGC backup and RC control connection status incorrect
Here, data_link_lost was used by SI2 to monitor whether an instance of QGC (implied to be the Backup GCS) was connected or not.
However, data_link_lost considers any Mavlink connection as a valid data link; for example, it does not distinguish between RTK Sender and QGC.
Instead, we now count the number of manual_control_inputs (manual_control_setpoint candidates) that are live and are from a Mavlink source which is published to a new topic called "sees_manual_control_data".
Mavlink connections must have a joystick connected (can be virtual) to provide a setpoint, and our configuration should only ever have one Mavlink Joystick connected; the Backup GCS.
Subsequently, other 'passive' Mavlink connections are ignored in this count (RTK QGC, Mavlink Router, LDS...).
This also allows us to monitor if there are any additional unexpected Mavlink Joysticks connected that may cause conflict (e.g accidental connection from Van Backup GCS).
The number of RC manual_control_inputs is also logged but is not currently used for anything.
We also now pass the desired control source (RC or Mav) as this is more informative in the way we now toggle between sources.
4385 - RC Kill switch does not work if in “No Source” state
Previously, the RC switches would only be executed if the current selected input was valid and its source was RC.
This PR moves the RC switch execution process to a separate method which can then be called under 2 different conditions:
3792 - PX4 doesn't switch to local RC on Taranis mode change.
Now that RC switches always work (see 4385 above), we've added a toggle to RC Control when the RC flight mode switch is changed.
4401 - MAV/RC transition info message incorrect
This was a small bug where a datatype was wrongly set to Bool instead of Int. This resulted in the feedback always equalling RC (1) even when it should be Mav (2).
Additional improvements: