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.

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

Option to ignore error frames #388

Closed

Conversation

yanick-douven
Copy link

Finalized implementation of #362 with the addition of:

  • ROS parameter parsing (ignore-errors/error-name=true/false)
  • CAN_ERR_LOSTARB is ignored by default
  • tested on physical CAN bus

@Timple
Copy link

Timple commented May 8, 2020

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
Copy link
Member

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:

  • 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
  • serializable_map is not needed, can::Settings should be subclassed if you want to offer an API
  • class XmlRpcSettings : public Settings{
    should be used for ROS, perhaps templated to not depend on XmlRpcValue directly
  • canopen_chain_node should pass the settings to CANLayer somehow.

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
Copy link
Author

@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 mathias-luedtke added the wrid20 World ROS-Industrial Day 2020 label Jul 6, 2020
@mathias-luedtke mathias-luedtke mentioned this pull request Jul 7, 2020
3 tasks
@mathias-luedtke
Copy link
Member

superseded by #362

@gavanderhoorn gavanderhoorn removed the wrid20 World ROS-Industrial Day 2020 label Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants