Fix connect error reporting (#38, #39) and three status/response mismatches (#47) - #56
Merged
Merged
Conversation
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 <noreply@anthropic.com>
…efused the connection" 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 <noreply@anthropic.com>
…nt sweep
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 <noreply@anthropic.com>
… feedback) 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three connect/response fixes, all exercised against a live ILCE-7M5 (
CFCA6014E092) over USB, SDK V2.02.00, darwin-arm64. One commit per issue; each builds independently.ff98a9cSDK::Connectstatus discarded963d27d96f942c#39 is the one that matters, and it is more common than the issue suggests
The issue attributes
0x8208to a marginal USB link. I reproduced it 3/3 and deterministically from process contention — a second server attempting to connect a camera another process already holds. That is not an exotic hardware fault; it is what happens whenever the MCP extension's server and a manually started one coexist, or a leftover process is still holding the camera. I hit that state twice by accident while working on this repo.Before:
Both halves wrong: the cause is another process, and the advice sends you to credentials. After, it names the two real causes and the credentials hint is suppressed for non-auth codes.
#47 item 3 had a different root cause than filed
The issue reads as "the camera block is empty", implying it is never populated. In fact
executeZoomAction()fills it correctly viapopulateResponseCamera()— the RESTful action dispatcher then copied onlysuccess/message/dataout of the result and dropped.camera. Worth knowing before someone goes looking for a missing populate call.What is not verified
SDK::Connectfailure — process contention, contention with credentials, and a connect afterSIGKILLing a server holding the camera — all returned the real SDK code via the async path, which already worked. The defect is plain in the code, but its branch may be unreachable on this platform/SDK version. The fix is a strict improvement and costs nothing, but the priority on connect() discards the synchronous SDK::Connect status — failures report a 15s timeout that never happened #38 should probably drop.0x8219,0x8211,0x820B).0x8219was observed on this hardware before the change, but I could not reproduce it on demand afterwards to see the new text.Regression check
Against the live camera after the change:
properties/all200,properties/iso200,live-view/status200, SD-card listing 200 (280 files),logs?lines=3200, disconnect 200, reconnectremote-transfer200. Negative cases held too — a valid mode still short-circuits to200 "Camera already connected", and/properties/allstill returns 200 for a real camera.Scope note
48 handlers pass a result to
toJson()and only about ten set a status code, so most endpoints return 200 regardless of outcome. Only the endpoint named in #47 is changed here; altering the other 38 is a far larger behavioural change and deserves its own discussion.🤖 Generated with Claude Code