search

ros-industrial / industrial_core

ROS-Industrial core communication packages (http://wiki.ros.org/industrial_core)
156 stars 181 forks source link

Rebased 238: TcpClient won't re-establish connection #263

Closed gavanderhoorn closed 3 years ago

gavanderhoorn commented 3 years ago

Rebased version of #238.

Got rid of the merge commits and tested against Roboguide on a different machine (which allowed me to 'disconnect' the controller by physically unplugging the cable).

The proposed changed do improve UX when it comes to handling dropped connections in the sense that the IRC nodes (and derived drivers) try to reconnect and can actually succeed.

However, the streaming trajectory relay actually drops traj pts when it loses connection. It claims to then "retry later", but it actually skips the point it dropped and continues with the next one in the trajectory. Before this change, that was not too much of a problem, as the connection would not come up again (requiring a restart, emptying queues and restarting everything).

With this change, it's possible for motion servers to immediately continue with the points they do receive (after the connection has been re-established), which is obviously less than ideal.

This PR is still an improvement though, so it'll be merged -- as soon as CI is green.

Edit: provenance and attribution retained for all commits.

gavanderhoorn commented 3 years ago

Seeing as the last two commits are basically fixup commits, I'll squash-merge everything.

gavanderhoorn commented 3 years ago

I've reviewed the original #239 and accepted it.

I'm going to merge this one without an additional review.

haudren commented 3 years ago

Thanks for merging this @gavanderhoorn ! I see that you have merged a few PRs in a row, are you planning to re-release the simple message package?

gavanderhoorn commented 3 years ago

However, the streaming trajectory relay actually drops traj pts when it loses connection. It claims to then "retry later", but it actually skips the point it dropped and continues with the next one in the trajectory. Before this change, that was not too much of a problem, as the connection would not come up again (requiring a restart, emptying queues and restarting everything).

tracking this in #265.

gavanderhoorn commented 3 years ago

I see that you have merged a few PRs in a row, are you planning to re-release the simple message package?

I'm planning to merge all open PRs -- if they make sense and if they still need to be merged.

Then I'll release whatever the state is.

gavanderhoorn commented 3 years ago

That doesn't change the status of simple_message and the rest of IRC though: maintenance only.

haudren commented 3 years ago

Thanks a lot sounds good :+1: