Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/module/device_control.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ viam::sdk::ProtoStruct setDeviceProperty(std::shared_ptr<DeviceT> device,
}
}

return {};
return {{"error", "property not supported: " + property_map.begin()->first}};
}

// Get property list
Expand Down Expand Up @@ -253,11 +253,13 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr<DeviceT> device,
return {{"error", "properties must be a struct"}};
}
std::unordered_set<std::string> writable_properties;
std::unordered_set<std::string> seen_properties;
auto const& properties_map = properties.template get_unchecked<viam::sdk::ProtoStruct>();
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) {
seen_properties.insert(property_item.name);
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.";
Expand Down Expand Up @@ -299,6 +301,11 @@ viam::sdk::ProtoStruct setDeviceProperties(std::shared_ptr<DeviceT> device,
}
}
}
for (auto const& [name, _] : properties_map) {
if (seen_properties.count(name) == 0) {
return {{"error", "property not supported: " + name}};
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes

return getDeviceProperties(device, command, writable_properties);
}

Expand Down
10 changes: 9 additions & 1 deletion src/module/orbbec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1471,6 +1471,9 @@ vsdk::Camera::image_collection Orbbec::get_images(std::vector<std::string> filte
}

vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) {
if (command.empty()) {
return vsdk::ProtoStruct{{"error", std::string("empty command")}};
}
bool call_get_properties = false;
try {
constexpr char firmware_key[] = "update_firmware";
Expand All @@ -1484,7 +1487,8 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) {
const std::lock_guard<std::mutex> lock(devices_by_serial_mu());
auto search = devices_by_serial().find(serial_number);
if (search == devices_by_serial().end()) {
throw std::invalid_argument("device is not connected");
VIAM_RESOURCE_LOG(error) << "[do_command] device is not connected";
return vsdk::ProtoStruct{{"error", std::string("device is not connected")}};
}

std::unique_ptr<ViamOBDevice>& dev = search->second;
Expand Down Expand Up @@ -1573,6 +1577,9 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) {
return device_control::createModuleConfig<ViamOBDevice, ob::VideoStreamProfile>(dev);
} else if (key == "call_get_properties") {
call_get_properties = true;
} else {
VIAM_RESOURCE_LOG(error) << "[do_command] unknown command: " << key;

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

this is a nit, not blocking

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great call done

return vsdk::ProtoStruct{{"error", "unknown command: " + key}};
}
}
} // unlock devices_by_serial_mu_
Expand Down Expand Up @@ -1601,6 +1608,7 @@ vsdk::ProtoStruct Orbbec::do_command(const vsdk::ProtoStruct& command) {
}
} catch (const std::exception& e) {
VIAM_RESOURCE_LOG(error) << service_name << ": exception caught: " << e.what();
return vsdk::ProtoStruct{{"error", std::string(e.what())}};
}
return vsdk::ProtoStruct{};
}
Expand Down