-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
Implement transition to fw as an action. #150
Conversation
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.
Cool, thanks for the contribution!
I left comments concerning docs and naming.
Also, it would be good if you could add an integration test to try that in SITL.
And I'm assuming you also want to add a command to transition back?
plugins/action/action.h
Outdated
@@ -119,6 +119,13 @@ class Action | |||
Result return_to_launch() const; | |||
|
|||
/** | |||
* @brief Send command to *change flight mode* (FW). |
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.
Please describe what this does for an outsider that doesn't know the concept of flight modes and neither the acronym FW.
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.
And also comment that this is only applicable for hybrid/VTOL vehicles.
plugins/action/action.cpp
Outdated
@@ -42,6 +42,11 @@ Action::Result Action::return_to_launch() const | |||
return _impl->return_to_launch(); | |||
} | |||
|
|||
Action::Result Action::transition_to_fw() const |
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'm tempted to suggest to rename this to transition_to_fixedwing
. The goal is to have an easy to use, and self explanatory API. I would error on the side of verbosity.
@xgerrmann I'm to blame for failing CI. Working on it... |
@xgerrmann for your build problems: It should build just fine if you remove what you have installed, and just do |
|
@julianoes @hamishwillee |
Ok, does the transition happen when you try it in SITL? |
@xgerrmann Can you please remove the install folder that you checked in from this PR? |
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.
Please remove branchfile, install directory, best with git rebase -i
because the binary lib file should not be in the git history.
I've added an example now for the transition between multicopter and fixedwing. In the mean time I rebased on the latest head of the develop branch. I removed the install directory from the git, since I now know how to correctly install with 'make default install'. Timeouts still occur in the example, I switched off the 'return 1', so that the actions still are performed. So it seems like the requested behaviour is performed, but with timeout errors from MAVLINK. |
@xgerrmann once @julianoes is done with his review can you please do some cleanup with your commits? perhaps merging some of them with an interactive rebase, I want to encourage better commit titles and messages |
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.
There seem to be lots of changes not related to the intended changeset which makes it complicated to review for me @xgerrmann, if its ok with you @julianoes I'm gonna leave this one out to you to review, please ping me if you need my help
@mrpollo Ill clean it up >tomorrow<. So please don't bother checking until then. |
plugins/action/action.h
Outdated
@@ -119,6 +119,26 @@ class Action | |||
Result return_to_launch() const; | |||
|
|||
/** | |||
* @brief Send command to transition the drone to fixedwing. | |||
* | |||
* Note that this is only for the vtol type. |
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.
Suggest replace both of these points (and update similarly elsewhere) to:
The associated action will only be executed for VTOL vehicles in multicopter mode.
On other vehicles/modes the command will fail with aResult
.
I assume that the statement is correct - ie that the command will fail with an error?
I'm going to go so far as to suggest you abandon this PR and re-add to latest develop - a lazy man's rebase. There are just too many "accidental" changes in here for anyone to be sure it is safe to acccept. The method signatures look OK to me. I have added comment inline about document improvement. Can we also get current VTOL mode added to telemetry? @julianoes Would be good to hear your ideas on best way to do this - as part of a VTOL info class or something more generic? |
Ok guys, I squashed the commits. It's much cleaner now :)
|
@@ -187,6 +207,26 @@ class Action | |||
void return_to_launch_async(result_callback_t callback); | |||
|
|||
/** | |||
* @brief Send command to transition the drone to fixedwing (asynchronous). | |||
* | |||
* Note that this is only for the vtol type. |
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.
Argg, my comment from yesterday lost. Can you check your email for my suggested wording?
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.
Found it :) Next commit will include your comment.
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.
Thanks. Looks good. We will need to test that the comment is actually true once the timeout is fixed :-)
As per this note it is a good idea to first write integration tests BEFORE writing example code. You'll need them anyway! @julianoes I ran this, and the standard retries/timeout code is indeed being shown. What is interesting though is that the commands are being obeyed - ie the transition is happening. So either the ack is not being sent or it is being handled incorrectly. Such a pity that logging isn't enabled so we could check all the traffic and see if "something" is really coming back. Any ideas on debugging? |
In the latest commit, I've implemented an integrationtest.
|
@xgerrmann thanks a lot. I will look into the issues. I might not get to it today I'm sorry but definitely tomorrow. |
@@ -119,6 +119,26 @@ class Action | |||
Result return_to_launch() const; | |||
|
|||
/** | |||
* @brief Send command to transition the drone to fixedwing. | |||
* | |||
* The associated action will only be executed for VTOL vehicles in multicopter mode. |
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.
Thanks! This will need to be checked of course (once we have got the timeout issue fix). It may be that PX4 would simply ignore the command if sent while already in the current mode (rather than fail) and similarly it might ignore the command for non vtol rather than failing it.
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.
@julianoes So based on your change PX4/PX4-Autopilot#8264 would I be correct in saying that
- on a vtol vehicle the result will either be "success" or timeout, where success is an ack of message received, not that the vehicle will act on it? In other words, if you're already in FW and you call this it will just succeed?
- What will happen on an MC or FW only vehicle - will your added code still get run? If so, then basically this will return success if received by vehicle, which will be a bit odd on MC. Should our code therefore check the type of vehicle we're talking to before even sending the message?
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.
- on a vtol vehicle the result will either be "success" or timeout, where success is an ack of message received, not that the vehicle will act on it? In other words, if you're already in FW and you call this it will just succeed?
correct, it will just succeed.
- What will happen on an MC or FW only vehicle - will your added code still get run? If so, then basically this will return success if received by vehicle, which will be a bit odd on MC. Should our code therefore check the type of vehicle we're talking to before even sending the message?
It will just time out because the VTOL controller is not running and therefore not responding.
A check to see if it is actually a VTOL and a specific error message for it would be nice!
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.
@xgerrmann So
- Can we check this is VTOL and have a specific error message for other types (rather than getting timeout).
- So the text needs to change slightly as a result of Julian's feedback:
The associated action will fail with a Result if called for non-VTOL vehicles The action is ignored if called when the vehicle is in fixed-wing mode mode.
@julianoes What about if someone is using firmware pre your fix? Should we also have a PX4 version check?
The problem here is that even in an old version we always want to send the message, resulting in the timeout. But in an old version we want to ignore the ack result. To me that sounds like we want to have a version of our send that if we don't get an ack on we can force to succeed anyway. THoughts?
Without having actually tried it, using EXTENDED_SYS_STATE |
Ok, I'm debugging the missing ack and writing down my steps as suggested by @hamishwillee: I check out the PR:
I look for the new integration test:
Ok found it let's run it:
And of course run px4 latest master:
I can reproduce the missing ack:
Command Let's grep where this command is handled in Firmware:
This seems wrong because we're not interested in mission items. Let's look for just
Let's look at the controller itself first: Looks like the command is just not acked at all. Let's change that: |
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.
A couple of small comments, then we can merge this!
CMakeLists.txt
Outdated
@@ -143,7 +143,7 @@ add_library(dronecore ${library_type} | |||
target_link_libraries(dronecore | |||
${CMAKE_THREAD_LIBS_INIT} | |||
${curl_lib} | |||
tinyxml2 | |||
# tinyxml2 |
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.
Please don't change this stuff. Make sure to either merge or rebase on current develop and get rid of this diff.
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'd say rebase
else() | ||
add_definitions("-std=c++11 -WX -W2") | ||
set(platform_libs "Ws2_32.lib") | ||
endif() |
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.
Once you merge or rebase you will see that this file gets a lot simpler.
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.
The required changes are being documented in mavlink/MAVSDK-docs#62 (in progress). The interesting bit is here - basically we're changing to use system-wide install by default AND dronecore itself is now linking to the thread library - so you no longer need to do most of this work.
// Wait | ||
std::this_thread::sleep_for(std::chrono::seconds(10)); | ||
|
||
// Change to VTOL |
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 agree.
|
||
Device &device = dc.device(); | ||
|
||
// Takeoff |
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 would get rid of comments that are identical to a function name. They don't add any information. I suggest only to add comments if you need to explain something, add additional info for the reader, e.g. We need to takeoff first, otherwise we can't actually transition
😄
std::this_thread::sleep_for(std::chrono::seconds(wait_time_s)); | ||
|
||
|
||
EXPECT_GT(device.telemetry().position().relative_altitude_m, altitude_m - 0.25f); |
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.
Nice check, that will be useful once these integration tests make it into the PX4 Firmware testing.
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.
Thanks, but I have to give credit where due. I got it from the simple_hover example ;)
_parent->send_command_with_ack( | ||
MAV_CMD_DO_VTOL_TRANSITION, | ||
MavlinkCommands::Params {float(MAV_VTOL_STATE_FW)}, | ||
MavlinkCommands::DEFAULT_COMPONENT_ID_AUTOPILOT)); |
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.
Yeah, that's it. I like that you added both sync and async methods, thanks!
@julianoes I Really liked how you've written down your steps, this gives insight in your workflow and results in learning opportunities for people reading along (referring to myself here). |
device.action().transition_to_fixedwing(); | ||
|
||
// Wait a little before the transition back to multicopter, |
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.
And noting that we need to update to check transition once that is in place.
example and also implement async.
example and also implement async. Include an integration test.
@hamishwillee I really liked @julianoes short write up, can we find it a home in our docs? |
I already created mavlink/MAVSDK-docs#63 to track this. It is not as easy as you might think to turn this into useful generic advice. |
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.
👍 thanks for the contribution 💯
IMO there are a number of comments still not addressed.
|
// Wait | ||
std::this_thread::sleep_for(std::chrono::seconds(5)); | ||
|
||
// Return to launch |
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.
Actions in examples should be output to console rather than commented so that users have something to watch in the console. So "arming", "Taking off", "Landing", "Returning" etc.
* Implementation of multi-copter to fixed-wing and vice versa. * Include an example and also implement async. * Make integration test. * Add more descriptive prints and comments. * Remove non-informative comments. * run make fix_style * deleted zero byte file (was by mistake still present)
minor fixes for MAVSDK-Java autogen
Implementation of the transition to Fixed Wing from VTOL.
One problem: It only builds if the action header files in the install directory contain the correct definitions. I did not include these files since the directory is placed in the .gitignore.
Checks to perform prior to acceptance: