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

Follow up for VTOL transition additions #163

Merged
merged 6 commits into from
Nov 16, 2017
Merged

Follow up for VTOL transition additions #163

merged 6 commits into from
Nov 16, 2017

Conversation

julianoes
Copy link
Collaborator

This follows up on #150 (comment).

@julianoes julianoes requested review from mrpollo and hamishwillee and removed request for mrpollo November 14, 2017 16:05
@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.476% when pulling 99abe73 on vtol-fixes into d9bd982 on develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.476% when pulling 56c733e on vtol-fixes into d9bd982 on develop.

@@ -106,6 +106,7 @@ int main(int /*argc*/, char ** /*argv*/)
std::this_thread::sleep_for(std::chrono::seconds(10));

// Change to VTOL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove - incorrect and not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thx will do.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

BY INSPECTION (not testing)

  • minor change to docs in example. I may have others when I actually get round to running this, but I think it looks really solid.
  • What will happen on PX4 prior to version with ACK added? I assume that this will still timeout. Should there be a test for version?
  • Comments on methods not quite correct - below for the transition to fixed wing:
    Currently
    "The associated action will only be executed for VTOL vehicles in multicopter mode. On other vehicles/modes the command will fail with a Result.".
    Should be (AFAIK):
    "The associated action will only be executed for VTOL vehicles (on other vehicle types the command will fail with a Result). The command will succeed if called when the vehicle is already in fixed-wing mode."

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.476% when pulling 0dbc777 on vtol-fixes into 9c6e936 on develop.

@julianoes
Copy link
Collaborator Author

What will happen on PX4 prior to version with ACK added? I assume that this will still timeout. Should there be a test for version?

It will timeout but still work, so not really a problem in my opinion.

Comments on methods not quite correct - below for the transition to fixed wing:

Thanks, fixed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.476% when pulling fc1111f on vtol-fixes into 9c6e936 on develop.

@hamishwillee
Copy link
Collaborator

What will happen on PX4 prior to version with ACK added? I assume that this will still timeout. Should there be a test for version?

It will timeout but still work, so not really a problem in my opinion.

I somewhat disagree, because you're getting a fail error even though the action will actually be executed (if the action was not being executed I'd feel differently). The caller has to "magically" know that this fail is OK.

Your call though.

Otherwise all good.

@julianoes
Copy link
Collaborator Author

I somewhat disagree, because you're getting a fail error even though the action will actually be executed (if the action was not being executed I'd feel differently). The caller has to "magically" know that this fail is OK.

DroneCore does not "officially" support any kind of release yet but is only tested against master. Therefore, we don't need to worry about earlier releases/versions at this point.

@julianoes julianoes merged commit e680ffe into develop Nov 16, 2017
@julianoes julianoes deleted the vtol-fixes branch November 16, 2017 13:25
@hamishwillee
Copy link
Collaborator

Thanks!

dlech pushed a commit to dlech/MAVSDK that referenced this pull request Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants