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

AP_DDS: External Control enable #28429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tizianofiorenzani
Copy link
Contributor

@tizianofiorenzani tizianofiorenzani commented Oct 18, 2024

User Story

As an operator, I want to be able to completely cut off any external control from the DDS interface. This functionality is especially helpful during debugging tests, when a quick reaction is of utmost importance.
I would like to be able to assign a switch to Enable/Disable external control, as well as the ability for a script or an external GCS to trigger External Control.

What Changed

  • AP_ExternalControl has now the ability to set and retrieve its status.
  • AP_DDS prevents any Subscription or Service to affect the vehicle: Subscriptions callbacks are simply returned, while service requests reply with false.
  • RC_Channel has been given a new functionality DDS_EXTERNAL_CONTROL = 181 which allows a channel to enable External control when HIGH.

Test

  • Start SITL (plane or copter)
  • Call a mode switch or arm service call and verify that the call is successful.
ros2 service call /ap/mode_switch ardupilot_msgs/srv/ModeSwitch "mode: <mode_num>"
ros2 service call /ap/arm_motors  ardupilot_msgs/srv/ArmMotors "arm: true"
  • Set the RC switch to High ros2 topic pub /ap/joy sensor_msgs/msg/Joy "{axes: [0.0, 0.0, 0.0, 0.0, .nan, 1]}"
  • Assigned RC6_OPTION = 181
  • Notice that the GCS will show Command rejected: External Control Enabled.
  • Call other services and verify they are responded to succesfully.
  • Set the RC switch to Low ros2 topic pub /ap/joy sensor_msgs/msg/Joy "{axes: [0.0, 0.0, .nan, .nan, .nan, -1]}"
  • Try to Arm the vehicle with ros2 service call /ap/arm_motors ardupilot_msgs/srv/ArmMotors "arm: true"
  • Arming command is rejected!

image

Test with Mavproxy

Disable all the channels first and then set the rc channel dedicated to enabling the external control high.

rc all 65535
rc 6 1900

@tizianofiorenzani tizianofiorenzani marked this pull request as draft October 18, 2024 21:40
@Ryanf55 Ryanf55 self-requested a review October 18, 2024 22:02
@tizianofiorenzani
Copy link
Contributor Author

@Ryanf55 what is the best way to make it testable?

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Oct 19, 2024

@Ryanf55 what is the best way to make it testable?

I'd do something like a mock flyaway recovery.

  1. Take off
  2. Command 100% throttle on AP_DDS joystick topic
  3. Wait till drone reaches 400'
  4. Disable external control using mavlink
  5. Check it never hits 500'

Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

I've replicated your behavior, looks great! I think this is a nice clean way for a GCS to take authority from a companion computer.

Running SITL

ros2 launch ardupilot_sitl sitl_dds_udp.launch.py transport:=udp4 synthetic_clock:=True wipe:=False model:=quad speedup:=1 slave:=0 instance:=0 defaults:=$(ros2 pkg prefix ardupilot_sitl)/share/ardupilot_sitl/config/default_params/copter.parm,$(ros2 pkg prefix ardupilot_sitl)/share/ardupilot_sitl/config/default_params/dds_udp.parm sim_address:=127.0.0.1 master:=tcp:127.0.0.1:5760 sitl:=127.0.0.1:5501

mavproxy

param set RC6_OPTION 181
 reboot

Then use the CLI to test with joystick.

libraries/RC_Channel/RC_Channel.cpp Show resolved Hide resolved
libraries/AP_ExternalControl/AP_ExternalControl.h Outdated Show resolved Hide resolved
libraries/AP_ExternalControl/AP_ExternalControl.h Outdated Show resolved Hide resolved
@tizianofiorenzani tizianofiorenzani force-pushed the wips/servos branch 2 times, most recently from d3f0cf6 to 42c31a0 Compare October 29, 2024 15:12
@tizianofiorenzani tizianofiorenzani marked this pull request as ready for review October 29, 2024 15:13
@IamPete1
Copy link
Member

IamPete1 commented Nov 4, 2024

Could this be done at the top RC_Channels level rather than down in plane and copter? Then it should work for rover too.

ArduCopter/RC_Channel.cpp Outdated Show resolved Hide resolved
libraries/RC_Channel/RC_Channel.cpp Outdated Show resolved Hide resolved
@tizianofiorenzani
Copy link
Contributor Author

Suggestions have been applied, rebased and tested.

@tizianofiorenzani tizianofiorenzani force-pushed the wips/servos branch 2 times, most recently from 1ac2319 to 000b7f6 Compare November 5, 2024 15:52
Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

main concern is link flooding

libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
libraries/RC_Channel/RC_Channel.cpp Outdated Show resolved Hide resolved
@tizianofiorenzani
Copy link
Contributor Author

@tridge I missed that define in RC library. I applied all your suggestions and pushed again.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Needs to pass CI.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Nov 18, 2024

