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

Refine fatal error configuration. #423

Open
wants to merge 1 commit into
base: melodic-devel
Choose a base branch
from
Open

Refine fatal error configuration. #423

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions socketcan_interface/include/socketcan_interface/socketcan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,18 @@ class SocketCANInterface : public AsioDriver<boost::asio::posix::stream_descript
| CAN_ERR_TRX /* transceiver status / data[4] */
| CAN_ERR_ACK /* received no ACK on transmission */
);
can_err_mask_t fatal_error_mask = parse_error_mask(settings, "fatal_error_mask", fatal_errors) | CAN_ERR_BUSOFF;
can_err_mask_t error_mask = parse_error_mask(settings, "error_mask", report_errors | fatal_error_mask) | fatal_error_mask;
can_err_mask_t fatal_error_mask = parse_error_mask(settings, "fatal_error_mask", fatal_errors);
can_err_mask_t error_mask = parse_error_mask(settings, "error_mask", report_errors | fatal_errors) | fatal_error_mask;
if(fatal_error_mask){
std::string fatal_errors;
translateError(fatal_error_mask, fatal_errors);
ROSCANOPEN_INFO("socketcan_interface", "Treating these errors as fatal: " << fatal_errors);
}
if(error_mask){
std::string errors;
translateError(error_mask, errors);
ROSCANOPEN_INFO("socketcan_interface", "Logging these errors: " << errors);
}
return init(device, loopback, error_mask, fatal_error_mask);
}

Expand Down Expand Up @@ -244,10 +254,13 @@ class SocketCANInterface : public AsioDriver<boost::asio::posix::stream_descript
input_.id = frame_.can_id & CAN_EFF_MASK;
input_.is_error = 1;

if (frame_.can_id & fatal_error_mask_) {
if (frame_.can_id & error_mask_) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this change..
This triggers the state callback on every error message and might a little bit noisy in general

Copy link
Author

Choose a reason for hiding this comment

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

My idea was that error_mask_ is used to test for errors which must be logged. The fatal_error_mask_ is intended for errors which must put the driver in not ready mode (using setNotReady). Is this correct?

I like this idea, apart from the recovery strategy, users can configure which errors should be fatal. Another idea would be to simply log all errors, en only allow to configure which errors are fatal.

The concept of "fatal" being the behavior that the driver is put into not ready mode.

Copy link
Author

Choose a reason for hiding this comment

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

This triggers the state callback on every error message and might a little bit noisy in general

Error's should only happen every now and then (arbitration error, ack error). If there are many error's on the bus, it's probably okay to log them? Another strategy could be to rate limit the amount of errors logged, and summarize them, but this would be more work.

Copy link
Author

Choose a reason for hiding this comment

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

Btw, the reason why this change is important, is that in case of an error, every now and then, the driver hangs on "failed to send message".

Now that I think about it, in the current implementation all errors, except the BUS_OFF error can be configured to be ignored, right? Setting the whole bunch of parameters to false will do the trick!

Copy link
Member

Choose a reason for hiding this comment

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

My idea was that error_mask_ is used to test for errors which must be logged.

error_mask entry will be reported to the error frames callback!

We could add a logging_mask for this purpose, which could default to error_mask.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not in favor of adding extra configration. I suggest to move the logging inside the fatal error clause?

I guess the error frame callback is call by calling setInternalError?

ROSCANOPEN_ERROR("socketcan_interface", "internal error: " << input_.id);
setInternalError(input_.id);
setNotReady();
if (frame_.can_id & fatal_error_mask_) {
ROSCANOPEN_ERROR("socketcan_interface", "fatal error, switching to not ready");
setNotReady();
}
}
}else{
input_.is_extended = (frame_.can_id & CAN_EFF_FLAG) ? 1 :0;
Expand Down