search

OpenAgricultureFoundation / openag_brain

ROS package for controlling an OpenAg food computer
GNU General Public License v3.0
221 stars 68 forks source link

Sensor errors should be sent as error codes, not strings #76

Closed gordonbrander closed 7 years ago

gordonbrander commented 7 years ago

Was getting this error:

[ERROR] [WallTime: 1376891179.350615] Message from device dropped: message larger than buffer.

Per http://wiki.ros.org/rosserial/Overview/Limitations, I thought we had maxed out our 25 pubs/25 subs, but that wasn't the case (only had 15).

It turned out to be sensor errors. All of the errors go through a single topic. The messages on it are just long enough to overflow the buffer sometimes.

We should send error codes (as float) over the wire instead of error strings.

gordonbrander commented 7 years ago

From https://github.com/OpenAgInitiative/openag_python/blob/master/docs/firmware_modules.rst:

The :cpp:class:Module superclass defines a :cpp:member:status_level attribute which the firmware module should use to report its current status. Valid value for this attribute (all defined in the header file for the :cpp:class:Module superclass) are :c:data:OK (which means that the module is "ok"), :c:data:WARN (which means that there is some warning for the module), and :c:data:ERROR (which means that there is an error preventing the module from working as desired). The superclass also defines a :cpp:member:status_msg attribute which is a :cpp:class:String that the firmware module should use to describe the status of the module. This is generally an empty string when the status level is "ok" and an error message when the status level is "warn" or "error".

gordonbrander commented 7 years ago

@LeonChambers proposal: change status_msg to status_code. By convention, module.json, defines a status_code object which is used to translate status codes to messages for nice logging:

{
  ...
  "status_code": {
    "0": "OK",
    "30": "Sensor responded with the wrong function code",
    "31": "Sensor did not return enough information"
  }
}

On the C++ side, status codes are arbitrary int values chosen by the module author to be relevant within the context of the module.

LeonChambers commented 7 years ago

Sounds good to me.

For reference, the current convention was chosen for easy integration with the ROS diagnostics stack, specifically for populating DiagnosticStatus messages. But to minimize overhead on the serial connection, it makes sense to adopt this new format and have something on the ROS side translate status codes to messages.

gordonbrander commented 7 years ago

Status codes should be uint8_t (0..255)

gordonbrander commented 7 years ago

Pull requests for an interim fix that simply changes messages for string error codes:

LeonChambers commented 7 years ago

Fixed in #102