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

prevent target_system=0 in toggle safety switch #3269

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Jan 5, 2024

a user tried to toggle safety on an antenna tracker and MissionPlanner sent it to target_system=0, which resulted in toggling safety on a flying aircraft, causing it to crash
Note that we really should make it a separate on/off buttons, as per this issue: #3268

This is what MissionPlanner sent:

2023-09-16 08:52:05.284 SET_MODE {target_system : 0, base_mode : 128, custom_mode : 1}
2023-09-16 08:52:05.308 SET_MODE {target_system : 0, base_mode : 128, custom_mode : 1}

I can only guess that MissionPlanner had not seen a SYS_STATUS from the antenna tracker when the button was pressed

a user tried to toggle safety on an antenna tracker and MissionPlanner
sent it to target_system=0, which resulted in toggling safety on a
flying aircraft, causing it to crash
@tridge tridge added the bug label Jan 5, 2024
@rmackay9
Copy link
Contributor

rmackay9 commented Jan 5, 2024

Thanks for this. I think this protection is a good idea.
I wonder if perhaps on this system, some device was transmitting using system id 0.

@EosBandi
Copy link
Collaborator

EosBandi commented Jan 6, 2024

I see something fishy here, target_system id in the toggle command comes from the currently connected AND selected vehicle. The only way that the button send out a command with a zero sysid if the antenna tracker (or plane) sysid was set to zero, which is a serious configuration error.
Having a zero sysid can cause several serious issues in other functions.
Shouldn't we handle this in the AP code ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to add a variable and cast the sysidcurrent to byte
if (MainV2.comPort.sysidcurrent == 0).... is just fine
also this check must be added to temp.cs at line 1188 (CTRL-F screen).
Instead of logging, a messagebox would be better to let the user know that the button did nothing...
(CustomMessageBox.Show("Not toggling safety on sysid 0")

@MichelleRos
Copy link

Shouldn't we handle this in the AP code ?

Perhaps a good idea to put a PreArm or something to guard against that error (which may not be what happened here), but I think it's definitely a good idea to have this in here too. It's still a bug for MP to send to the broadcast id something that is only intended for a single vehicle.

@meee1 meee1 merged commit c70e8a9 into ArduPilot:master Jan 12, 2024
7 checks passed
@ROSStargh
Copy link
Contributor

ROSStargh commented Mar 21, 2024

I've experienced a crash 2 years ago when toggling safety switch under control-F. Multiple drones were connected to GCS. One was in the air, and the others were on the ground. I was trying to take off another drone.

There was no obvious indicator that shows the state of the safety switch. And the command had effect on every drone on the link. So I had no idea all drones had their safety switch already disabled.

It looks like the cause is that the command was sent to target_system=0.

4ec17ff6093a8556ff89373586f7628a095af31d.png

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

Successfully merging this pull request may close these issues.

6 participants