-
Notifications
You must be signed in to change notification settings - Fork 5
[RSDK-12362] Provide Support for Orbbec Gemini 335LE Camera #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RSDK-12362] Provide Support for Orbbec Gemini 335LE Camera #58
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for the Orbbec Gemini 335LE camera model in addition to the existing Astra 2, implementing a model-based configuration system to handle multiple camera types. The changes include refactoring model-specific configurations, adding firmware update support, Windows registry setup utilities, and improving device discovery for both USB and Ethernet-connected cameras.
- Introduced model configuration system with separate configs for Astra 2 and Gemini 335LE
- Added firmware update functionality and Windows registry setup as separate modules
- Enhanced device discovery to support network-connected cameras
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/module/orbbec_windows_registry.hpp | Header for Windows registry setup utilities |
| src/module/orbbec_windows_registry.cpp | Implementation of Windows device registry configuration |
| src/module/orbbec_firmware.hpp | Header declaring firmware update functionality |
| src/module/orbbec_firmware.cpp | Implementation of firmware download and update process |
| src/module/orbbec.hpp | Added model configurations, multiple model support, and updated method signatures |
| src/module/orbbec.cpp | Refactored to use model configs, added validation helpers, improved error handling |
| src/module/main.cpp | Registered both Astra2 and Gemini335Le models |
| src/module/discovery.cpp | Enhanced discovery to support network devices and multiple models |
| CMakeLists.txt | Added new firmware and Windows registry source files to build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…, preventing reutilizing a camera by multiple resources by the if my_dev->started in startDevice
…ons are not supported instead of silently picking other ones
…until it has been tested, removing support for RGB color stream on Gemini as it didnt work with a 640X480 resolution -> need to investigate which are actually supported, removed raw pointers in favor of const references, adding supported and default formats to each model config
…n nowUs <= imageTimeUs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ut just moved to its own file
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
544917e to
767ce34
Compare
|
Hello @williamjhyland , @IanWhalen Please not that for the time being, we are supporting the following set of resolutions, and only MJPG/Y16 as formats for color/depth streams respectively, is this OK for the customer? |
|
Checking regarding customer commentary. |
| deviceInfoString << "Device " << (i + 1) << " - Name: " << deviceName << ", Serial: " << serialNumber | ||
| << ", Connection: " << connectionType; | ||
| if (!ipAddress.empty()) { | ||
| deviceInfoString << ", IP: " << ipAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ethernet cameras does the IP address need to be in the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the SDK takes care of that, it has its own "discovery service", it detects all Orbbec cameras reachable therough USB or the network
src/module/discovery.cpp
Outdated
|
|
||
| VIAM_RESOURCE_LOG(info) << "Successfully configured device " << (i + 1) << " with serial: " << serialNumber; | ||
| } catch (ob::Error& deviceError) { | ||
| VIAM_RESOURCE_LOG(warn) << "Failed to get device info for device " << (i + 1) << ": " << deviceError.what(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be an error log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, raised to error
src/module/discovery.cpp
Outdated
| VIAM_RESOURCE_LOG(warn) << "Discovery failed - check network connectivity for Ethernet cameras or USB connection for USB cameras"; | ||
| } catch (...) { | ||
| VIAM_RESOURCE_LOG(error) << "Failed to discover Orbbec devices: unknown error"; | ||
| VIAM_RESOURCE_LOG(warn) << "Discovery failed - check network connectivity for Ethernet cameras or USB connection for USB cameras"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with these, they feel more like errors than warnings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, raised to error
src/module/orbbec.cpp
Outdated
| const OrbbecModelConfig& OrbbecModelConfig::forDevice(const std::string& device_name) { | ||
| if (device_name.find("Astra2") != std::string::npos || device_name.find("Astra 2") != std::string::npos) | ||
| return ASTRA2_CONFIG; | ||
| if (device_name.find("Gemini 335Le") != std::string::npos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible the device name could be "Gemini335Le" like the Astra2 check above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the exact names that I get from the Device for simplicity.
Just bear in mind that for unexplicable reasons, Astra2 can return to different names depending on the way you query, either "Orbbec Astra2" or "Orbbec Astra 2", hence I added support for multiple names :/
src/module/orbbec.cpp
Outdated
| throw std::runtime_error("Unsupported Orbbec camera model: " + device_name); | ||
| } | ||
|
|
||
| bool OrbbecModelConfig::isSupported(const std::string& device_name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we combine this with the function above since they pretty much do the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| std::string const serial = attrs["serial_number"].get_unchecked<std::string>(); | ||
| if (serial.empty()) { | ||
| throw std::invalid_argument("serial_number must be a non-empty string"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could me make a helper for these above checks since the code is repeated in the validateGemini function ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is it possible to combine into only one validate function? Would be harder to mantain a validate function for every model if we add more in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done
| buffer << "Device firmware already on version " << model_config_->min_firmware_version; | ||
| resp.emplace(firmware_key, buffer.str()); | ||
| break; | ||
| VIAM_RESOURCE_LOG(info) << buffer.str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we log the buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no harm in adding this to the logs as well, but we may want it to investigate potential issues, specially with customers. This log is not repetitive, so wont be intrusive.
| } else { | ||
| VIAM_SDK_LOG(warn) << "VIAM_MODULE_DATA is set to " << viam_module_data | ||
| << " but is not a valid directory, using current working directory to store PCD file"; | ||
| VIAM_RESOURCE_LOG(warn) << "VIAM_MODULE_DATA is set to " << viam_module_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you run into this env var not having a valid directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, the only change I'm doing in this part is to use VIAM_RESOURCE_LOG instead of VIAM_SDK_LOG for better clarity in the logs
src/module/orbbec.cpp
Outdated
| VIAM_SDK_LOG(error) << "Current device does not support hardware depth-to-color " | ||
| "alignment."; | ||
| return; | ||
| VIAM_SDK_LOG(warn) << "Device " << serialNumber << " does not support hardware depth-to-color alignment, trying software alignment"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we do this both in registerDevice and startDevice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was present before, which raises a really good point, I restructured this logic as follows:
registerDevice -> Only creates an unconfigured device, containing only its serialNumber and an empty ViamOBDevice.
configureDevice -> New function, takes care of actually configuring the device, i.e. taking the specs from the user and making all adjustments to the device to comply with them
startDevice -> Simply starts the device, nothing more
README.md
Outdated
| ## Model viam:orbbec:gemini_335le | ||
|
|
||
| ### Attributes | ||
| Use [Orbbec cameras](https://www.orbbec.com/gemini-335le/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong label for link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/module/discovery.cpp
Outdated
| try { | ||
| const auto& modelConfig = orbbec::OrbbecModelConfig::forDevice(deviceName); | ||
| viamModelSuffix = modelConfig.viam_model_suffix; | ||
| } catch (const std::runtime_error& e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're sure all runtime errors = camera unsupported? any more specific error to catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| int requiredMajor = 0, requiredMinor = 0, requiredPatch = 0; | ||
|
|
||
| // ignore any trailing text in the case of a beta or RC version | ||
| sscanf(version.c_str(), "%d.%d.%d*s", &major, &minor, &patch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is *s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means to discard the string after that last integer, in particular
"*" -> means discard
"s" -> means string
put together is to discard the trailing string.
|
|
||
| namespace orbbec { | ||
|
|
||
| namespace windows_registry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled this camera for all OSs other than Linux for now, I tested on Mac and the camera could not be reached.
src/module/orbbec_firmware.hpp
Outdated
| viam::sdk::LogSource& logger); | ||
|
|
||
| } // namespace firmware | ||
| } // namespace orbbec No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] newline eof missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
… std::optional when appropriate
… can be used to detect Orbbec cameras directly
…new function configureDevice and is the only place to handle the device configuration, as the resolutions config logic was duplicated
…README per feedback, fixing some nits
…odule.tar.gz rule
Providing support for Gemini 335LE cameras, until now, the Orbbec module was designed to exclusively support Astra2 cameras, so this PR involves a redesign of the Orbbec module to support for different Orbbec model cameras, adding support at this instance to Gemini 335LE.
Per documentation, the camera only supports MJPG color stream format natively, it supports RGB variants through the SDK. I'm disabling that option for now until thoroughly tested as well, so only supporting MJPG at this time.
Testing Performed: