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

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

Merged

Conversation

gavanderhoorn
Copy link
Member

@gavanderhoorn gavanderhoorn commented Jun 22, 2021

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

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

@gavanderhoorn
Copy link
Member Author

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

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

@gavanderhoorn gavanderhoorn merged commit 27f8314 into ros-industrial:melodic-devel Jun 22, 2021
@gavanderhoorn gavanderhoorn deleted the rebased_fix_issue_238 branch June 22, 2021 19:08
@haudren
Copy link

haudren commented Jun 23, 2021

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

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

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

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

@haudren
Copy link

haudren commented Jun 23, 2021

Thanks a lot sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants