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.

Sign up for GitHub

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

Jump to bottom

Fix catkin lint erros #296

Merged
merged 1 commit into from Nov 6, 2017
Merged

Fix catkin lint erros #296

merged 1 commit into from Nov 6, 2017

Conversation

Baycken
Copy link
Contributor

@Baycken Baycken commented Jul 7, 2017 •
edited

  • Fix all catkin lint -W2 errors and warning.
    This is mostly cosmetic changes and should not affect execution of the code.

@Baycken Baycken changed the title catkin lint -W2 fix and Exit on topic callback KeyError for Python serial_node catkin lint -W2 fix, Exit on topic callback KeyError for Python serial_node, and Set arduino-cmake as submodule Jul 7, 2017
mikepurvis
mikepurvis requested changes Jul 18, 2017
View reviewed changes
Copy link
Member

@mikepurvis mikepurvis left a comment

Choose a reason for hiding this comment

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

Several of the comments apply in multiple places.

.gitmodules Outdated
@@ -0,0 +1,3 @@
[submodule "rosserial_arduino/arduino-cmake"]
path = rosserial_arduino/arduino-cmake
url = git@github.com:mcgill-robotics/arduino-cmake.git
Copy link
Member

Choose a reason for hiding this comment

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

Historically, we couldn't do this because bloom didn't support upstreams with submodules. The ticket tracking that appears to be a bit inconclusive as far as the current state of affairs: ros-infrastructure/bloom#217

Could you investigate please, and determine whether this is going to work?

You can try running bloom locally with the following script: https://gist.github.com/mikepurvis/7036293

rosserial_arduino/CMakeLists.txt Outdated
@@ -26,7 +26,7 @@ install(DIRECTORY arduino-cmake
DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}
)

install(PROGRAMS src/rosserial_arduino/make_libraries.py
install(PROGRAMS src/${PROJECT_NAME}/make_libraries.py
Copy link
Member

Choose a reason for hiding this comment

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

This should actually be catkin_install_python, I think.

rosserial_arduino/package.xml Outdated
@@ -2,7 +2,7 @@
<name>rosserial_arduino</name>
<version>0.7.6</version>
<description>
Libraries and examples for ROSserial usage on Arduino/AVR Platforms.
ROSserial for Arduino/AVR Platforms.
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere, the name is styled as rosserial, all lowercase. Please use that here, since we're updating.

rosserial_client/cmake/rosserial_client-extras.cmake Outdated
@@ -28,7 +28,6 @@ function(rosserial_generate_ros_lib)
COMMAND ${CATKIN_ENV} rosrun ${make_libraries_PACKAGE} ${make_libraries_SCRIPT} ${PROJECT_BINARY_DIR}
)
add_custom_target(${PROJECT_NAME}_ros_lib DEPENDS ${PROJECT_BINARY_DIR}/ros_lib)
add_dependencies(${PROJECT_NAME}_ros_lib rosserial_msgs_genpy std_msgs_genpy)
Copy link
Member

Choose a reason for hiding this comment

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

This probably shouldn't be removed. What's the rationale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message generation target name have changed from <pkg_name>_genpy to <pkg_name>_generate_messages_py so they no longer work.

I just realized that the add_custom_target does not inherent dependencies. Maybe this should depend directly on rosserial_msgs and std_msgs?

Copy link
Member

Choose a reason for hiding this comment

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

This has been fixed a different way as of #320. Would you mind rebasing the rest of the change so that I can merge it?

rosserial_embeddedlinux/CMakeLists.txt Outdated
nav_msgs
rosserial_client
sensor_msgs
std_msgs
Copy link
Member

Choose a reason for hiding this comment

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

These aren't necessary at build time. The file to fix here is the package.xml.

rosserial_mbed/CMakeLists.txt Outdated

catkin_install_python(PROGRAMS src/${PROJECT_NAME}/make_libraries.py
DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION}
)
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to clean up the rest of this file, removing commented-out lines, etc.

rosserial_python/CMakeLists.txt Outdated
@@ -6,6 +6,6 @@ catkin_package()

catkin_python_setup()

install(PROGRAMS nodes/serial_node.py nodes/message_info_service.py
install(PROGRAMS nodes/message_info_service.py nodes/serial_node.py
Copy link
Member

Choose a reason for hiding this comment

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

catkin_install_python, please.

rosserial_python/src/rosserial_python/SerialClient.py Outdated
@@ -493,6 +493,8 @@ def run(self):
self.callbacks[topic_id](msg)
except KeyError:
rospy.logerr("Tried to publish before configured, topic id %d" % topic_id)
rospy.logerr("Exiting...")
rospy.signal_shutdown("Topical callback KeyError")
Copy link
Member

Choose a reason for hiding this comment

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

This is a massive behaviour change. Please remove from this PR and create a new one if you want to discuss further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll revert this back

@Baycken Baycken changed the title catkin lint -W2 fix, Exit on topic callback KeyError for Python serial_node, and Set arduino-cmake as submodule catkin lint -W2 fix Jul 18, 2017
@Baycken
Copy link
Contributor Author

Baycken commented Jul 18, 2017

@mikepurvis question, should examples such as the one in rosserial_embeddedlinux be installed?

@mikepurvis
Copy link
Member

I wouldn't bother installing examples. Users who reference them will be looking at the source regardless.

@Baycken
Copy link
Contributor Author

Baycken commented Aug 9, 2017

@mikepurvis Can you have another look?

@Baycken
Copy link
Contributor Author

Baycken commented Nov 5, 2017

@mikepurvis rebased.

@Baycken Baycken changed the title catkin lint -W2 fix Fix catkin lint erros Nov 5, 2017
@mikepurvis
Copy link
Member

Looks great! Thanks for doing this cleanup.

@mikepurvis mikepurvis merged commit d7e08ea into ros-drivers:jade-devel Nov 6, 2017
romainreignier pushed a commit to 1r0b1n0/rosserial that referenced this pull request Nov 7, 2018
@1r0b1n0
Merge pull request ros-drivers#2 in PHEN/rosserial from update_upstre…
f3572cc 
…am to jade-devel

* commit 'a707373c7e9f9ad40ad9440550840e29960c9290':
  Changed hardcoded pin 13 to LED_BUILTIN (ros-drivers#328)
  Re-attempting rosserial for VEX Cortex (ros-drivers#380)
  Added service to force an Arduino hard reset in serial_node.py (ros-drivers#349)
  Fix compiling on boost > 1.66 (ros-drivers#362)
  Use the ! prefix introduced in gcc4mbed for mbed examples (ros-drivers#304)
  Add support for boolean parameters (ros-drivers#355)
  Change Travis to use clang-5.0.0 (ros-drivers#363)
  [python] fix an unboundlocalerror (ros-drivers#346)
  Added ESP32 support (ros-drivers#345)
  Retry opening the serial port every 3 seconds (ros-drivers#342)
  In rosserial_arduino, changed embedded type size for ROS uint64 to 8 bytes from 4. (ros-drivers#312)
  0.7.7
  Changes
  Copy src/examples to install dir so make_libraries.py doesn't fail (ros-drivers#336)
  Add overall spin timeout to rosserial read. (ros-drivers#334)
  Fixing formatting on files. (ros-drivers#333)
  Fix catkin lint errors (ros-drivers#296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikepurvis mikepurvis mikepurvis requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Baycken @mikepurvis