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

Adapted acceptance radius for rovers, and added vehicle_type field #11918

Merged
merged 6 commits into from
Jun 13, 2019

Conversation

AmeliaEScott
Copy link
Contributor

Test data / coverage
I tested these changes in SITL, by running a mission from start to finish on the following targets:

  • gazebo_standard_vtol
  • gazebo_tiltrotor
  • gazebo_plane
  • gazebo_tailsitter
  • gazebo_iris
  • gazebo_rover

Describe problem solved by the proposed pull request
Many modules used !vehicle_status.is_rotary_wing to determine if a vehicle is fixed wing. This led to assuming that other vehicle types, like rovers, are fixed-wing.

Describe your preferred solution
The vehicle_status message was changed to remove the is_rotary_wing flag and replace it with an enum, vehicle_type, which can take the values FIXED_WING, ROTARY_WING, or GROUND

Describe possible alternatives
I considered using the existing system_type field in vehicle_status, but this enum is too complex. There are many different types of rotary wing aircraft, for example. This field also does not consider the state of a VTOL; that is, whether it is in fixed-wing or rotary-wing mode.

Additional context
This is a continuation from the work done in #7175

@AmeliaEScott AmeliaEScott changed the title Tweaks to rover code Changed vehicle_status message to include vehicle_type field Apr 26, 2019
@AmeliaEScott AmeliaEScott force-pushed the pr-rover branch 2 times, most recently from db345cb to 2e4a4d2 Compare April 26, 2019 14:10
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

A couple of first comments but I need to go through it in detail again.

msg/vehicle_status.msg Outdated Show resolved Hide resolved
msg/vehicle_status.msg Outdated Show resolved Hide resolved
src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved
src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved
src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved
@dagar
Copy link
Member

dagar commented Apr 27, 2019

Overall this looks good, we just need to be careful with the usage surrounding VTOL where various modules depend on status.is_rotary_wing also reflecting pertinent VTOL state.

src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved
src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved
src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved
src/modules/navigator/mission_block.cpp Show resolved Hide resolved
src/modules/navigator/mission_block.cpp Outdated Show resolved Hide resolved
@AmeliaEScott AmeliaEScott changed the title Changed vehicle_status message to include vehicle_type field Changed mission block distance acceptance to account for XY distance and Z distance separately, and added vehicle_type field to vehicle_status message May 3, 2019
@AmeliaEScott AmeliaEScott changed the title Changed mission block distance acceptance to account for XY distance and Z distance separately, and added vehicle_type field to vehicle_status message Adapted acceptance radius for rovers, and added vehicle_type field May 16, 2019
julianoes
julianoes previously approved these changes May 16, 2019
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

This looks all correct now from what I can tell, thanks @ItsTimmy for all the fixes!

src/modules/commander/Commander.cpp Show resolved Hide resolved
@AmeliaEScott
Copy link
Contributor Author

I have rebased this PR so it is ready to merge, if @dagar or @julianoes would like to review it again.

Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Looks correct to me. @dagar please review and/or merge.

@julianoes
Copy link
Contributor

@dagar any objections?

@julianoes julianoes merged commit 3273547 into PX4:master Jun 13, 2019
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.

4 participants