[RSDK-12844] Add do command error responses by hexbabe · Pull Request #68 · viam-modules/orbbec

hexbabe · 2026-03-02T12:51:42Z

Summary

do_command had six silent failure paths that returned an empty {} to the caller with no indication anything went wrong:

Empty command — do_command({}) acquired locks, looked up the device, iterated zero times, and returned {}
Device not connected — throw std::invalid_argument was caught by the outer catch, which logged the error but fell through to return {}
Unknown command key — the if/else chain had no final else, so unrecognized keys silently did nothing and returned {}
Unsupported property name (set_device_property) — property name not in the device's supported list silently returned {}
Unsupported property name (set_device_properties) — same silent {} for the plural/batch variant
Outer catch — logged the exception but returned {} instead of propagating the error

This PR fixes all six to return {"error": "<message>"}, consistent with the pattern already used by dump_pcl_files and the device_control helpers.

Manual Tests

Platform: linux/amd64, Framework Laptop
Module: viam:orbbec:astra2 built from this branch via viam module reload-local
Machine: sean-framework (part ID 08852a48-cafb-47fc-a747-14982036c715)
Device: Orbbec Astra 2 (serial AARY14100X5, USB 3.0)
viam-server: local, connected via Python SDK (viam-sdk)

Test 1 — Happy path (baseline)

await camera.do_command({"get_device_info": {}})
await camera.do_command({"get_orbbec_sdk_version": {}})
await camera.do_command({"get_device_properties": {}})

✅ get_device_info returns device metadata (serial, firmware, hardware version, etc.) with no "error" key.
✅ get_orbbec_sdk_version returns {"version": "2.4.8", "stage_version": "open-source-beta"} with no "error" key.
✅ get_device_properties returns 51 properties — normal operation unaffected.

Test 2 — Unknown command key (new else branch)

await camera.do_command({"bogus_command": True})

✅ Returns {"error": "unknown command: bogus_command"}. Before this PR: returned {}.

Test 3 — Device not connected (replaced throw with error return)

Started module with Astra2 plugged in, confirmed orbbec-1 working via get_device_info, then physically unplugged the USB cable. After ~20s:

await camera.do_command({"get_device_info": {}})

✅ Returns {"error": "device is not connected"}. Before this PR: exception caught by outer catch → returned {}.

Test 4 — Outer catch returns error (injected exception)

Injected throw std::runtime_error("test: simulated exception for outer catch") into the get_orbbec_sdk_version handler to force an exception that reaches the outer catch. Built and deployed (not committed):

await camera.do_command({"get_orbbec_sdk_version": {}})

✅ Returns {"error": "test: simulated exception for outer catch"}. Before this PR: exception logged but returned {}.

Test 5 — Unsupported property name via `set_device_property`

await camera.do_command({"set_device_property": {"OB_PROP_COLOR_GAMMA_INT": 255}})

✅ Returns {"error": "property not supported: OB_PROP_COLOR_GAMMA_INT"}. Before this PR: returned {}.

Test 6 — Unsupported property name via `set_device_properties` (plural)

await camera.do_command({"set_device_properties": {"OB_PROP_COLOR_GAMMA_INT": 255}})

✅ Returns {"error": "property not supported: OB_PROP_COLOR_GAMMA_INT"}. Before this PR: returned {}.

PR Process

Number of LGTM-ers (required): 2

Note

Driven by me, executed by OpenClaw.

first_run.sh used 'cd $(dirname $0)' then 'sudo ./install_udev_rules.sh', which could fail if sudo resets the working directory. install_udev_rules.sh set CURR_DIR but never used it, instead using 'cp ./99-obsensor-libusb.rules' which fails when cwd isn't the script's directory. Both scripts now resolve and use absolute paths.

…kaging Switches meta.json build command from 'make build' to 'make module.tar.gz' which uses Conan's deploy() method for tarball creation. Removes the manual staging/packaging from bin/build.sh since it's now redundant.

hexbabe · 2026-03-04T09:00:52Z

first_run.sh

 OS=$(uname)

 if [[ "$OS" == 'Linux' ]]; then
-    cd $(dirname $0)


this PR is stacked on top of #67

seanavery · 2026-03-04T17:37:51Z

