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

Add Logic to Check Result of COMMAND_LONG #782

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
109 changes: 90 additions & 19 deletions src/libs/vehicle/ardupilot/ardupilot.ts
@@ -1,3 +1,5 @@
import Swal from 'sweetalert2'

import { ConnectionManager } from '@/libs/connection/connection-manager'
import type {
MAVLinkMessageDictionary,
Expand All @@ -15,6 +17,7 @@ import {
MavMissionType,
MavModeFlag,
MavParamType,
MavResult,
MavState,
MavType,
} from '@/libs/connection/m2r/messages/mavlink2rest-enum'
Expand All @@ -34,6 +37,7 @@ import {
Altitude,
Attitude,
Battery,
CommandAck,
Coordinates,
FixTypeGPS,
Parameter,
Expand Down Expand Up @@ -116,6 +120,9 @@ export abstract class ArduPilotVehicle<Modes> extends Vehicle.AbstractVehicle<Mo
* @param {number} param5
* @param {number} param6
* @param {number} param7
* @param {boolean} waitForAck If the command should wait for an acknowledgment
* @param {boolean} noAlert Controls if swal alerts should not be shown
* @returns {Promise<CommandAck>} A promise that resolves with the command acknowledgment.
*/
sendCommandLong(
mav_command: MavCmd,
Expand All @@ -125,26 +132,73 @@ export abstract class ArduPilotVehicle<Modes> extends Vehicle.AbstractVehicle<Mo
param4?: number,
param5?: number,
param6?: number,
param7?: number
): void {
const command: Message.CommandLong = {
type: MAVLinkType.COMMAND_LONG,
param1: param1 ?? 0,
param2: param2 ?? 0,
param3: param3 ?? 0,
param4: param4 ?? 0,
param5: param5 ?? 0,
param6: param6 ?? 0,
param7: param7 ?? 0,
command: {
type: mav_command,
},
target_system: this.currentSystemId,
target_component: 1,
confirmation: 0,
}
param7?: number,
waitForAck = true,
noAlert = false
): Promise<CommandAck> {
return new Promise((resolve, reject) => {
// Construct the command
const command: Message.CommandLong = {
type: MAVLinkType.COMMAND_LONG,
param1: param1 ?? 0,
param2: param2 ?? 0,
param3: param3 ?? 0,
param4: param4 ?? 0,
param5: param5 ?? 0,
param6: param6 ?? 0,
param7: param7 ?? 0,
command: {
type: mav_command,
},
target_system: this.currentSystemId,
target_component: 1,
confirmation: 0,
}

this.write(command)
if (!waitForAck) {
resolve(new CommandAck())
return
}

let ackReceived = false
const ackHandler = (commandAck: CommandAck): void => {
console.log('Received command acknowledgment:', commandAck)
if (commandAck.command.type === mav_command) {
ackReceived = true
this.onCommandAck.remove(ackHandler)

this.write(command)
if (![MavResult.MAV_RESULT_ACCEPTED, MavResult.MAV_RESULT_IN_PROGRESS].includes(commandAck.result.type)) {
if (!noAlert) {
commandAck.alertMessage()
}
reject(new Error(`Command failed with result: ${commandAck.result.type}`))
Copy link
Member

Choose a reason for hiding this comment

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

We should receive an argument on the sendCommandAck to allow such validation done automatically, or allow the code to deal with the result manually.
It would be nicer if the returned object is not commandAck but a wrapper that allows dealing with it in a better way, including raising error/warning messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should receive an argument on the sendCommandAck to allow such validation done automatically, or allow the code to deal with the result manually.

Are you thinking of having another method that calls sendCommandLong and does the ack listening after the sendCommandLong method returns?

It would be nicer if the returned object is not commandAck but a wrapper that allows dealing with it in a better way, including raising error/warning messages.

What methods do you think should go in the object? Are there any other objects that are similar that I can use for reference?

Copy link
Member

Choose a reason for hiding this comment

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

Are you thinking of having another method that calls sendCommandLong and does the ack listening after the sendCommandLong method returns?

I'm talking about having just an optional argument for the sendCommandLong, for now that should be enough.

What methods do you think should go in the object? Are there any other objects that are similar that I can use for reference?

A function on the commandAck should be enough, an alert() that emits an Swal.fire would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know about the Swal.fire that seems better than as a text alert! I will add that and remove the current text alert I will also add the optional argument as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know what you think of the changes. I can tweak as needed

Copy link
Member

Choose a reason for hiding this comment

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

Just putting my 2 cents (I didn't check the whole logic as patrick is already taking care of that):

I think it's important that the one invoking the command method is informed about the result of the promise (even for timeouts), and should also have the option to decide whether or not a Swal popup should be fired in case of failure or timeout. Different interfaces will have different approaches on that. Some will prefer the automatic handling, and others will need more control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added another optional argument to suppress swals based on this

} else {
this.onCommandAck.remove(ackHandler)
resolve(commandAck)
}
}
}

this.onCommandAck.add(ackHandler)

// Setup a timeout for the ACK
const timeout = 5000
setTimeout(() => {
if (!ackReceived) {
if (!noAlert) {
Swal.fire({
title: 'Command timeout',
text: 'No acknowledgment was received for the command: ' + mav_command + ' in ' + timeout / 1000 + 's',
icon: 'error',
confirmButtonText: 'OK',
})
}
this.onCommandAck.remove(ackHandler)
reject(new Error('Timeout waiting for command acknowledgment'))
}
}, timeout)
})
}

/**
Expand Down Expand Up @@ -245,6 +299,23 @@ export abstract class ArduPilotVehicle<Modes> extends Vehicle.AbstractVehicle<Mo
this.onMAVLinkMessage.emit_value(mavlink_message.message.type, mavlink_message)

switch (mavlink_message.message.type) {
// command_ack
case MAVLinkType.COMMAND_ACK: {
const commandAck = mavlink_message.message as Message.CommandAck

const ack = new CommandAck({
command: commandAck.command,
result: commandAck.result,
progress: commandAck.progress,
resultText: commandAck.resultText,
targetSystem: commandAck.targetSystem,
targetComponent: commandAck.targetComponent,
})

// emit on command_ack
this.onCommandAck.emit_value(ack)
break
}
case MAVLinkType.AHRS2: {
const ahrsMessage = mavlink_message.message as Message.Ahrs2
this._altitude.msl = ahrsMessage.altitude
Expand Down
64 changes: 64 additions & 0 deletions src/libs/vehicle/types.ts
@@ -1,3 +1,7 @@
import Swal from 'sweetalert2'

import type { Type } from '@/libs/connection/m2r/messages/mavlink2rest'
import { MavCmd, MavResult } from '@/libs/connection/m2r/messages/mavlink2rest-enum'
import { AlertLevel } from '@/types/alert'

/**
Expand Down Expand Up @@ -100,6 +104,66 @@ export class Coordinates {
}
}

/**
* Command Acknowledgment
*/
export class CommandAck {
command: Type<MavCmd>
result: Type<MavResult>
progress: number
resultText: string
targetSystem: number
targetComponent: number

/**
* Creates an instance of CommandAck.
* @param {Partial<CommandAck>} [init]
*/
constructor(init?: Partial<CommandAck>) {
Object.assign(this, init)
}

/**
* Alert message
*/
public alertMessage(): void {
if (
this.result.type === MavResult.MAV_RESULT_TEMPORARILY_REJECTED ||
this.result.type === MavResult.MAV_RESULT_DENIED ||
this.result.type === MavResult.MAV_RESULT_UNSUPPORTED ||
this.result.type === MavResult.MAV_RESULT_FAILED
) {
Swal.fire({
title: 'Command Failed',
text: 'Command: ' + this.command.type + ' failed with result: ' + this.result.type,
icon: 'error',
confirmButtonText: 'OK',
})
} else if (this.result.type === MavResult.MAV_RESULT_ACCEPTED) {
Swal.fire({
title: 'Command Accepted',
text: 'Command: ' + this.command.type + ' succeeded with: ' + this.result.type,
icon: 'success',
confirmButtonText: 'OK',
})
} else if (this.result.type === MavResult.MAV_RESULT_IN_PROGRESS) {
Swal.fire({
title: 'Command In Progress',
text: 'Command: ' + this.command.type + ' is in progress',
icon: 'info',
confirmButtonText: 'OK',
})
} else {
Swal.fire({
title: 'Result Unknown',
text: 'Command: ' + this.command.type + ' result is unknown',
icon: 'error',
confirmButtonText: 'OK',
})
}
}
}

/**
* Body frame attitude
*/
Expand Down
2 changes: 2 additions & 0 deletions src/libs/vehicle/vehicle.ts
Expand Up @@ -6,6 +6,7 @@ import type {
Altitude,
Attitude,
Battery,
CommandAck,
Coordinates,
PageDescription,
Parameter,
Expand Down Expand Up @@ -75,6 +76,7 @@ export abstract class AbstractVehicle<Modes> {
onAltitude = new Signal<Altitude>()
onBatteries = new Signal<Battery[]>()
onCpuLoad = new Signal<number>()
onCommandAck = new Signal<CommandAck>()
onGenericVariables = new Signal<Record<string, unknown>>()
onMode = new Signal<Modes>()
onPosition = new Signal<Coordinates>()
Expand Down