Rework the package

[VictorLamoine]

Depends on https://github.com/dermesser/libsocket/pull/55 Merged!

Changes

• Added -Wall -Wextra compile flags
• Fixed non void return function reaching the end without a return, changeProgramCallback in src/keyence_driver_node.cpp
• Moved files for a cleaner CMakeLists.txt
• Merged the two CMakeLists.txt
• Created install targets in the CMakeLists.txt
• Fixed all catkin_lint -W3 warnings
• Changed LIBSOCKET_INCLUDE_DIRS to libsocket_INCLUDE_DIRS (and all other similar variables), this prevents catkin from complaining because of the naming mismatch (see https://github.com/dermesser/libsocket/pull/55).

Testing

• Travis uses my fork right now: https://github.com/InstitutMaupertuis/libsocket
• This should be changed once https://github.com/dermesser/libsocket/pull/55 is merged Done

Others

Tell me if there is anything you don't like, I'll test this version this afternoon but there is no functional change so I don't think it can break anything.

[Jmeyer1292]

If you think this is the right thing to do, I have no real objection.

I originally made the CMakeLists lay out the way I did because I wanted to make an effort at truly separating the ROS crap from the core communication library.

I made the headers keyence instead of keyence_experimental because the package was supposed to be renamed to remove the experimental reference eventually. But like all the other ROS-I "experimental" stuff it just ended up being a bad idea.

Anyway, if you test and feel good about it then we'll merge.

[gavanderhoorn]

@Jmeyer1292 wrote:

I originally made the CMakeLists lay out the way I did because I wanted to make an effort at truly separating the ROS crap from the core communication library.

+1 to keeping the base library separate from the ROS wrapper / crap.

[VictorLamoine]

I don't think it's possible to completely separate the ROS wrapper / node from the library because the library has to be installed using catkin variables.

I cannot add install targets for the library in the top CMakeLists.txt because CMake doesn't see the targets of the sub CMake, this results in this error:

CMake Error at /opt/ros/kinetic/share/catkin/cmake/test/gtest.cmake:323 (_install):
  install TARGETS given target "keyence_change_program" which does not exist
  in this directory.
Call Stack (most recent call first):
  keyence_experimental/CMakeLists.txt:85 (install)

The top CMake file looks like this:

add_subdirectory(keyence_library)

#############
## Install ##
#############

## Mark executables and/or libraries for installation
install(
  TARGETS
  keyence_driver_node
  keyence_change_program
  keyence_get_profile
  keyence_get_setting
  keyence_impl
  ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
  LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
  RUNTIME DESTINATION ${CATKIN_GLOBAL_BIN_DESTINATION}
)

Maybe there's something I don't know but from what I can tell I have to put install targets into the keyence_library/CMakeLists.txt, which means ROS and the library are not completely separated because I'm using catkin variables in the sub directory CMake file.

[VictorLamoine]

@Jmeyer1292 The current keyence experimental will not build (because LIBSOCKET_INCLUDE_DIRS is now libsocket_INCLUDE_DIRS, same for libraries), this pull request fixes the build, it would be nice if you could review this and merge once we agree on the modifications :whale:

I have tested the driver (LJ-V7100P + CB-EP100 + LJ-V7200) and it works fine.