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

Improvement suggestions #1

Open
VladimirIvan opened this issue Aug 5, 2020 · 3 comments
Open

Improvement suggestions #1

VladimirIvan opened this issue Aug 5, 2020 · 3 comments

Comments

@VladimirIvan
Copy link
Collaborator

VladimirIvan commented Aug 5, 2020

  • README.md recommend using catkin_tools (catkin build command) instead of the deprecated catkin_make.
  • package.xml unspecified version. Verion 2 or 3 is now supported on all platforms.
  • package.xml doesn't list pinocchio as a dependency. Check if the ROS binaries are working with the plugin.
  • Use #pragma once instead of the #ifdef guard. All modern compilers support it now (GCC3.4+).
  • Include a gif with example trajectory/state visualisation in the readme, it will attract more users.
  • Consider writing some documentation with examples or linking them here if they are in another package.
  • Since the code is compiled into a plugin, catkin_package() can be left empty in the cmakelist. The headers also don't need to be installed. This will prevent users accidentally linking against the plugin, which may cause problems.
  • /** ... **/ type doxygen comments annotate the following line. The header files use these to describe a group of variables but only the first variable will contain the description. Either use the member groups feature or just switch them to regular comments.

The code is well structured and documented. Well done.

@cmastalli
Copy link
Member

I have solved some of these issues. I will comment the ones that are remaining.

* `README.md` recommend using catkin_tools (`catkin build` command) instead of the deprecated catkin_make.

Not sure how if the compilation procedure usting catkin_tools. Do I need to clone all the packages as explained here: https://catkin-tools.readthedocs.io/en/latest/verbs/catkin_build.html?

* Use `#pragma once` instead of the `#ifdef` guard. All modern compilers support it now (GCC3.4+).

I opted to keep #ifdef since it seems to compile as fast as #pragma once in GCC compiler. I thought that it doesn't hurt to keep like this.

* Consider writing some documentation with examples or linking them here if they are in another package.

This is indeed important, I will keep this task for future

@VladimirIvan
Copy link
Collaborator Author

To use catkin tools, you need the the workspace to be set up for catkin tools. There is a migration guide.
Ultimately, if you want to switch over, remove devel/build/install/log directories from your workspace. Remove src/CMakeLists.txt. Then call catkin init in the root of the workspace, and catkin build anywhere in the workspace. This will work with this package because you haven't done anything "illegal" in the cmake.
There are configuration options you can set for the workspace (Release target, install, extend another workspace, etc.). See the documentation for that.

The #ifdef guard is functionally completely fine. Just consider #pragma once for future projects and when you have to rename a lot of header files and the corresponding guards in them.

@cmastalli
Copy link
Member

Thank you for the explanation.

Now it is missing only the examples. I need to think first how to do it.

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

No branches or pull requests

2 participants