src/module/orbbec.cpp

                } else if (key == "call_get_properties") {
                    call_get_properties = true;
+                } else {
+                    VIAM_RESOURCE_LOG(error) << "[do_command] unknown command: " << key;


Should we also check for empty command at the top to avoid unhelpful log here in that case?

this is a nit, not blocking

great call done

oliviamiller · 2026-03-05T15:59:44Z

These changes look great but it doesn't appear to address the issue described in the ticket you linked, which is that if you send a set_device_property property such as:

  {
   "set_device_property":  {
    "OB_PROP_COLOR_GAMMA_INT ": 255
   }
}

you don't get notified that the property is not supported by the astra2.

seanavery · 2026-03-06T18:47:14Z

src/module/orbbec.cpp

                } else if (key == "call_get_properties") {
                    call_get_properties = true;
+                } else {
+                    VIAM_RESOURCE_LOG(error) << "[do_command] unknown command: " << key;


this is a nit, not blocking

…setDeviceProperties Previously, when a user sent a property name that didn't match any supported property on the device, both setDeviceProperty and setDeviceProperties would silently return empty {} with no error. Fix setDeviceProperty to return an error dict when the property name is not found in the device's supported properties. Fix setDeviceProperties to check, after iterating supported properties, whether any user-supplied property names weren't found, and return an error for them.

hexbabe · 2026-03-09T17:21:44Z

These changes look great but it doesn't appear to address the issue described in the ticket you linked, which is that if you send a set_device_property property such as:
  {
   "set_device_property":  {
    "OB_PROP_COLOR_GAMMA_INT ": 255
   }
}
you don't get notified that the property is not supported by the astra2.

great callout, added code and tested (updated desc)

oliviamiller · 2026-03-09T17:28:42Z

src/module/device_control.hpp

+        if (seen_properties.count(name) == 0) {
+            return {{"error", "property not supported: " + name}};
+        }
+    }


this is not within the scope of your PR changes so feel free to ignore but this function has lots of repeated code from setDeviceProperty and should just call it.

oh like there's pre-existing code that should be DRY?

hexbabe added 2 commits March 1, 2026 21:43

RSDK-13471 — Fix local hot reload

6a9069f

chore: add all supported architectures to build section

58c4bd1

hexbabe force-pushed the RSDK-12844-do-command-error-responses branch 2 times, most recently from 4790f83 to 1193e1b Compare March 2, 2026 13:19

fix: package binary and udev rules into module tarball in build.sh

fceebd5

hexbabe force-pushed the RSDK-12844-do-command-error-responses branch 2 times, most recently from a7af649 to 62636be Compare March 2, 2026 13:53

hexbabe force-pushed the RSDK-12844-do-command-error-responses branch from 62636be to dbe28e5 Compare March 2, 2026 14:06

hexbabe added 2 commits March 2, 2026 17:49

RSDK-12844 — Return errors from do_command instead of empty struct

ea35697

hexbabe force-pushed the RSDK-12844-do-command-error-responses branch from dbe28e5 to ea35697 Compare March 2, 2026 22:49

hexbabe commented Mar 4, 2026

View reviewed changes

first_run.sh Outdated

OS=$(uname)

if [[ "$OS" == 'Linux' ]]; then

cd $(dirname $0)

Copy link

Collaborator Author

hexbabe Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR is stacked on top of #67

hexbabe requested review from SebastianMunozP, oliviamiller and seanavery March 4, 2026 09:13

seanavery reviewed Mar 4, 2026

View reviewed changes

Merge branch 'main' into RSDK-12844-do-command-error-responses

0a28dc3

seanavery approved these changes Mar 6, 2026

View reviewed changes

hexbabe added 3 commits March 9, 2026 13:01

return error for empty do_command

a37301e

simplify unsupported property check in setDeviceProperties

c0a70b4

oliviamiller approved these changes Mar 9, 2026

View reviewed changes

hexbabe merged commit f67fc7a into viam-modules:main Mar 10, 2026
5 checks passed

hexbabe deleted the RSDK-12844-do-command-error-responses branch March 10, 2026 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-12844] Add do command error responses#68

[RSDK-12844] Add do command error responses#68
hexbabe merged 10 commits intoviam-modules:mainfrom
hexbabe:RSDK-12844-do-command-error-responses

hexbabe commented Mar 2, 2026 •

edited

Loading

Uh oh!

hexbabe Mar 4, 2026

Uh oh!

seanavery Mar 4, 2026

Uh oh!

seanavery Mar 6, 2026

Uh oh!

hexbabe Mar 9, 2026

Uh oh!

oliviamiller commented Mar 5, 2026

Uh oh!

seanavery Mar 6, 2026

Uh oh!

hexbabe commented Mar 9, 2026

Uh oh!

oliviamiller Mar 9, 2026

Uh oh!

hexbabe Mar 9, 2026

Uh oh!

oliviamiller Mar 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hexbabe commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Manual Tests

Test 1 — Happy path (baseline)

Test 2 — Unknown command key (new else branch)

Test 3 — Device not connected (replaced throw with error return)

Test 4 — Outer catch returns error (injected exception)

Test 5 — Unsupported property name via set_device_property

Test 6 — Unsupported property name via set_device_properties (plural)

PR Process

Note

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviamiller commented Mar 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hexbabe commented Mar 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hexbabe commented Mar 2, 2026 •

edited

Loading

Test 5 — Unsupported property name via `set_device_property`

Test 6 — Unsupported property name via `set_device_properties` (plural)