Add Logic to Check Result of COMMAND_LONG #782
Conversation
|
Nice! |
np. updated! |
|
@rafaellehmkuhl It would probably be good to make the alert text wrap, but that should probably be a different PR |
| this._statusText.text = `Command: ${mav_command} failed with result: ${commandAck.result.type}` | ||
| this._statusText.severity = AlertLevel.Error | ||
| this.onStatusText.emit() | ||
| reject(new Error(`Command failed with result: ${commandAck.result.type}`)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should receive an argument on the
sendCommandAckto 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
commandAckbut 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?
There was a problem hiding this comment.
Are you thinking of having another method that calls
sendCommandLongand does the ack listening after thesendCommandLongmethod 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Let me know what you think of the changes. I can tweak as needed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added another optional argument to suppress swals based on this
ericjohnson97
force-pushed
the
feature/command_ack
branch
2 times, most recently
from
545f0a6
to
9937643
Compare
|
|
||
| this.write(command) | ||
| if (![MavResult.MAV_RESULT_ACCEPTED, MavResult.MAV_RESULT_IN_PROGRESS].includes(commandAck.result.type)) { | ||
| commandAck.alertMessage() |
There was a problem hiding this comment.
Now that alertMessage has its own check for ack type, this should be done before the if line.
There was a problem hiding this comment.
I wanted to get rid of this check but, if I don't, a swal will be emitted on all command ACKs including the accepted commands. I thought of only having alerts for failed ACKs, but I thought it would be useful in certain cases to call the alert on an accepted ACK.
ericjohnson97
force-pushed
the
feature/command_ack
branch
from
9937643
to
7c82e9d
Compare
Notifies user when COMMAND_LONG fails.
2024-02-27.16-03-39.mp4
closes #628