From ff98a9ca9a9a107ee17b889d3fc23e3ef4dadf22 Mon Sep 17 00:00:00 2001 From: jordlee Date: Thu, 13 Aug 2026 15:20:09 -0700 Subject: [PATCH 1/4] fix(server): keep the SDK status when SDK::Connect fails synchronously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit connect() zeroed m_lastError and then dropped the status on the synchronous failure path. The controller only calls wait_for_connection() when connect() succeeded, so last_error() was still 0 and the response fell into the "no SDK error, therefore it timed out" branch — reporting a 15s timeout for a call that returned immediately, and discarding the one value that identifies the fault. Store the status before returning, and gate the timeout branch on a `waited` flag so "did not complete within 15s" can only be claimed when a wait actually ran. A synchronous failure that reports no code now says exactly that instead of inventing a duration. Not reproduced at runtime. Three attempts on hardware (macOS, ILCE-7M5) to force a synchronous failure — process contention, contention with credentials, and a connect after SIGKILLing a server holding the camera — all returned the real SDK code via the async path, which already behaved correctly. The defect is plain in the code, but its branch may be unreachable on this platform and SDK version, so this ships code-verified only. Refs #38 Co-Authored-By: Claude Opus 5 --- api/server/src/device/CameraDeviceRest.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/api/server/src/device/CameraDeviceRest.cpp b/api/server/src/device/CameraDeviceRest.cpp index 7ae15fa..d74ca66 100644 --- a/api/server/src/device/CameraDeviceRest.cpp +++ b/api/server/src/device/CameraDeviceRest.cpp @@ -103,6 +103,11 @@ bool CameraDeviceRest::connect(SCRSDK::CrSdkControlMode openMode, inputId, password.c_str(), fingerprint.c_str(), static_cast(fingerprint.size())); if (CR_FAILED(status)) { + // Store the status before bailing out. Without this the caller sees + // last_error() == 0, skips wait_for_connection() (it only runs when + // connect() succeeded), and reports a timeout that never happened — + // discarding the one piece of information that identifies the fault. + m_lastError.store(status); return false; } // Default save destination to the current working directory. From 963d27d9beff683f1b2f28356d0fcbdd6f857ec5 Mon Sep 17 00:00:00 2001 From: jordlee Date: Thu, 13 Aug 2026 15:20:09 -0700 Subject: [PATCH 2/4] fix(server): stop reporting transport and session errors as "Camera refused the connection" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only three SSH codes had their own case; every other connect error fell to a default that blamed the camera and, when credentials were supplied, told the caller to go check their password. That is wrong for the transport codes, and it is wrong in the most common case of all. Reproduced on hardware (macOS, ILCE-7M5), 3/3 and deterministic: a second process attempting to connect a camera another process already holds yields CrError_Connect_TimeOut (0x8208) — reported as a refusal. This is not the marginal-USB-link scenario the issue describes; it is what happens whenever two servers coexist, which is routine. - 0x8208 Connect_TimeOut — name both real causes: another process on this machine holding the camera, or the link (hub / cable / port). - 0x8219 Connect_RemoteTransfer_NotSupported — observed after a SIGKILLed server left a session open; suggests the working recovery (connect 'remote', disconnect cleanly, retry). - 0x8210 Connect_SessionAlreadyOpened, 0x820B Connect_FailBusy, 0x8211 Connect_ContentsTransfer_NotSupported — own cases. - isSshAuthError() gates the "check username, password and fingerprint" hint so it only appears for codes that are actually about authentication. Verified on hardware: 0x8208 and 0x8210 render their new messages, and the credentials hint is correctly suppressed for a transport failure even when credentials were supplied. The other three cases are code-only — I could not induce those conditions on demand. Note the enum values are not where a naive count lands: CrError_Reconnect_TimeOut and two Reserved entries are interleaved (shared/sdk/include/CrError.h). Refs #39 Co-Authored-By: Claude Opus 5 --- api/server/src/CameraWebController.cpp | 86 +++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/api/server/src/CameraWebController.cpp b/api/server/src/CameraWebController.cpp index d0a6366..194dc4a 100644 --- a/api/server/src/CameraWebController.cpp +++ b/api/server/src/CameraWebController.cpp @@ -495,6 +495,33 @@ ApiResponse CameraWebController::getStatus() { return response; } +namespace { + +/** + * Is this connect error actually about access authentication? + * + * Used to decide whether "check your username, password and fingerprint" is + * useful advice. It was previously appended to every unrecognised code whenever + * credentials had been supplied, which meant a transport failure — a USB hub, or + * another process holding the camera — told you to go check your password. + */ +bool isSshAuthError(unsigned int err) { + switch (err) { + case SDK::CrError_Connect_SSH_NotSupported: + case SDK::CrError_Connect_SSH_InvalidParameter: + case SDK::CrError_Connect_SSH_ServerConnectFailed: + case SDK::CrError_Connect_SSH_ServerAuthenticationFailed: + case SDK::CrError_Connect_SSH_UserAuthenticationFailed: + case SDK::CrError_Connect_SSH_PortForwardFailed: + case SDK::CrError_Connect_SSH_GetFingerprintFailed: + return true; + default: + return false; + } +} + +} // namespace + ApiResponse CameraWebController::connectCamera(const std::string& connectionMode, const std::string& cameraId, const std::string& username, const std::string& password, const std::string& reconnecting, const std::string& fingerprint) { std::lock_guard lock(m_discoveryMutex); @@ -622,7 +649,12 @@ ApiResponse CameraWebController::connectCamera(const std::string& connectionMode // outcome lands on OnConnected or OnError afterwards. Reporting success off // the synchronous return made every connect look like it worked — including // one with a deliberately wrong password. Wait for the actual answer. + // Whether we actually waited. Only a real wait entitles us to say "timed + // out"; a synchronous SDK::Connect failure never waits, and claiming a + // 15s timeout there names a cause and a duration that did not happen. + bool waited = false; if (connect_result) { + waited = true; connect_result = targetCamera->wait_for_connection(kConnectTimeoutMs); } @@ -703,20 +735,70 @@ ApiResponse CameraWebController::connectCamera(const std::string& connectionMode std::string("This camera does not support access " "authentication (") + hex + ")."; break; + + // Transport / session-state codes. These are not refusals, and + // labelling them as such sends diagnosis to camera settings + // when the cause is the link or another process holding the + // camera. Reproduced on hardware: simply having a second + // process already connected yields TimeOut, not Busy. + case SDK::CrError_Connect_TimeOut: + response.message = + std::string("Timed out establishing the session (") + hex + + "). Most often another process on this machine already " + "has the camera — check for a second server or a leftover " + "process. Otherwise it is the link: if the camera is on a " + "USB hub try a direct port, or check the cable. For a " + "network body, confirm remote shooting is enabled."; + break; + case SDK::CrError_Connect_RemoteTransfer_NotSupported: + response.message = + std::string("This camera cannot enter remote-transfer mode " + "right now (") + hex + "). A previous session may " + "still be held open — connect in 'remote' mode, disconnect " + "cleanly, then retry."; + break; + case SDK::CrError_Connect_ContentsTransfer_NotSupported: + response.message = + std::string("This camera does not support contents-transfer " + "mode (") + hex + "). Use 'remote' or " + "'remote-transfer'."; + break; + case SDK::CrError_Connect_FailBusy: + response.message = + std::string("The camera is busy (") + hex + "). Another " + "client may hold the session, or the body is mid-operation."; + break; + case SDK::CrError_Connect_SessionAlreadyOpened: + response.message = + std::string("A session is already open on this camera (") + + hex + "). Disconnect the existing session before " + "reconnecting."; + break; + default: response.message = std::string("Camera refused the connection (") + hex + ")"; - if (!username.empty()) { + // Only point at credentials for codes that are actually about + // authentication; the catch-all previously appended this to + // transport errors too. + if (!username.empty() && isSshAuthError(err)) { response.message += ". Check the access-authentication " "username, password and fingerprint."; } break; } - } else { + } else if (waited) { response.message = "Connection did not complete within " + std::to_string(kConnectTimeoutMs / 1000) + "s. If this is a network connection, check that remote " "shooting is enabled on the camera."; + } else { + // connect() failed synchronously and reported no code. Say exactly + // that rather than inventing a wait that never happened. + response.message = + "The SDK rejected the connection request immediately, without " + "reporting an error code. This usually means the camera is no " + "longer present — re-run discovery (GET /api/cameras) and retry."; } std::cout << "❌ Web controller failed to connect to camera: " From 96f942c4b115162e232a5a9e7c4220d48a9e8174 Mon Sep 17 00:00:00 2001 From: jordlee Date: Thu, 13 Aug 2026 15:20:09 -0700 Subject: [PATCH 3/4] fix(server): correct three status/response mismatches from the endpoint sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three reproduced on hardware (macOS, ILCE-7M5) before the change and verified after, including the negative cases. 1. GET /properties/all returned 200 for an unknown or disconnected camera, with the failure visible only as "success": false in the body. openapi.yaml documents 400 for exactly this. Now 400 on failure, still 200 on success. 2. POST /connection ran the already-connected short-circuit before validating the mode, so an invalid mode returned 200 "Camera already connected" whenever a camera happened to be connected — and only rejected it correctly while disconnected. Mode is now validated first; a valid mode still short-circuits. 3. A successful zoom reported camera {connected:false, model:"", id:""}. The cause was not a missing populate call: executeZoomAction() fills the block via populateResponseCamera(), but the RESTful action dispatcher copied only success/message/data out of the result and dropped .camera. One assignment, discarding work already done. Scope note: 48 handlers pass a result to toJson() and only about ten set a status code, so most endpoints still return 200 regardless of outcome. Only the endpoint named in the issue is changed here — altering the other 38 is a much larger behavioural change than this fix warrants and deserves its own discussion. Refs #47 Co-Authored-By: Claude Opus 5 --- api/server/src/CameraWebController.cpp | 18 ++++++++++++++++++ api/server/src/CameraWebServer.cpp | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/api/server/src/CameraWebController.cpp b/api/server/src/CameraWebController.cpp index 194dc4a..c47ef50 100644 --- a/api/server/src/CameraWebController.cpp +++ b/api/server/src/CameraWebController.cpp @@ -527,6 +527,18 @@ ApiResponse CameraWebController::connectCamera(const std::string& connectionMode ApiResponse response; + // Validate the mode before anything else. This check used to live below the + // already-connected short-circuit, so an invalid mode returned + // 200 "Camera already connected" whenever a camera happened to be connected + // — reporting success for a request that could never have been honoured, + // and only rejecting it correctly while disconnected. + if (connectionMode != "remote" && connectionMode != "contents" + && connectionMode != "remote-transfer") { + response.success = false; + response.message = "Invalid connection mode. Use: 'remote', 'contents', or 'remote-transfer'"; + return response; + } + // Check if this specific camera is already connected (per-camera check) if (!cameraId.empty()) { std::lock_guard threadLock(m_cameraThreadsMutex); @@ -6956,6 +6968,12 @@ ApiResponse CameraWebController::executeActionGeneric(const std::string& cameraI response.success = zoomResult.success; response.message = zoomResult.message; response.data = zoomResult.data; + // executeZoomAction() fills the camera block via + // populateResponseCamera(); copying only success/message/data + // dropped it on the floor, so every zoom response — including + // successful ones on a live camera — reported + // {connected:false, model:"", id:""}. + response.camera = zoomResult.camera; return response; } else if (actionName == "focus-near-far") { diff --git a/api/server/src/CameraWebServer.cpp b/api/server/src/CameraWebServer.cpp index 6965327..443ad28 100644 --- a/api/server/src/CameraWebServer.cpp +++ b/api/server/src/CameraWebServer.cpp @@ -1524,6 +1524,11 @@ HttpResponse CameraWebServer::handleApiGetAllProperties(const std::string& camer HttpResponse response; response.contentType = "application/json"; + // openapi.yaml documents 400 for "camera not connected or bulk retrieval + // failed"; this handler previously left the default 200 in place, so a + // failure was reported as success at the HTTP layer and only contradicted + // by the body's "success": false. + response.statusCode = result.success ? 200 : 400; response.body = m_cameraController->toJson(result); return response; } From fbb4c7b4db8933329d334e93234f8d98e8b11d0d Mon Sep 17 00:00:00 2001 From: jordlee Date: Thu, 13 Aug 2026 15:25:11 -0700 Subject: [PATCH 4/4] refactor(server): single source of truth for connection modes (review feedback) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues found reviewing the preceding commits. 1. The new early mode-validation enumerated the three accepted modes, and the parsing chain below enumerated them again — two lists that had to stay in sync, with the second one's else branch now unreachable while still looking like validation. Adding a mode to one and not the other would have failed silently and confusingly. Both now read one table. The parsing could not simply be hoisted above the already-connected short-circuit to serve as the validation, because it also assigns m_currentConnectionMode, and doing that on a path that returns early would mutate connection state for a request that was never acted on — so the table is the way to share it without changing that behaviour. 2. The `waited` comment ran straight into the pre-existing comment above it, reading as one run-on block. Separated. Re-verified on hardware (macOS, ILCE-7M5) after the refactor: all three modes connect (remote, remote-transfer, and contents — the last exercised for the first time here), an invalid mode returns 400 both connected and disconnected, and a valid mode while connected still short-circuits to 200. Refs #47 Co-Authored-By: Claude Opus 5 --- api/server/src/CameraWebController.cpp | 58 ++++++++++++++++---------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/api/server/src/CameraWebController.cpp b/api/server/src/CameraWebController.cpp index c47ef50..64a808d 100644 --- a/api/server/src/CameraWebController.cpp +++ b/api/server/src/CameraWebController.cpp @@ -497,6 +497,30 @@ ApiResponse CameraWebController::getStatus() { namespace { +/** + * The accepted `mode` values, and what each maps to. + * + * Single source of truth: the validation below and the parsing further down + * both read this, so they cannot drift into disagreeing about which modes are + * legal. The parsing cannot simply be hoisted above the already-connected + * short-circuit to serve as the validation, because it also assigns + * m_currentConnectionMode — doing that on a path that currently returns early + * would mutate connection state for a request that was never acted on. + */ +struct ConnectionModeSpec { + SDK::CrSdkControlMode sdkMode; + ConnectionMode mode; +}; + +const std::map& connectionModeTable() { + static const std::map kModes = { + {"remote", {SDK::CrSdkControlMode_Remote, ConnectionMode::Remote}}, + {"contents", {SDK::CrSdkControlMode_ContentsTransfer, ConnectionMode::ContentsTransfer}}, + {"remote-transfer", {SDK::CrSdkControlMode_RemoteTransfer, ConnectionMode::RemoteTransfer}}, + }; + return kModes; +} + /** * Is this connect error actually about access authentication? * @@ -532,8 +556,8 @@ ApiResponse CameraWebController::connectCamera(const std::string& connectionMode // 200 "Camera already connected" whenever a camera happened to be connected // — reporting success for a request that could never have been honoured, // and only rejecting it correctly while disconnected. - if (connectionMode != "remote" && connectionMode != "contents" - && connectionMode != "remote-transfer") { + const auto modeIt = connectionModeTable().find(connectionMode); + if (modeIt == connectionModeTable().end()) { response.success = false; response.message = "Invalid connection mode. Use: 'remote', 'contents', or 'remote-transfer'"; return response; @@ -554,24 +578,11 @@ ApiResponse CameraWebController::connectCamera(const std::string& connectionMode } } - // Parse connection mode - SDK::CrSdkControlMode sdkMode = SDK::CrSdkControlMode_Remote; // Default - m_currentConnectionMode = ConnectionMode::Remote; - - if (connectionMode == "remote") { - sdkMode = SDK::CrSdkControlMode_Remote; - m_currentConnectionMode = ConnectionMode::Remote; - } else if (connectionMode == "contents") { - sdkMode = SDK::CrSdkControlMode_ContentsTransfer; - m_currentConnectionMode = ConnectionMode::ContentsTransfer; - } else if (connectionMode == "remote-transfer") { - sdkMode = SDK::CrSdkControlMode_RemoteTransfer; - m_currentConnectionMode = ConnectionMode::RemoteTransfer; - } else { - response.success = false; - response.message = "Invalid connection mode. Use: 'remote', 'contents', or 'remote-transfer'"; - return response; - } + // Apply the mode. Validity was already established above, and modeIt points + // into the same table, so there is no second list of accepted values to + // keep in sync — and no unreachable else branch pretending to validate. + const SDK::CrSdkControlMode sdkMode = modeIt->second.sdkMode; + m_currentConnectionMode = modeIt->second.mode; std::cout << "🔗 Attempting connection in mode: " << connectionMode << std::endl; @@ -661,9 +672,10 @@ ApiResponse CameraWebController::connectCamera(const std::string& connectionMode // outcome lands on OnConnected or OnError afterwards. Reporting success off // the synchronous return made every connect look like it worked — including // one with a deliberately wrong password. Wait for the actual answer. - // Whether we actually waited. Only a real wait entitles us to say "timed - // out"; a synchronous SDK::Connect failure never waits, and claiming a - // 15s timeout there names a cause and a duration that did not happen. + // + // Track whether we actually waited: only a real wait entitles us to say + // "timed out". A synchronous SDK::Connect failure never waits, and claiming + // a 15s timeout there names a cause and a duration that did not happen. bool waited = false; if (connect_result) { waited = true;