From d3f2c15e9d499ab88574f5407d980c5ee5c80294 Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Thu, 15 Jan 2026 18:29:13 -0300 Subject: [PATCH 1/7] Fix get device properties --- src/module/device_control.hpp | 1 - src/module/orbbec.cpp | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index 9be6465..34ec616 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -297,7 +297,6 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, } } } - return getDeviceProperties(device, command, writable_properties); } template diff --git a/src/module/orbbec.cpp b/src/module/orbbec.cpp index 06086be..b0dcb2b 100644 --- a/src/module/orbbec.cpp +++ b/src/module/orbbec.cpp @@ -1454,7 +1454,8 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) { } else if (key == "get_device_properties") { return device_control::getDeviceProperties(dev->device, key); } else if (key == "set_device_properties") { - return device_control::setDeviceProperties(dev->device, value, key); + device_control::setDeviceProperties(dev->device, value, key); + call_get_properties = true; } else if (key == "get_device_property") { return device_control::getDeviceProperty(dev->device, value, key); } else if (key == "set_device_property") { From 7b3b93da6d3f5c9846cf3495c44740e529df3528 Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Thu, 15 Jan 2026 18:29:13 -0300 Subject: [PATCH 2/7] Fix get device properties --- src/module/device_control.hpp | 1 - src/module/orbbec.cpp | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index 9be6465..34ec616 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -297,7 +297,6 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, } } } - return getDeviceProperties(device, command, writable_properties); } template diff --git a/src/module/orbbec.cpp b/src/module/orbbec.cpp index f93fdd6..5418f27 100644 --- a/src/module/orbbec.cpp +++ b/src/module/orbbec.cpp @@ -1485,7 +1485,8 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) { } else if (key == "get_device_properties") { return device_control::getDeviceProperties(dev->device, key); } else if (key == "set_device_properties") { - return device_control::setDeviceProperties(dev->device, value, key); + device_control::setDeviceProperties(dev->device, value, key); + call_get_properties = true; } else if (key == "get_device_property") { return device_control::getDeviceProperty(dev->device, value, key); } else if (key == "set_device_property") { From a7ccc49a6eed9c174aa9823ab8fbb166679ed353 Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Tue, 3 Feb 2026 10:48:44 -0500 Subject: [PATCH 3/7] Fixing set_device_properties method --- src/module/device_control.hpp | 5 +++++ src/module/orbbec.cpp | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index 34ec616..73db12d 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -247,6 +247,9 @@ template viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, const viam::sdk::ProtoValue& properties, std::string const& command) { + if (device == nullptr) { + return {{"error", "device is null"}}; + } if (!properties.template is_a()) { return {{"error", "properties must be a struct"}}; } @@ -269,6 +272,7 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, std::stringstream error_ss; error_ss << "Value for property " << property_item.name << " is not a struct, skipping."; VIAM_SDK_LOG(warn) << error_ss.str(); + continue; } auto const& value = value_struct->at("current"); writable_properties.insert(property_item.name); @@ -297,6 +301,7 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, } } } + return getDeviceProperties(device, command, {}); } template diff --git a/src/module/orbbec.cpp b/src/module/orbbec.cpp index 5418f27..4a16534 100644 --- a/src/module/orbbec.cpp +++ b/src/module/orbbec.cpp @@ -1418,6 +1418,7 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) { std::unique_ptr& dev = search->second; for (auto const& [key, value] : command) { + VIAM_RESOURCE_LOG(info) << "Received command: " << key; if (key == firmware_key) { VIAM_RESOURCE_LOG(info) << "Received firmware update command"; vsdk::ProtoStruct resp = viam::sdk::ProtoStruct{}; @@ -1483,10 +1484,10 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) { } else if (key == "set_depth_unit") { return device_control::setDepthUnit(dev->device, value, key); } else if (key == "get_device_properties") { + VIAM_RESOURCE_LOG(info) << "Received get_device_properties command"; return device_control::getDeviceProperties(dev->device, key); } else if (key == "set_device_properties") { - device_control::setDeviceProperties(dev->device, value, key); - call_get_properties = true; + return device_control::setDeviceProperties(dev->device, value, key); } else if (key == "get_device_property") { return device_control::getDeviceProperty(dev->device, value, key); } else if (key == "set_device_property") { From 1f0938f82517ddcaff99af92e75d7ae99004d27a Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Tue, 3 Feb 2026 10:54:39 -0500 Subject: [PATCH 4/7] Improving setDeviceProperties return --- src/module/device_control.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index 73db12d..f724fae 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -301,7 +301,7 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, } } } - return getDeviceProperties(device, command, {}); + return getDeviceProperties(device, command, writable_properties); } template From 279e81216fe1a69b577f44d0b44264431bc4e8fd Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Tue, 3 Feb 2026 10:57:45 -0500 Subject: [PATCH 5/7] Improving error returns on setDeviceProperties --- src/module/device_control.hpp | 8 ++++++-- src/module/orbbec.cpp | 2 -- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index f724fae..076bae9 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -248,10 +248,14 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, const viam::sdk::ProtoValue& properties, std::string const& command) { if (device == nullptr) { - return {{"error", "device is null"}}; + std::stringstream ss; + ss << "calling " << command << " on null device"; + return {{"error", ss.str()}}; } if (!properties.template is_a()) { - return {{"error", "properties must be a struct"}}; + std::stringstream ss; + ss << "calling " << command << " on non-struct properties"; + return {{"error", ss.str()}}; } std::unordered_set writable_properties; auto const& properties_map = properties.template get_unchecked(); diff --git a/src/module/orbbec.cpp b/src/module/orbbec.cpp index 4a16534..f93fdd6 100644 --- a/src/module/orbbec.cpp +++ b/src/module/orbbec.cpp @@ -1418,7 +1418,6 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) { std::unique_ptr& dev = search->second; for (auto const& [key, value] : command) { - VIAM_RESOURCE_LOG(info) << "Received command: " << key; if (key == firmware_key) { VIAM_RESOURCE_LOG(info) << "Received firmware update command"; vsdk::ProtoStruct resp = viam::sdk::ProtoStruct{}; @@ -1484,7 +1483,6 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) { } else if (key == "set_depth_unit") { return device_control::setDepthUnit(dev->device, value, key); } else if (key == "get_device_properties") { - VIAM_RESOURCE_LOG(info) << "Received get_device_properties command"; return device_control::getDeviceProperties(dev->device, key); } else if (key == "set_device_properties") { return device_control::setDeviceProperties(dev->device, value, key); From 8444c16263ee0dce9ed3ba01bfd33f4c1cb72307 Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Tue, 3 Feb 2026 16:28:06 -0500 Subject: [PATCH 6/7] Removing dead-code --- src/module/device_control.hpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index 076bae9..6cd16f8 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -265,10 +265,9 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, if (properties_map.count(property_item.name) > 0) { if (property_item.permission == OB_PERMISSION_DENY || property_item.permission == OB_PERMISSION_READ) { std::stringstream error_ss; - error_ss << "Property " << property_item.name << " is not writable, skipping."; + error_ss << "Property " << property_item.name << " is not writable."; VIAM_SDK_LOG(warn) << error_ss.str(); return {{"error", error_ss.str()}}; - continue; } try { auto const& value_struct = properties_map.at(property_item.name).get(); From 77de3d5bffacfb043c0566c8b34f90ac509f6d47 Mon Sep 17 00:00:00 2001 From: Sebastian Munoz Date: Tue, 3 Feb 2026 16:44:28 -0500 Subject: [PATCH 7/7] Enforcing a all or nothing strategy for set_properties --- src/module/device_control.hpp | 158 ++++++++++++++++++++++++++-------- 1 file changed, 124 insertions(+), 34 deletions(-) diff --git a/src/module/device_control.hpp b/src/module/device_control.hpp index 6cd16f8..8da6bdd 100644 --- a/src/module/device_control.hpp +++ b/src/module/device_control.hpp @@ -1,4 +1,5 @@ #pragma once +#include #include namespace device_control { @@ -243,6 +244,15 @@ viam::sdk::ProtoStruct getDeviceProperties(std::shared_ptr device, return properties; } +// Helper struct to hold validated property information for two-phase set +struct ValidatedProperty { + OBPropertyItem item; + int int_value; + double float_value; + bool bool_value; + enum class Type { INT, FLOAT, BOOL } value_type; +}; + template viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, const viam::sdk::ProtoValue& properties, @@ -257,53 +267,133 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr device, ss << "calling " << command << " on non-struct properties"; return {{"error", ss.str()}}; } - std::unordered_set writable_properties; + auto const& properties_map = properties.template get_unchecked(); + + // Build a map of device properties for quick lookup + std::unordered_map device_properties; int const supportedPropertyCount = device->getSupportedPropertyCount(); for (int i = 0; i < supportedPropertyCount; i++) { OBPropertyItem property_item = device->getSupportedProperty(i); - if (properties_map.count(property_item.name) > 0) { - if (property_item.permission == OB_PERMISSION_DENY || property_item.permission == OB_PERMISSION_READ) { + device_properties[property_item.name] = property_item; + } + + // Phase 1: Validate all properties before setting any + std::vector validated_properties; + std::unordered_set writable_properties; + + for (auto const& [prop_name, prop_value] : properties_map) { + // Check if property exists on the device + auto it = device_properties.find(prop_name); + if (it == device_properties.end()) { + std::stringstream error_ss; + error_ss << "Property '" << prop_name << "' is not supported by this device."; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; + } + + OBPropertyItem const& property_item = it->second; + + // Check if the property is writable + if (property_item.permission == OB_PERMISSION_DENY || property_item.permission == OB_PERMISSION_READ) { + std::stringstream error_ss; + error_ss << "Property '" << prop_name << "' is not writable (permission: " << permissionToString(property_item.permission) + << ")."; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; + } + + // Validate value structure + auto const value_struct_opt = prop_value.template get(); + if (!value_struct_opt) { + std::stringstream error_ss; + error_ss << "Value for property '" << prop_name << "' must be a struct with a 'current' field."; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; + } + + auto const& value_struct = *value_struct_opt; + if (value_struct.count("current") == 0) { + std::stringstream error_ss; + error_ss << "Value for property '" << prop_name << "' is missing required 'current' field."; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; + } + + auto const& value = value_struct.at("current"); + + // Validate type compatibility and extract value + ValidatedProperty validated; + validated.item = property_item; + + if (property_item.type == OB_INT_PROPERTY) { + if (!value.template is_a()) { std::stringstream error_ss; - error_ss << "Property " << property_item.name << " is not writable."; - VIAM_SDK_LOG(warn) << error_ss.str(); + error_ss << "Property '" << prop_name << "' expects numeric value for int property."; + VIAM_SDK_LOG(error) << error_ss.str(); return {{"error", error_ss.str()}}; } - try { - auto const& value_struct = properties_map.at(property_item.name).get(); - if (!value_struct) { - std::stringstream error_ss; - error_ss << "Value for property " << property_item.name << " is not a struct, skipping."; - VIAM_SDK_LOG(warn) << error_ss.str(); - continue; - } - auto const& value = value_struct->at("current"); - writable_properties.insert(property_item.name); - if (property_item.type == OB_INT_PROPERTY && value.is_a()) { - int int_value = static_cast(value.get_unchecked()); - device->setIntProperty(property_item.id, int_value); - VIAM_SDK_LOG(info) << "[setDeviceProperties] Set int property " << property_item.name << " to " << int_value; - } else if (property_item.type == OB_FLOAT_PROPERTY && value.is_a()) { - double float_value = value.get_unchecked(); - device->setFloatProperty(property_item.id, float_value); - VIAM_SDK_LOG(info) << "[setDeviceProperties] Set float property " << property_item.name << " to " << float_value; - } else if (property_item.type == OB_BOOL_PROPERTY && value.is_a()) { - bool bool_value = value.get_unchecked(); - device->setBoolProperty(property_item.id, bool_value); - VIAM_SDK_LOG(info) << "[setDeviceProperties] Set bool property " << property_item.name << " to " - << (bool_value ? "true" : "false"); - } else { - VIAM_SDK_LOG(warn) << "[setDeviceProperties] Type mismatch or unsupported type for property " << property_item.name - << ", skipping."; - } - } catch (ob::Error& e) { + validated.int_value = static_cast(value.template get_unchecked()); + validated.value_type = ValidatedProperty::Type::INT; + } else if (property_item.type == OB_FLOAT_PROPERTY) { + if (!value.template is_a()) { std::stringstream error_ss; - error_ss << "Failed to set property " << property_item.name << ": " << e.what(); + error_ss << "Property '" << prop_name << "' expects numeric value for float property."; VIAM_SDK_LOG(error) << error_ss.str(); return {{"error", error_ss.str()}}; } + validated.float_value = value.template get_unchecked(); + validated.value_type = ValidatedProperty::Type::FLOAT; + } else if (property_item.type == OB_BOOL_PROPERTY) { + if (!value.template is_a()) { + std::stringstream error_ss; + error_ss << "Property '" << prop_name << "' expects boolean value for bool property."; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; + } + validated.bool_value = value.template get_unchecked(); + validated.value_type = ValidatedProperty::Type::BOOL; + } else { + std::stringstream error_ss; + error_ss << "Property '" << prop_name << "' has unsupported type: " << propertyTypeToString(property_item.type) << "."; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; + } + + validated_properties.push_back(validated); + writable_properties.insert(prop_name); + } + + // Phase 2: Apply all validated properties + for (auto const& validated : validated_properties) { + try { + switch (validated.value_type) { + case ValidatedProperty::Type::INT: + device->setIntProperty(validated.item.id, validated.int_value); + VIAM_SDK_LOG(info) << "[setDeviceProperties] Set int property " << validated.item.name << " to " << validated.int_value; + break; + case ValidatedProperty::Type::FLOAT: + device->setFloatProperty(validated.item.id, validated.float_value); + VIAM_SDK_LOG(info) << "[setDeviceProperties] Set float property " << validated.item.name << " to " + << validated.float_value; + break; + case ValidatedProperty::Type::BOOL: + device->setBoolProperty(validated.item.id, validated.bool_value); + VIAM_SDK_LOG(info) << "[setDeviceProperties] Set bool property " << validated.item.name << " to " + << (validated.bool_value ? "true" : "false"); + break; + } + } catch (ob::Error& e) { + // Note: At this point we may have partially applied properties. + // This is unavoidable since the SDK doesn't support transactions. + std::stringstream error_ss; + error_ss << "Failed to set property '" << validated.item.name << "': " << e.what() + << " (some properties may have been partially applied)"; + VIAM_SDK_LOG(error) << error_ss.str(); + return {{"error", error_ss.str()}}; } } + return getDeviceProperties(device, command, writable_properties); }