Fixed auto reconnection to the RTDE server. - #384
Conversation
When reconnecting to RTDE it requires you to set the input and output recipes again, this is now done properly.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #384 +/- ##
==========================================
- Coverage 78.22% 76.51% -1.72%
==========================================
Files 92 92
Lines 4212 4283 +71
Branches 467 482 +15
==========================================
- Hits 3295 3277 -18
- Misses 680 761 +81
- Partials 237 245 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Also fixed that we can destroy the rtdeClient while reconnecting
urfeex
left a comment
urfeex
left a comment
There was a problem hiding this comment.
There seems to be a race condition between destroying the RTDE client and the reconnection mechanism. E.g. when the client is destroyed right after reading failed, the reconnection mechanism is still called leaving the program hanging there.
| if (stream_.getStreamType() == URStreamType::RTDE) | ||
| { | ||
| if (on_rtde_reconnect_cb_) | ||
| { | ||
| URCL_LOG_WARN("Failed to read from RTDE stream, invoking on reconnect callback and stopping the producer"); | ||
| on_rtde_reconnect_cb_(); | ||
| } | ||
| else | ||
| { | ||
| URCL_LOG_ERROR("Failed to read from RTDE stream without a reconnect handler stopping the producer"); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I didn't really like the special handling of an RTDE producer here. Couldn't we generalize this by checking whether there is a reconnection callback registered? If none then we simply do not reconnect and if yes, trigger reconnect independent of the stream type. Sorry that I just come up with this now, but that seems cleaner to me.
There was a problem hiding this comment.
Reconnecting to the stream, will just not have any affect if it is a RTDE stream, but I will change the check.
Applied suggestions from code review
urfeex
left a comment
urfeex
left a comment
There was a problem hiding this comment.
Just some nitpicky suggestions left that I can commit mys
| } | ||
|
|
||
| bool RTDEClient::negotiateProtocolVersion(const uint16_t protocol_version) | ||
| std::pair<bool, uint16_t> RTDEClient::setProtocolVersion() |
There was a problem hiding this comment.
It seems like we are only interested in the boolean return value, so should we reduce the signature to that one?
Co-authored-by: Felix Exner <feex@universal-robots.com>
Co-authored-by: Felix Exner <feex@universal-robots.com>
2 participants
When reconnecting to RTDE it requires you to set the input and output recipes again, this is now done properly.