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

OA installation script #466

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

OA installation script #466

wants to merge 1 commit into from

Conversation

mrivi
Copy link
Contributor

@mrivi mrivi commented Aug 13, 2019

The PR provides the users with an installation script to be run after ROS and PX4 Toolchain are installed to quickly setup the avoidance package

@mrivi mrivi changed the title README: add installation script to be run after the ones in the dev g… OA installation script Aug 13, 2019
@mrivi
Copy link
Contributor Author

mrivi commented Aug 14, 2019

@hamishwillee - Your feedback would be much appreciated! Thanks!

README.md Outdated
@@ -43,6 +43,28 @@ The documentation contains information about how to setup and run the two planne

## Installation

### Script Installation (Recommended)
1. Go to the [PX4 Development Guide](https://dev.px4.io/v1.8.2/en/setup/dev_env_linux.html#gazebo-with-ros) and run the Gazebo with ROS installation script
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a specific version of the setup for PX4 1.8. Not sure if that is intentional. Master is at http://dev.px4.io/master/en/setup/dev_env_linux_ubuntu.html#rosgazebo

Copy link
Contributor

@hamishwillee hamishwillee Aug 15, 2019

Choose a reason for hiding this comment

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

Also, did you test these? I've been running into problems on the other scripts due to recent CMAKE/Python issues
ie nothing wrong with your steps, just that the scripts might have been broken underneath you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was intentional. That's the last script that installs ROS Kinetic instead of Melodic. I didn't test the entire thing yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! Up to you if you want to spell that out in a note (i.e. so that people don't just use master version without thinking about it.

@@ -0,0 +1,9 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW Don't know what is standard, but generally I quite like it if scripts have brief docs about what they do/when they are used. Maybe a link to where this is defined.

Entirely up to you

README.md Outdated
roslaunch global_planner global_planner_depth-camera.launch
```

Alternitavely you can follow the step by step instructions in the following sections.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo Alternitavely > Alternatively

README.md Outdated
``` bash
source install_avoidance.sh
```
To run the local planner
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we just want to run one of the planners? Might be better to make that clear like this

  1. Run the desired planner:
    • Local planner:
      roslaunch local_planner local_planner_sitl_3cam.launch
    • Safe landing planner
      roslaunch safe_landing_planner safe_landing_planner.launch
    • Global planner
      roslaunch global_planner global_planner_depth-camera.launch

README.md Outdated
```bash
roslaunch safe_landing_planner safe_landing_planner.launch
```
To run the safe global planner
Copy link
Contributor

Choose a reason for hiding this comment

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

"Safe" global planner?

@hamishwillee
Copy link
Contributor

Looks great, and good idea. I'm a fan of scripted installation (and even more of proper app packages). A few minor comments inline

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.

None yet

2 participants