For dev call: Are we OK in proceeding with the approach to gate all external control data (from DDS) through the AP_ExternalControl library? This would include:

  • waypoint control
  • velocity control
  • arm, disarm
  • trajectory control
  • change mode
  • takeoff service (copter only)

Costs:

  • Flash
  • CPU
  • More code that's largely passthrough a library

Advantages:

  • Ability to take back control of an aircraft if there are bugs in the companion computer ROS code from a switch
  • Ability to manage external control in a single place which is more maintainable

mode_switch_response.curr_mode = AP::vehicle()->get_mode();

if (external_control->is_enabled()) {
Copy link
Collaborator

@Ryanf55 Ryanf55 Nov 21, 2024

Choose a reason for hiding this comment

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

This logic can get cleaned up once #28696 goes in. Either merge order is fine. I'll ensure all interfaces are gated by this interface regardless.

@davidbuzz
Copy link
Collaborator

In the real world, without DDS but with a companion, we gated our companion computer "doing stuff" behind two boolean truths: "is rc 5 channel set high", and "is the reported mode GUIDED", and only when it saw both of these "states" was it allowed to attempt control of the vehicle. RC-channel 5 was passed through from the ground-operator's transmitter switch through the pass-through, so a human-holding-rc-transmitter could always "turn off" external control, even in guided mode. ... AND it meant that the other way to turn-off external control was to be in any-other-flight-mode-except guided, which was trivial for GCS operator to do from there as well. ( eg, press rtl ). This particular scenario didnt need or use dds, but its a real-world example of enabling/disabling external control that works.

@tizianofiorenzani
Copy link
Contributor Author

tizianofiorenzani commented Nov 22, 2024

That means you limit your companion to only Guided mode. And you rely on the RC to be connected at all time. I don't exclude a priori the Companion to set any mode if necessary, and in this implementation we leave the External control open to be changed from GCS command as well.

@Ryanf55
Copy link
Collaborator

Ryanf55 commented Nov 22, 2024

That means you limit your companion to only Guided mode. And you rely on the RC to be connected at all time. I don't exclude a priori the Companion to set any mode if necessary, and in this implementation we leave the External control open to be changed from GCS command as well.

Yea, in DDS, we want to be able to all modes, all control methodologies, and even RC control over DDS. Having the companion computer read an RC signal seems like a reasonable approach, but that also assumes the companion computer software is still functional. If there's a serious problem, such as a deadlock in this RC reading part of the code, then it could keep sending bad commands to the autopilot and crash the vehicle.

By making the autopilot in control of gating commands, we are aiming to improve the safety of using a companion computer.

mode_switch_response.curr_mode = AP::vehicle()->get_mode();

if (external_control->is_enabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the best way to go about enabling and disabling external control? It's going to be a lot of these calls - flash and easy to mess up.

Did you consider putting this on the deserialize part instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you saying we reject any input data in AP_DDS if the aux function is triggered? If we did that, it would also affect things like AP_DDS visual odom, or state requests that don't change behavior of the autopilot such as pre_arm_check.

I was thinking about this yesterday, and it seems there's nothing the autopilot can do now to protect against bad visual odom data either other than the user switch to a mode that doesn't use it.

I'm open to alternatives that still satisfy the user story. The calls need to go somewhere, whether it's in AP_DDS, or here. Last dev call, the team seemed to be on board with the approach, specifically for blocking control commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I'm not going to block this as others have agreed to it.

But I'd suggest a whitelist of object_id.id which you check if control is disabled, checked before hitting the switch statement. Probably done as an additional switch statement in a separate method, use_object_id_when_control_disabled or something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yea, that seems like a good alternative. @tizianofiorenzani Can you try a separate implementation of that and compare the flash? It would be good to know which approach is better before we get too deep in adding all the rest of the interfaces.

AP_ExternalControl *external_control = AP_ExternalControl::get_singleton();
if (external_control != nullptr) {
if (ch_flag == AuxSwitchPos::HIGH) {
GCS_SEND_TEXT(MAV_SEVERITY_INFO, "RC: External Control Enabled");
Copy link
Contributor

Choose a reason for hiding this comment

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

Put these in RC_Channel::LookupTable RC_Channel::lookuptable instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that.

@peterbarker
Copy link
Contributor

Board                    AP_Periph  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeBlack                           128    *           144     56                136    136    128
CubeOrange-periph-heavy  *                                                                     
CubeOrangePlus                      128    *           184     168               136    152    128
CubeRedPrimary                      152    *           176     168               168    176    152
Durandal                            128    *           152     152               144    160    128
Hitec-Airspeed           *                                                                     
KakuteH7-bdshot                     152    *           168     176               32     168    152
MatekF405                           176    *           240     152               216    208    176
Pixhawk1-1M-bdshot                  168                216     200               184    176    168
f103-QiotekPeriph        *                                                                     
f303-Universal           *                                                                     
iomcu                                                                *                         
revo-mini                           176    *           208     200               192    200    176
skyviper-journey                                       168                                     
skyviper-v2450                                         264                                     

We really can't keep leaking flash like this - DDS isn't in play on these boards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

7 participants