RSDK-11439 — Avoid hardcoding fields for protos strings (things like types response.source_name and response. mime_type)#66
Merged
hexbabe merged 10 commits intoviam-modules:mainfrom Mar 5, 2026
Conversation
Move inline constants from orbbec.cpp into a dedicated constants header file (src/module/constants.hpp) under the orbbec:: namespace. Fixes RSDK-11439
Add kAttrSerialNumber, kAttrSensors, kAttrWidth, kAttrHeight, kAttrFormat to constants.hpp and use them throughout orbbec.cpp and discovery.cpp instead of repeating raw string literals.
New attribute key constants (kAttrSerialNumber, kAttrSensors, kAttrWidth, kAttrHeight, kAttrFormat) declared in the existing CONSTANTS block in orbbec.cpp alongside the original constants. constants.hpp removed.
hexbabe
requested review from
SebastianMunozP,
oliviamiller and
seanavery
and removed request for
oliviamiller
seanavery
reviewed
Mar 2, 2026
seanavery
left a comment
seanavery
left a comment
There was a problem hiding this comment.
this PR mentions discovery.cpp but only diff is in orbec.cpp.. are we missing some const replacements there?
src/module/orbbec.cpp
Outdated
| const uint64_t maxFrameSetTimeDiffUs = | ||
| 2000; // max time difference between frames in a frameset to be considered simultaneous, in microseconds (equal to 2 ms) | ||
|
|
||
| const std::string kAttrSerialNumber = "serial_number"; |
There was a problem hiding this comment.
Should these be in the header orbbec.hpp?
Collaborator
Author
There was a problem hiding this comment.
yes, they should so they can be shared across other TUs. Done
src/module/orbbec.cpp
Outdated
| const std::string kAttrWidth = "width"; | ||
| const std::string kAttrHeight = "height"; | ||
| const std::string kAttrFormat = "format"; | ||
| // CONSTANTS END |
seanavery
reviewed
Mar 2, 2026
There was a problem hiding this comment.
Do we also need to use the new consts in device_control.hpp?
hexbabe
force-pushed
the
fix/no-hardcoded-consts
branch
from
44fbd36 to
14ff73e
Compare
Moves kAttrSerialNumber, kAttrSensors, kAttrWidth, kAttrHeight, kAttrFormat to orbbec.hpp as inline const so they can be shared across translation units. Updates discovery.cpp to use kAttrSerialNumber instead of hardcoded string.
hexbabe
force-pushed
the
fix/no-hardcoded-consts
branch
from
14ff73e to
34db767
Compare
Collaborator
Author
@seanavery thanks for the thorough review. De-consted more files, and manually re-tested. |
Replaces hardcoded "serial_number", "sensors", "width", "height", "format" strings in device_control.hpp with orbbec::kAttr* constants from orbbec.hpp.
hexbabe
force-pushed
the
fix/no-hardcoded-consts
branch
from
290de7a to
c4b0c96
Compare
seanavery
approved these changes
Mar 4, 2026
src/module/device_control.hpp
Outdated
| depth_sensor[orbbec::kAttrHeight] = static_cast<int>(sp->template as<VideoStreamProfileT>()->getHeight()); | ||
| depth_sensor[orbbec::kAttrFormat] = | ||
| ob::TypeHelper::convertOBFormatTypeToString(sp->template as<VideoStreamProfileT>()->getFormat()); | ||
| sensors["depth"] = depth_sensor; |
src/module/device_control.hpp
Outdated
| color_sensor[orbbec::kAttrHeight] = static_cast<int>(sp->template as<VideoStreamProfileT>()->getHeight()); | ||
| color_sensor[orbbec::kAttrFormat] = | ||
| ob::TypeHelper::convertOBFormatTypeToString(sp->template as<VideoStreamProfileT>()->getFormat()); | ||
| sensors["color"] = color_sensor; |
Collaborator
Author
There was a problem hiding this comment.
done. moved "color" and "depth" to hpp so they can be shared too
Address review feedback: use constants instead of raw string literals for sensor names in device_control.hpp. Moved kColorSourceName and kDepthSourceName from orbbec.cpp to orbbec.hpp so they can be shared across translation units.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RSDK-11439
Summary
RSDK-11439
Replaces hardcoded attribute key strings (
"serial_number","sensors","width","height","format") with named inline const constants defined inorbbec.hpp.Constants are shared across translation units via the existing
orbbec.hppheader — no new files. Updated usages in:orbbec.cpp— config parsing (Reconfigure, discovery config generation)device_control.hpp— device info, camera params, module config creationdiscovery.cpp— resource discovery attribute populationManual Tests
Tested on
linux/amd64(Framework Laptop) using viam module reload-local:viam:orbbec:discovery— successfully discovered Astra2 serial AARY14100X5viam:orbbec:astra2with discovered serial —get_images()returned color (1280×720 JPEG ✅) and depth (1.8MB ✅)PR Process
Number of LGTM-ers (required): 1
Note
This is my first PR done using OpenClaw running on my Framework laptop. It handled implementation as well as manual testing via viam-cli and browser tooling by signing into and using the Viam Web App.