Skip to content

fix(libwaterlinked): harden TCP command handling - #32

Merged
wltry merged 5 commits into
waterlinked:mainfrom
Kurtuveys:fix/dvl-driver-hardening
Aug 17, 2026
Merged

fix(libwaterlinked): harden TCP command handling#32
wltry merged 5 commits into
waterlinked:mainfrom
Kurtuveys:fix/dvl-driver-hardening

Conversation

@Kurtuveys

Copy link
Copy Markdown
Contributor

Summary

This PR improves reliability of the Water Linked TCP client and callback handling.

The TCP command path had several reliability risks:

  • pending_requests_ was accessed from multiple threads without locking.
  • A fast DVL response could arrive before the promise was registered.
  • Pending command requests could remain unresolved when the DVL did not respond.
  • Pending requests were not failed explicitly when the TCP connection was lost.
  • The socket could leak if connection setup failed after socket() succeeded.
  • Report callback vectors could be modified while the polling thread was iterating over them.

Changes

  • Protect pending_requests_ with request_mutex_.
  • Register the pending promise before sending the command.
  • Add command response timeout handling.
  • Fail all pending commands on connection loss.
  • Close the socket on constructor connection failure paths.
  • Protect report callback registration and callback iteration with a mutex.

Validation

Tested with:

colcon build --packages-up-to waterlinked_dvl_driver
colcon test --packages-select libwaterlinked --event-handlers console_direct+
colcon test-result --verbose

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the libwaterlinked TCP client’s command/response and callback handling to improve reliability under concurrency, fast responses, timeouts, and connection-loss scenarios, and updates the ROS driver wiring to pass the new timeout configuration.

Changes:

  • Extend WaterLinkedClient to support a configurable command-response timeout and track pending requests with deadlines.
  • Add mutex protection around pending command requests and around callback registration/iteration.
  • Update the driver to pass the timeout for both connection setup and command responses, and clarify the parameter description.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
waterlinked_dvl_driver/src/waterlinked_dvl_driver.cpp Passes the timeout into the updated WaterLinkedClient constructor (connection + command).
waterlinked_dvl_driver/src/waterlinked_dvl_driver_parameters.yaml Clarifies that the timeout applies to both connection and command responses.
libwaterlinked/src/client.cpp Implements pending-request locking, deadline-based timeouts, connection-loss failure, and callback locking.
libwaterlinked/include/libwaterlinked/client.hpp Updates the public API and internal pending-request tracking structures to support command timeouts and locking.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread libwaterlinked/src/client.cpp
@wltry

wltry commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Thank you for the pull request - and sorry for taking so long to review it. In general it looks sane, with protecting requests and handling of socket connection. It seems that the locking of request_mutex_ when calling the blocking send() could be avoided, and rather use a send_mutex to protect writing to the socket. Nothing that can't be fixed after merge, but I'll keep it open for a little while longer.

@wltry wltry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. Fixes issue #30 and improves TCP socket handling.

@wltry
wltry merged commit 56f3c5d into waterlinked:main Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants