Conversation
max246
left a comment
There was a problem hiding this comment.
Nice work — the approach is right and the code is correct. I traced the dotted-path walk: a bare verb (left) resolves on the ptz surface, preset.goto walks ptz().preset().goto(id) with args landing only on the leaf, and a missing verb or missing leaf both fail cleanly. Keeping it a path-walk instead of per-verb cases is the right call — the transport stays capability-agnostic, and hardware-verifying the movement verbs + preset list/preview is exactly what a fire-and-forget P2P control needs. Tests and the new device.action protocol section are thorough.
One thing before it can merge: it's currently CONFLICTING, and that's on us — two changes landed on dev after you branched and both touch the same device.action block:
- the ws-server refactor (#52) reorganized the file and reformatted the
surfacesarray to multi-line, and - the lock PR (#49) added
dev.lock?.()to that array.
So the resolution is mechanical — rebase onto dev and the merged case is your change on top of theirs:
const surfaces = [dev.smartLight?.(), dev.camera?.(), dev.lock?.(), dev.ptz?.()].filter(Boolean);
const path = action.split(".");
const leaf = path.pop();
let target = surfaces.find((s) => typeof s?.[path[0] ?? leaf] === "function");
for (const seg of path) target = typeof target?.[seg] === "function" ? target[seg]() : undefined;
if (typeof target?.[leaf] !== "function") return fail(`no action '${action}' on ${msg.sn}`);
// ...
const result = await target[leaf](...args);That's just: keep dev.lock?.() from #49, add your dev.ptz?.(), and apply your walk logic in the new (single-switch) structure. Approving on the code; give it a rebase and it's good to merge. (CI also hasn't reported on the branch yet — the rebase push should kick it.)
|
@Filpin011 can you fix the conflict? |
|
Yeah, tomorrow i'll fix it |
`device.action` resolved a verb against `[smartLight, camera]` only, so every pan-tilt control a host might draw came back `no action 'left'` — the PTZ verbs live on `dev.ptz()`, which was never consulted. Add that surface. Preset operations sit one level deeper: `preset` ANSWERS rather than acts (it hands back the sub-API namespace), so a flat lookup can't reach `goto`/`save`. Resolve a dotted `action` by walking the namespace — every segment before the leaf is called with no arguments, and only the leaf receives `args`. That makes `preset.goto` mean `dev.ptz().preset().goto(id)` with no per-verb wiring, and the same mechanism covers any future sub-API. Document `device.action` (it had no protocol section at all) and correct "Not yet exposed", which still listed PTZ move. Pan calibration stays listed there: 6017/6251 are raw P2P command ids the SDK never promotes to capability members, so `device.action` cannot reach them by design.
bcb0001 to
e070476
Compare
|
@max246 done |
What
device.actionresolved a verb against[smartLight, camera]only. The PTZ verbs live ondev.ptz(), which was never consulted, so every pan-tilt control a host might draw came backno action 'left'—ws-protocol.mdlisted "PTZ move" under Not yet exposed for exactly thisreason.
Adding the surface is one entry in that list. Presets needed a little more:
presetanswersrather than acts — it hands back the sub-API namespace — so a flat lookup cannot reach
goto/save/list. A dottedactionnow walks the namespace: every segment before the leaf iscalled with no arguments, and only the leaf receives
args.preset.gototherefore meansdev.ptz().preset().goto(id), with no per-verb wiring, and the same mechanism covers any sub-APIadded later.
Why it is shaped this way
case "preset.goto"per verb would need touching thisfile for every future one. Walking the path keeps the transport capability-agnostic, which is the
line the rest of the file already holds.
CMD_INDOOR_PAN_CALIBRATION(6017) andCMD_OUTDOOR_PAN_CALIBRATION(6251) are raw P2P command ids the SDK never promotes to capabilitymembers, so
device.actioncannot reach them by design — worth saying out loud, since a hostauthor will look for a calibrate verb and not find one.
Docs
device.actionhad no protocol section at all; it has one now, including the fire-and-forgetcaveat (P2P sends no ack, so
ok: truemeans the frame left, not that the camera moved) and thesilent no-op on an empty preset slot. Not yet exposed is corrected accordingly.
Tests
wiring.test.mjsgains a pan-tilt device in the fake SDK and covers: a bare verb resolving on theptz surface, a dotted path reaching the preset namespace with the args landing on the leaf only, a
missing verb and a missing leaf both failing cleanly, and a fixed camera refusing movement.
Note for reviewers on Windows:
config.test.mjsfails there before and after this change, on apath separator (
data\.eufy-fcm.jsonvsdata/.eufy-fcm.json). Unrelated, and green in CI.Verified on hardware
The four movement verbs and
preset.list/preset.previewwere driven against a real pan-tiltcamera through this branch.