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

Move ARDroneLib to an external project #80

Merged
merged 8 commits into from
Jan 18, 2014

Conversation

kbogert
Copy link

@kbogert kbogert commented Jan 9, 2014

This patchset removes the ARDroneLib directory and associated build step in favor of cmake's external project module. This allows the ARDrone libraries to be pulled and built automatically during catkin_make and keeps the source directory clean of binaries.

@mani-monaj
Copy link
Member

@kbogert Very interesting patch. Something that has been on my TODO list for a long time. Thank you very much for that. I will test it very soon. One quick suggestion:

Isn't it better to pull external ARDroneSDK files from a tagged git repo? This way you can upgrade the external repo with new SDKs without worrying of breaking the old versions of the driver that depend on an older version.

@kbogert
Copy link
Author

kbogert commented Jan 10, 2014

You are correct that using a git tag would be an improvement, and this can be done by adding "GIT_TAG (tag)" to the CMakeLists file. Right now the ARDroneSDK files are being pulled from a repository on my github account and I was thinking you all would want to fork it or provide your own so that it could be controlled. I don't mind maintaining it, however.

Alternatively the ARDroneSDK could be provided as an archive stored in the ardrone_autonomy repository. Externalproject would expand this into the catkin build directories instead of the source. This would be slightly more friendly to the end user as no network access would be required during building. I'm planning on keeping my ARDroneSDK repository for a while to act as version control when new versions are dropped on us (I'm also planning on making some changes, though these will be in a branch) so this could be as simple as downloading the github provided zip file when ready.

@mani-monaj
Copy link
Member

Thank you for the quick update. I can confirm that it compiles fine on my machine. I will test it with a real drone soon.

  • I could not completely understand changes that are made by ardrone_lib.patch file (except for the ardrone_tool.h file. Could you please help me with that?
  • I think we'd better ship the SDK alongside the driver as an archive. This will allow offline people to re-compile. It will also leave us with one less repo to maintain :) I'll take care of that.

@kbogert
Copy link
Author

kbogert commented Jan 10, 2014

I actually don't know much about the changes in that file, it was created by comparing the current ARDroneLib directory stored in ardrone_autonomy with the one found in my ARDroneSDK repository. It should be all the changes that have been applied to v2.0.1 by this project, and I added a modified version of the sdk.makefile file to compile and install the library.

SHA: afa3bd2 has some of the changes, but the others I don't know (particularly the video related stuff). It's possible that my repository has some issues, as I just cloned it from someone else :/

@kbogert
Copy link
Author

kbogert commented Jan 10, 2014

That was it, the actual diff is just afa3bd2 and some noise. I'm updating the patch and my repository.

@kbogert
Copy link
Author

kbogert commented Jan 10, 2014

I rebuilt my repository using the tarball from parrot, you may have to delete the git directory for the ardrone repository before you can compile this (mine is in ~/hydro_workspace/build/ardrone)

The resulting patch file is much cleaner.

@mani-monaj
Copy link
Member

@kbogert Thanks. The new patch works great. I will make the changes to ship the tarball with the driver. I also sent this PR for rosdep database to include libiw-dev.

I think we are getting to the point to submit the driver to ROS build farm and make that available using apt-get.

@mani-monaj mani-monaj merged commit aa49db1 into AutonomyLab:catkin Jan 18, 2014
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.

2 participants