ros-industrial / ros_canopen

CANopen driver framework for ROS (http://wiki.ros.org/ros_canopen)
GNU Lesser General Public License v3.0
332 stars 270 forks source link

Option to ignore error frames #388

Closed yanick-douven closed 4 years ago

yanick-douven commented 4 years ago

Finalized implementation of #362 with the addition of:

Timple commented 4 years ago

Since technically speaking the default behavior changes with this PR. Could this PR be evaluated before the official noetic release?

To prevent any discussions later if defaults can/might/should be changed within a release.

mathias-luedtke commented 4 years ago

Could this PR be evaluated before the official noetic release?

I had a brief look already. I cannot promise that I will have enough time until May 23.

My quick review:

To prevent any discussions later if defaults can/might/should be changed within a release.

I don't want to branch off for noetic. Only the arbitration lost error should get handled differently. This change will affect melodic and noetic.

yanick-douven commented 4 years ago

@ipa-mdl Thank you for taking the time to review! Few comments/considerations:

  • the default error mask should be kept as-is, perhaps configurable
  • the handling of the errors should be configurable, arbitration lost error should get ignored

This means a possible high load from kernel to user space, as mentioned here. Why should the kernel still report all errors if we are ignoring them anyways in the user space?

  • serializable_map is not needed, can::Settings should be subclassed if you want to offer an API

I will move (parts of) serializable_map into a subclassed version of can::Settings.

After sending the settings to can::SocketCANInterface::init() we are leaving the ROS side of things, so what would we need XmlRpcSettings for? These ignored_errors-settings are generic and not ROS-specific.

  • canopen_chain_node should pass the settings to CANLayer somehow.

I don't understand exactly what you mean, does this refer to the handling of errors?

This change will affect melodic and noetic.

Good to hear!

mathias-luedtke commented 4 years ago

superseded by https://github.com/ros-industrial/ros_canopen/pull/362