Skip to content

Commit e84c769

Browse files
Remove save-bump-restore from connect()
Reviewer feedback: the save-bump-restore in connect() mimics G5's pattern but for the wrong reason. G5 bumps because it does an unsolicited blocking read for the dashboard server's boot-up message after TCPSocket::setup() — explicitly called out in the G5 source ("The first read after connection can take more time"). PolyScope X's connect() just issues an HTTP GET for /openapi.json (~200 ms over VPN per maintainer measurement); no analogous unsolicited read exists. The original cursor-bot concern that motivated b1f900d was that the 1 s constructor read_timeout left openapi.json under-resourced while in-tree callers raised the timeout only post-connect. That concern was resolved by bumping the constructor default from 1 s to 10 s, which gives openapi.json the same headroom the example wrapper applies manually. The save-bump-restore became dead weight: a no-op for default callers, and it overrides explicit caller values shorter than 10 s — paternalistic behavior the reviewer is rightly pushing back on. Drop the bump, the try/catch(...) rethrow guard, and the restore. connect() returns to its pre-b1f900d shape (early returns from the GET-and-parse pipeline). The recv_timeout_ storage stays because commandPowerOn still uses it for its own per-call save-bump-restore, which is a different and legitimate case (caller-supplied longer deadline for an operation known to take longer).
1 parent cdd9988 commit e84c769

1 file changed

Lines changed: 18 additions & 51 deletions

File tree

src/ur/dashboard_client_implementation_x.cpp

Lines changed: 18 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -91,61 +91,28 @@ std::string DashboardClientImplX::sendAndReceive([[maybe_unused]] const std::str
9191
bool DashboardClientImplX::connect([[maybe_unused]] const size_t max_num_tries,
9292
[[maybe_unused]] const std::chrono::milliseconds reconnection_time)
9393
{
94-
// The initial openapi.json fetch can take significantly more time than steady-state
95-
// dashboard calls (larger payload, first-contact handshake). Mirror the G5 pattern:
96-
// temporarily extend the read timeout for setup, then restore the configured value.
97-
// The restore must run on every exit, including exceptions from json::parse or
98-
// VersionInformation::fromString — hence the catch(...) rethrow guard.
99-
//
100-
// The 10 s here is a minimum-headroom for setup, not a cap on the caller's
101-
// preferences. If the caller (or an enclosing call like commandPowerOn) has already
102-
// configured a larger read timeout, never shrink it — a lazy connect() inside such a
103-
// call would otherwise reduce the in-flight deadline during the openapi.json GET.
104-
timeval configured_tv = getConfiguredReceiveTimeout();
105-
constexpr time_t kSetupMinSeconds = 10;
106-
timeval setup_tv = configured_tv;
107-
if (setup_tv.tv_sec < kSetupMinSeconds)
108-
{
109-
setup_tv.tv_sec = kSetupMinSeconds;
110-
setup_tv.tv_usec = 0;
111-
}
112-
setReceiveTimeout(setup_tv);
113-
114-
bool result = false;
115-
try
94+
std::string endpoint = base_url_ + "/openapi.json";
95+
// The PolyScope X Robot API doesn't require any connection prior to making calls. However, this
96+
// check call will assurea that the endpoint for making Robot API calls exist. This could fail if
97+
// the IP address is wrong or the robot at the IP doesn't have the necessary software version.
98+
if (auto res = cli_->Get(endpoint))
11699
{
117-
std::string endpoint = base_url_ + "/openapi.json";
118-
// The PolyScope X Robot API doesn't require any connection prior to making calls. However, this
119-
// check call will assure that the endpoint for making Robot API calls exist. This could fail if
120-
// the IP address is wrong or the robot at the IP doesn't have the necessary software version.
121-
if (auto res = cli_->Get(endpoint))
100+
if (res->status != 200)
122101
{
123-
if (res->status != 200)
124-
{
125-
URCL_LOG_ERROR("Received non-200 response code when connecting to Robot API: %d", res->status);
126-
}
127-
else
128-
{
129-
auto db_res = handleHttpResult(res, false);
130-
auto json_data = json::parse(db_res.message);
131-
if (db_res.ok && json_data.contains("info") && json_data["info"].contains("version") &&
132-
json_data["info"]["version"].is_string())
133-
{
134-
robot_api_version_ = VersionInformation::fromString(json_data["info"]["version"]);
135-
URCL_LOG_DEBUG("Connected to Robot API version: %s", robot_api_version_.toString().c_str());
136-
result = true;
137-
}
138-
}
102+
URCL_LOG_ERROR("Received non-200 response code when connecting to Robot API: %d", res->status);
103+
return false;
104+
}
105+
auto db_res = handleHttpResult(res, false);
106+
auto json_data = json::parse(db_res.message);
107+
if (db_res.ok && json_data.contains("info") && json_data["info"].contains("version") &&
108+
json_data["info"]["version"].is_string())
109+
{
110+
robot_api_version_ = VersionInformation::fromString(json_data["info"]["version"]);
111+
URCL_LOG_DEBUG("Connected to Robot API version: %s", robot_api_version_.toString().c_str());
112+
return true;
139113
}
140114
}
141-
catch (...)
142-
{
143-
setReceiveTimeout(configured_tv);
144-
throw;
145-
}
146-
147-
setReceiveTimeout(configured_tv);
148-
return result;
115+
return false;
149116
}
150117

151118
void DashboardClientImplX::disconnect()

0 commit comments

Comments
 (0)