Added service to force an Arduino hard reset in serial_node.py #349

chrisspen · 2018-01-11T04:04:13Z

An Arduino may become unresponsive, either due to subtle coding bugs, memory fragmentation, or electrical faults. When this happens, it will often become completely unresponsive, resulting in the repeated rosserial error "Lost sync with device, restarting...". However, despite this message, there's no code in serial_node.py to actually restart the Arduino. It merely waits for the Arduino to come back online, which rarely ever happens. Currently, in this instance, the only fix is to reset the Arduino, either by manually pressing the Arduino's reset button or by running a program like ard-reset-arduino <port>.

This change adds the ability to programmatically reset the Arduino via a service call to the serial_node.py. It has the same effect as someone pressing the Arduino's reset button.

I debated adding code to automatically call this reset, but decided not to because I thought the logic of when it should be called would heavily depends on the application, and so it would better be left to the end user to implement in a custom node.

The intended use case of this would be to implement a custom node that monitors the diagnostic messages generated by serial_node.py, looking for the "last sync lost" key, and if this is seen repeatedly, then to call the ~hard_reset service on the node to force the Arduino to come back online.

romainreignier · 2018-01-11T09:04:19Z

I like the idea of remote resetting an Arduino but it is very Arduino specific and it is pity to put it in the general rosserial_python node (I know it is the only place to put that snippet).

rosserial is used in a lot more systems than Arduinos and will be useless there.

Maybe rename the service ~hard_reset_arduino to make it clear for the user that it only applies to Arduinos?

chrisspen · 2018-01-11T12:12:53Z

That's a good point. I've only used this package for Arduino, but you're right, Arduino isn't the only thing supported. Renaming the service makes sense.

mikepurvis · 2018-01-11T14:10:39Z

rosserial_python/src/rosserial_python/SerialClient.py

