-
Notifications
You must be signed in to change notification settings - Fork 22
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 Takeoff and Land #627
Add Takeoff and Land #627
Conversation
Nice addition! |
Thanks @rafaellehmkuhl ! for reference I have been using this docker-compose file to test
|
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.
Eric, I have made some change requests that I think can improve on this patch.
Let me also say that it is super exciting receiving this contribution. It's cool to see the codebase being challenged with the addition of a vehicle very different then the ones it was originally built with.
Next thing I'm gonna do is install sitl to start trying quads with cockpit 😄
takeoff(): boolean { | ||
this.setMode(this.modesAvailable().get('GUIDED') as Modes) | ||
this.arm() | ||
this._takeoff(10) |
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.
Should we wait after setting guided mode and also after arming to send the takeoff command, or is it ok to send them continuously?
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.
That should be mostly ok
@@ -298,6 +300,8 @@ export abstract class ArduPilotVehicle<Modes> extends Vehicle.AbstractVehicle<Mo | |||
|
|||
this._isArmed = Boolean(heartbeat.base_mode.bits & MavModeFlag.MAV_MODE_FLAG_SAFETY_ARMED) | |||
this.onArm.emit() | |||
this._flying = heartbeat.system_status.type === MavState.MAV_STATE_ACTIVE |
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.
Is this MAV_STATE_ACTIVE shared between all vehicles? @ES-Alexander maybe can help answering that one.
If so, maybe we can have a better name that also works for subs/boats/rovers, but if we don't find one, flying is also ok. I usually say or subs are flying 😅
@rafaellehmkuhl Thanks for the feedback! I have off tomorrow, so I am planning on working through your comments tomorrow |
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.
Minor request and points.
/** | ||
* Takeoff the vehicle | ||
*/ | ||
function takeoff(): void { | ||
mainVehicle.value?.takeoff() | ||
} | ||
|
||
/** | ||
* Land the vehicle | ||
*/ | ||
function land(): void { | ||
mainVehicle.value?.land() | ||
} |
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.
I have mixed feelings with such functions on mainVehicle, that's not a blocker, but just something for us to have in mind for future rework.
The idea of cockpit is to be generic, the concept of takeoff and landing applies mostly to aerial vehicles, having such functions here in mainVehicle is a bit complicated.
I would recommend in the future for us to have a generic callback map that the vehicle could specify such "logic". We need to put more thought on it, there is now no way for us to check if the vehicle does support such functions.
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.
Yep. Same thoughts.
But as we don't have discussed yet how to go on that, I think we should just add that now and discuss changes after.
@ericjohnson97 I forgot to mention, but I saw the |
I was following the arm function as a guide. I was also originally hoping to have the command_ack drive the true false response, but I don't think I will any more.
would you like the functions to be void still? also I noticed that takeoff is still async, which I now want to change to just be synchronous. |
Got it. I think you can leave as Boolean and we can review them all together in the future. I've been trying to stick with the javascript approach (void + throw), but indeed some of our code still has a c++ similar approach (return Boolean). |
@rafaellehmkuhl I see. I am more familiar with the c++ methodology. I think the void throw method is fine. I'll change the functions and make takeoff synchronous now |
Cool! |
@rafaellehmkuhl Have fun! we can resume the review when you get back. |
The reason behind that is that a throw can get in an unhandle behavior and there is no checks on the typescript linters to see if you are not dealing with a thrown, creating unsafe code. |
Only the calling function will get unhandled, not the entire application. In this case, the takeoff button wouldn't call a takeoff and that's it. |
This merge request adds a mini widget that commands takeoff and land for an aerial vehicle.
closes #579