Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves UDP socket settings to support sending large amounts of data (e.g., configuration dumps) between remote gateways. It parameterizes previously hardcoded values in acknowledge() and sendAndReceive(), and updates the remote gateway socket to use a 4MB receive buffer.
Changes:
acknowledge()inTransceiverSocketnow accepts a configurablemaxChunkSizeparameter (defaulting to 1500) instead of a hardcoded constant, allowing larger UDP datagrams (up to 65500 bytes) when sending large payloads.sendAndReceive()inTransceiverSocketnow accepts a configurableinterPacketTimeoutUSecondsparameter (defaulting to 10000 µs) instead of a hardcoded 10ms, enabling longer wait between packets for large multi-packet responses.GatewaySupervisoris updated to initialize the remote gateway socket with a 4MB receive buffer, use 200ms inter-packet timeout for command responses, and emit more informative diagnostic messages on timeout.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
otsdaq/NetworkUtilities/TransceiverSocket.h |
Adds maxChunkSize and interPacketTimeoutUSeconds parameters to acknowledge() and sendAndReceive() |
otsdaq/NetworkUtilities/TransceiverSocket.cc |
Implements the new parameters, replacing hardcoded 1500 and 10000 constants |
otsdaq/GatewaySupervisor/GatewaySupervisor.cc |
Increases receive buffer to 4MB, uses 200ms inter-packet timeout for commands, and improves timeout error diagnostics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(commandResponseString.empty()) | ||
| { | ||
| ss << "No data was received at all."; | ||
| } | ||
| else | ||
| { | ||
| const size_t maxPrint = 500; | ||
| ss << "Received " << commandResponseString.size() << " bytes so far. "; | ||
| if(commandResponseString.size() <= maxPrint) | ||
| { | ||
| ss << "Full received text: [" << commandResponseString << "]"; | ||
| } | ||
| else | ||
| { | ||
| ss << "First " << maxPrint << " chars: [" | ||
| << commandResponseString.substr(0, maxPrint) << "] ... Last " | ||
| << maxPrint << " chars: [" | ||
| << commandResponseString.substr(commandResponseString.size() - | ||
| maxPrint) | ||
| << "]"; | ||
| } |
There was a problem hiding this comment.
The check if(commandResponseString.empty()) at line 2843 can never be true. The code only reaches this block when commandResponseString.size() > strlen("Done") + 1 (i.e., the string is non-empty with more than 5 characters). The additional receive calls in the loop can only append data to commandResponseString, never empty it. The "No data was received at all." branch is therefore dead code.
Consider removing the dead commandResponseString.empty() branch, or restructuring the logic to check the initial empty state before this block.
| if(commandResponseString.empty()) | |
| { | |
| ss << "No data was received at all."; | |
| } | |
| else | |
| { | |
| const size_t maxPrint = 500; | |
| ss << "Received " << commandResponseString.size() << " bytes so far. "; | |
| if(commandResponseString.size() <= maxPrint) | |
| { | |
| ss << "Full received text: [" << commandResponseString << "]"; | |
| } | |
| else | |
| { | |
| ss << "First " << maxPrint << " chars: [" | |
| << commandResponseString.substr(0, maxPrint) << "] ... Last " | |
| << maxPrint << " chars: [" | |
| << commandResponseString.substr(commandResponseString.size() - | |
| maxPrint) | |
| << "]"; | |
| } | |
| const size_t maxPrint = 500; | |
| ss << "Received " << commandResponseString.size() << " bytes so far. "; | |
| if(commandResponseString.size() <= maxPrint) | |
| { | |
| ss << "Full received text: [" << commandResponseString << "]"; | |
| } | |
| else | |
| { | |
| ss << "First " << maxPrint << " chars: [" | |
| << commandResponseString.substr(0, maxPrint) << "] ... Last " | |
| << maxPrint << " chars: [" | |
| << commandResponseString.substr(commandResponseString.size() - | |
| maxPrint) | |
| << "]"; |
There was a problem hiding this comment.
fair point but I think it doesn't really hurt to keep this in
| << std::endl; | ||
|
|
||
| constexpr size_t MAX_SEND_SIZE = 1500; | ||
| const size_t MAX_SEND_SIZE = maxChunkSize; |
There was a problem hiding this comment.
The local variable MAX_SEND_SIZE is a redundant alias for maxChunkSize — it is initialized from maxChunkSize and never modified. Consider using maxChunkSize directly throughout the function body to reduce indirection and make the code clearer.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
changed to UDP settings to allow a 'large' amount of data to be sent to the top level