@@ -334,7 +334,7 @@ def __init__(self, port=None, baud=57600, timeout=5.0, fix_pyserial_for_test=Fal

        self.pub_diagnostics = rospy.Publisher('/diagnostics', diagnostic_msgs.msg.DiagnosticArray, queue_size=10)

-        rospy.Service('~hard_reset', Empty, self.hard_reset)
+        rospy.Service('~hard_reset_arduino', Empty, self.hard_reset_arduino)


Pedantic, but isn't this more of a soft reset?

I guess it depends on your definition. I would define a soft reset asking the Arduino to reset itself in your sketch, which may or may not be supported. I believe toggling the DTR pin actually pulls the Arduino's reset pin, so it's not effected by the sketch.

mikepurvis · 2018-01-11T14:37:26Z

I'm also not over the moon about putting something so Arduino-specific into the generic node. The ideal way to handle this would be putting a hook in SerialClient.py such that an Arduino-specific version of the python node (eg, in the rosserial_arduino package) could import the generic functionality and supply the additional stuff on top.

That said, I can appreciate the pragmatism of just baking this in.

chrisspen · 2018-01-11T14:52:58Z

Should I relocate this functionality to an arduino-specific serial_node.py and SerialClient.py and put them in rosserial_arduino?

mikepurvis · 2018-01-11T17:02:17Z

I would prefer that path if it's not ridiculous. However, I can definitely picture that it might be, in which case the proposed scheme is acceptable.

chrisspen · 2018-01-11T21:02:17Z

It'll create a lot more changed files, but it would cleanly separate the Arduino-specific functionality.

…k when both Arduino and serial_node.py get stuck writing to each other. Added pylint config to help clean up code.

chrisspen · 2018-03-22T01:49:17Z

I've refactored the Arduino-specific reset functionality into a "rosserial_arduino" package, as requested.

I also fixed a major deadlock bug in the main serial_node.py, causing periodic freezes on some platforms, by refactoring it to use separate threads for reads and writes.

chrisspen · 2018-03-22T01:53:38Z

Do you know why the travis tests are failing? It's complaining about a missing compiler environment variable, but I haven't touched any C components.

mikepurvis · 2018-03-23T14:52:28Z

I think the Travis container's version of Clang got updated; I've fixed our builds now and re-kicked this PR's job, so it should now pass.

mikepurvis · 2018-03-23T14:54:22Z

pep8.sh

@@ -0,0 +1,2 @@
+#!/bin/bash
+pylint --rcfile=pylint.rc rosserial_python/src/rosserial_python rosserial_arduino/src/rosserial_arduino/SerialClient.py


Please remove this and the config file below. My preference would be to integrate roslint (http://wiki.ros.org/roslint), but if there are valid reasons to do something separate here, let's discuss it on a PR dedicated to that purpose.

mikepurvis · 2018-03-23T14:56:35Z

rosserial_arduino/nodes/serial_node.py

+                rospy.loginfo("Shutting down process %r", process)
+                process.terminate()
+                process.join()
+            rospy.loginfo("All done")


Are there arduinos using the TCP connection? If not, let's remove this section and the params above.

mikepurvis · 2018-03-23T14:57:41Z

rosserial_arduino/setup.py

@@ -0,0 +1,11 @@
+#!/usr/bin/env python


Adding Python modules to rosserial_arduino requires a bunch of changes to the CMakeLists.txt and package.xml files. Please ensure those are included here.

I think these should now be up to date. Please let me know if I missed anything.

…include dependencies. Removed unnecessary tcp functionality from arduino-specific serial_node.py

…into jade-devel

mikepurvis · 2018-03-23T18:27:04Z

Thanks for the quick response! Looks like there's now a build problem that Travis has caught:

CMake Error at /opt/ros/indigo/share/catkin/cmake/catkin_python_setup.cmake:24 (message):
  generate_messages() must be called after catkin_python_setup() in project
  'rosserial_arduino'
Call Stack (most recent call first):
  rosserial/rosserial_arduino/CMakeLists.txt:23 (catkin_python_setup)
-- Configuring incomplete, errors occurred!
See also "/home/travis/catkin_ws/build/CMakeFiles/CMakeOutput.log".
See also "/home/travis/catkin_ws/build/CMakeFiles/CMakeError.log".
Invoking "cmake" failed

chrisspen · 2018-03-24T02:55:13Z

I think CMakeLists.txt should now be fixed.

mikepurvis · 2018-03-24T11:35:18Z

Great, thanks!

Please take a quick look at the rosserial_arduino tutorials, and update where appropriate to recommend the use of the server from rosserial_arduino rather than rosserial_python and note the availability of the new functionality.

chrisspen · 2018-03-24T14:36:30Z

Will do.

…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)

Added hard_reset service call to serial_node

050ff0a

Renamed reset method to denote arduino specific.

ae3176d

mikepurvis reviewed Jan 11, 2018

View reviewed changes

chrisspen added 2 commits January 16, 2018 23:39

Refactored SerialClient to use a write thread, working around deadloc…

63b7823

…k when both Arduino and serial_node.py get stuck writing to each other. Added pylint config to help clean up code.

Removed debugging statements.

f6aae16

mikepurvis requested changes Mar 23, 2018

View reviewed changes

chrisspen added 2 commits March 23, 2018 13:34

Removed pep8 validation files. Updated cmakelists and package.xml to …

59925fa

…include dependencies. Removed unnecessary tcp functionality from arduino-specific serial_node.py

Merge branch 'jade-devel' of https://github.com/ros-drivers/rosserial …

7fd4995

…into jade-devel

mikepurvis approved these changes Mar 23, 2018

View reviewed changes

chrisspen added 2 commits March 23, 2018 22:33

Reorganized cmakelist

bc5076f

Reorganized cmakelist

dfad357

mikepurvis merged commit fe1e233 into ros-drivers:jade-devel Mar 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added service to force an Arduino hard reset in serial_node.py #349

Added service to force an Arduino hard reset in serial_node.py #349

chrisspen commented Jan 11, 2018

romainreignier commented Jan 11, 2018

chrisspen commented Jan 11, 2018

mikepurvis Jan 11, 2018

chrisspen Jan 11, 2018

mikepurvis commented Jan 11, 2018

chrisspen commented Jan 11, 2018

mikepurvis commented Jan 11, 2018

chrisspen commented Jan 11, 2018

chrisspen commented Mar 22, 2018

chrisspen commented Mar 22, 2018

mikepurvis commented Mar 23, 2018

mikepurvis Mar 23, 2018

chrisspen Mar 23, 2018

mikepurvis Mar 23, 2018

chrisspen Mar 23, 2018

mikepurvis Mar 23, 2018

chrisspen Mar 23, 2018

mikepurvis commented Mar 23, 2018

chrisspen commented Mar 24, 2018

mikepurvis commented Mar 24, 2018

chrisspen commented Mar 24, 2018

		@@ -0,0 +1,2 @@
		#!/bin/bash
		pylint --rcfile=pylint.rc rosserial_python/src/rosserial_python rosserial_arduino/src/rosserial_arduino/SerialClient.py

Added service to force an Arduino hard reset in serial_node.py #349

Added service to force an Arduino hard reset in serial_node.py #349

Conversation

chrisspen commented Jan 11, 2018

romainreignier commented Jan 11, 2018

chrisspen commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepurvis commented Jan 11, 2018

chrisspen commented Jan 11, 2018

mikepurvis commented Jan 11, 2018

chrisspen commented Jan 11, 2018

chrisspen commented Mar 22, 2018

chrisspen commented Mar 22, 2018

mikepurvis commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepurvis commented Mar 23, 2018

chrisspen commented Mar 24, 2018

mikepurvis commented Mar 24, 2018

chrisspen commented Mar 24, 2018