Adding simple autorecover - hopefully all of my build errors are fixed #249
Conversation
There was a problem hiding this comment.
First all: Thanks for your contribution!
Please find my immediate change requests below.
In ros_canopen auto-recovering should not be done by default.
So please add a parameter to enable this feature explicitly.
It would be great to have a Trigger service that could be called e.g. in non-auto mode.
PS: You can add changes in this PR, it will update automatically. There is no need to open new PRs.
| @@ -1,4 +1,5 @@ | |||
| cmake_minimum_required(VERSION 2.8.3) | |||
| add_compile_options(-std=c++11) | |||
There was a problem hiding this comment.
C++11 shall not be used!
There was a problem hiding this comment.
Sigh. It's 2018 i.e. 2 more standards later.
There was a problem hiding this comment.
c++14 is now the default for melodc 🎉
| ${catkin_LIBRARIES} | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Is there a .clang-format file which can be used to avoid this kind of error?
| socketcan_to_topic | ||
| socketcan_to_topic_node | ||
| topic_to_socketcan | ||
| topic_to_socketcan_node | ||
|
|
| @@ -131,4 +151,6 @@ if(CATKIN_ENABLE_TESTING) | |||
| ${catkin_LIBRARIES} | |||
| ) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
whitespace, please keep the new line ending
| { | ||
| state_pub_ = nh->advertise<can_msgs::CanState>("can_state", 1, true); | ||
|
|
||
| timer_ = nh->createWallTimer(ros::WallDuration(5), [this](const ros::WallTimerEvent& event) {recover();}, true, false); |
There was a problem hiding this comment.
please migrate the lambda into a dedicated function
There was a problem hiding this comment.
It might be good to make the timeout configurable.
| } | ||
| else | ||
| { | ||
| timer_.stop(); |
There was a problem hiding this comment.
Why do you need to stop a one-shot timer?
There was a problem hiding this comment.
If status has been updated before the callback fires, we we're giving a little time for this system to "maybe" fix itself
There was a problem hiding this comment.
void ros::WallTimer::stop ()
| Stop the timer. Once this call returns, no more callbacks will be called. Does nothing if the timer is already stopped.
ie, if status became ready we don't want the callback to fire
There was a problem hiding this comment.
There is no way that the state can change from bad to good without intervention (?).
There was a problem hiding this comment.
I wasn't sure if that statement would hold true in future versions of socketcan_interface
| */ | ||
| void SocketCANRecoverCtrl::recover() | ||
| { | ||
| timer_.stop(); |
There was a problem hiding this comment.
This stop is explicitly required, you will have to forgive me I don't remember why, either it made the timer hang holding a reference preventing from being able to simple Ctrl-C the app, or it was needed to call start below, ill look into my reasoning here
There was a problem hiding this comment.
Ah yes, it might block at ctrl+c.
But fixing it here is just handling the symptom.
| default: | ||
| state_msg.driver_state = can_msgs::CanState::CLOSED; | ||
| break; | ||
| } |
There was a problem hiding this comment.
I would prefer to just copy it: state_msg.driver_state = state.driver_state;
There was a problem hiding this comment.
can::State and can_msgs::CanState are incompatible, i think
| @@ -149,7 +149,7 @@ class SocketCANInterface : public AsioDriver<boost::asio::posix::stream_descript | |||
| ret = true; | |||
| } | |||
| if( internal_error & CAN_ERR_RESTARTED){ | |||
| str += "ontroller restarted;"; | |||
| str += "controller restarted;"; | |||
| */ | ||
| void SocketCANRecoverCtrl::stateCallback(const can::State & state) { | ||
| publishStatus(state); | ||
| if(!state.isReady()) |
There was a problem hiding this comment.
I guess the driver should get recovered only if it is not closed.
|
@watters-torc: Do you have time for working on it? If not, I will see if I can find some time to implement the follwing base on part of your code:
|
|
Is this still work in progress? We also receive error frames which kill the bridge all the time. |
|
Works like a charm for us. Would be nice if this could make it into an official release |
|
Would be still a good thing to have a auto recovery when having errors. For my case, when disconnecting the can network from our hardware and reconnect it again, there is no time the can messages will be send again. It always says: "[ERROR] [...]: Failed to send message: 8...#00....." |
|
Anything more needed to get this merged? |
Maintainers: This is a feature we needed for a project, originally we were under the impression the socketcan_bridge and socketcan_interface were under BSD, it seems like this project (ros_canopen) has a bunch of mixed licenses. I don't necessarily see this as a production ready fix but it works for what we need. I'm able to make changes or take direction from the maintainers if you'd like work to further be done here.
Proposed fix / gap stop for #244