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

Jump to bottom

Add support for setting socket max num tries and reconnect timeout #172

Merged
merged 5 commits into from
Sep 5, 2023
Merged

Add support for setting socket max num tries and reconnect timeout #172

merged 5 commits into from
Sep 5, 2023

Conversation

fmauch
Copy link
Collaborator

@fmauch fmauch commented Aug 29, 2023 •
edited
Loading

This continues #141 and fixes #140.

ToDo:

  • Add tests
    • Attempt to connect to a non-running robot with specified timeout and max_num_retries

@fmauch fmauch mentioned this pull request Aug 29, 2023
Closed
@fmauch fmauch added this to the Release 1.3.3 milestone Aug 29, 2023
@fmauch fmauch marked this pull request as ready for review September 1, 2023 15:04
@codecov
Copy link

codecov bot commented Sep 1, 2023 •
edited
Loading

Codecov Report

Patch coverage is 90.00% of modified lines.

Files Changed Coverage
include/ur_client_library/comm/tcp_socket.h ø
include/ur_client_library/rtde/rtde_client.h ø
include/ur_client_library/ur/dashboard_client.h ø
include/ur_client_library/comm/producer.h 50.00%
src/ur/dashboard_client.cpp 50.00%
include/ur_client_library/comm/pipeline.h 66.66%
include/ur_client_library/comm/stream.h 100.00%
src/comm/tcp_socket.cpp 100.00%
src/rtde/rtde_client.cpp 100.00%

📢 Thoughts on this report? Let us know!.

@fmauch fmauch force-pushed the max_num_tries branch 3 times, most recently from d2ed8ef to 1e0a34a Compare September 1, 2023 16:10
@fmauch
Copy link
Collaborator Author

fmauch commented Sep 1, 2023

Going back to draft because of failing tests

@fmauch fmauch marked this pull request as draft September 1, 2023 16:18
@fmauch fmauch mentioned this pull request Sep 4, 2023
Merged
@fmauch fmauch marked this pull request as ready for review September 4, 2023 10:11
@fmauch fmauch requested review from urmahp and urrsk September 4, 2023 10:11
urmahp
urmahp reviewed Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@urmahp urmahp left a comment

Choose a reason for hiding this comment

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

Overall it looks very good.

I have added a few comments, regarding the const keyword as it is used somewhere, but not everywhere.

include/ur_client_library/comm/tcp_socket.h Outdated Show resolved Hide resolved
include/ur_client_library/comm/pipeline.h Outdated Show resolved Hide resolved
include/ur_client_library/rtde/rtde_client.h Outdated Show resolved Hide resolved
include/ur_client_library/rtde/rtde_client.h Outdated Show resolved Hide resolved
include/ur_client_library/comm/tcp_socket.h
@@ -50,7 +50,7 @@ class TCPSocket
private:
std::atomic<int> socket_fd_;
std::atomic<SocketState> state_;
std::chrono::seconds reconnection_time_;
std::chrono::milliseconds reconnection_time_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that reconnection_time was not added in this PR, but I suggest that we add the reconnection_time as an input argument to the setup function instead of a member variable. Then max_num_tries and reconnection_time can be modified in the same way and both variables are only used within the setup function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added it to the setup method and added a deprecation notice. Due to being implemented as a default parameter value I couldn't properly overload in order to keep the old and current API stable. Hence, I added a manual conflict resolution to choose the value set through the deprecated API with an additional deprecation notice.

@fmauch fmauch requested a review from urmahp September 4, 2023 14:12
@fmauch
Copy link
Collaborator Author

fmauch commented Sep 4, 2023 •
edited
Loading

@urmahp I found one more flaw when passing reconnection time to the socket. The tests were succeeding but taking too long, since the default timeout is 10 seconds. I added a limit of 1.5 seconds (since the connection should take 2*500ms+overhead). This way we can make sure that the set timeout is actually being used.

Edit: Seeing that two tests now fail because of taking too long. Going to continue on this tomorrow.

@fmauch
Copy link
Collaborator Author

fmauch commented Sep 5, 2023

@urmahp If you could give it one more review, that would be great. I didn't clean up the commits since this seems to confuse coverage.io quite a lot. I'll do that right before merging.

urmahp
urmahp approved these changes Sep 5, 2023
View reviewed changes
Copy link
Collaborator

@urmahp urmahp left a comment

Choose a reason for hiding this comment

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

Overall it looks very good. I only have a small comment regarding include of ratio in the test, which I believe is not used.

Other than that I feel fine with merging this.

tests/test_producer.cpp Outdated Show resolved Hide resolved
tests/test_tcp_socket.cpp Outdated Show resolved Hide resolved
@fmauch fmauch merged commit b90a3ec into UniversalRobots:master Sep 5, 2023
18 of 19 checks passed
@fmauch fmauch deleted the max_num_tries branch September 5, 2023 14:04
@fmauch fmauch mentioned this pull request Sep 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urmahp urmahp urmahp approved these changes

@urrsk urrsk Awaiting requested review from urrsk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 1.3.3
Development

Successfully merging this pull request may close these issues.

TCPSocket loops forever - add timeout or max num tries
3 participants
@fmauch @urmahp @michaelaeriksen