Skip to content

Bump viam-cpp-sdk to 0.38.1 and drop Reconfigurable - #80

Merged
Nicolas Palpacuer (NickPPC) merged 4 commits into
mainfrom
migrate/remove-reconfigurable
Jul 29, 2026
Merged

Bump viam-cpp-sdk to 0.38.1 and drop Reconfigurable#80
Nicolas Palpacuer (NickPPC) merged 4 commits into
mainfrom
migrate/remove-reconfigurable

Conversation

@NickPPC

Copy link
Copy Markdown
Contributor

Summary

Supersedes #77. That PR bumps viam-cpp-sdk 0.20.1 -> 0.38.1 but does not build, because the SDK removed viam::sdk::Reconfigurable in v0.35.0 (viamrobotics/viam-cpp-sdk#630):

src/module/orbbec.hpp:5:10: fatal error: viam/sdk/resource/reconfigurable.hpp: No such file or directory

This branches off #77's commit and adds the migration on top, so it carries the bump and actually compiles. I opened it against main rather than pushing onto chore/upgrade-viam-deps because that branch belongs to the viam-overwatch bot and would likely be force-updated out from under the work.

Changes

  • src/module/orbbec.hpp: Drop public viam::sdk::Reconfigurable and the reconfigure declaration; drop the include; document why the constructor/destructor now carry the load.
  • src/module/orbbec.cpp: Drop the include and the ~70-line Orbbec::reconfigure definition; move two cleanups into ~Orbbec().
  • src/module/discovery.hpp: Drop the include — it was already unused, nothing there implemented the interface.

Why deleting reconfigure is safe

Reconfigurable's own deprecation notice states the new contract:

"Reconfigure is no longer part of the Resource interface. You can still implement it if you like, but viam-server will rebuild resources on configuration change rather than calling a reconfigure method."

Nothing can call our reconfigure any more — reconfigure appears in zero files in v0.38.1.

The rebuild ordering is destroy-then-construct, so our destructor runs before the replacement constructor. ModuleService::ReconfigureResource -> ResourceManager::replace_one:

do_remove(name);                    // old destructed first
do_add(name, create_resource());    // then new constructed

with a deliberate scope above it dropping the shared_ptr refcount so the destructor really does run first. That matters for us because stopDevice keys off the serial number and stops a pipeline shared via process-global maps — construct-first ordering would have had the dying instance stop the new instance's pipeline.

The constructor already redoes everything reconfigure did: configure, configureDevice, startDevice, and the serial_by_resource mapping. stopDevice deliberately leaves the devices_by_serial entry in place (stopped, not erased), so the constructor still finds the device.

The part that isn't mechanical

Two cleanups existed only in reconfigure, so deleting it drops them silently. Both moved into ~Orbbec():

cleanup consequence of losing it
config_by_serial().erase(...) stale entry leaks when the serial_number attribute changes
frame_set_by_serial().erase(...) rebuilt instance finds a frame from the previous config

The second is a regression risk, not hygiene. It was added by 77d353c"RSDK-11302 bug fix reconfigure causes no recent frame error" — specifically to prevent that error. Left out, a stale frameset surviving the rebuild trips the 1-second staleness guard:

throw std::runtime_error("no recent color frame: check connection, diff: ...");

which is exactly the reported bug. The window is short (~33ms at 30fps) and the guard rejects rather than serves the stale frame, so it is a misleading error rather than bad data — but it would have quietly reopened a fixed ticket. The erase runs after stopDevice, so the pipeline is already stopped and no further frames can land after it.

Testing

  • All 10 SDK headers still included by src/ verified present in v0.38.1 — no further missing-header breaks behind the one that was masking everything.
  • No references to Reconfigurable / reconfigure remain in src/.
  • Line lengths within the 140 ColumnLimit.
  • Not compiled locally — building needs the full SDK build, so CI is the first real check. A fatal error on a missing include stops the translation unit, so API changes across 18 releases may still be hiding behind it; I would not be surprised by a second round.

For reviewers

  • tests/astra2_test.go has a "Reconfigure camera" subtest calling BuiltInReconfigure. I left it alone: it already t.Skips when the interface is absent and continues on error, and it is hardware-in-the-loop so I could not run it. Worth deciding whether it should now assert the rebuild path or be dropped.
  • Reasonable alternative: retarget this at chore/upgrade-viam-deps so chore(deps): bump Viam SDKs #77 stays the merge vehicle. I avoided that only because of the bot force-push risk.
  • Unrelated pre-existing wart noticed nearby: conanfile.py defines validate() twice, identically.

Claude Code Prompts Used

  • "look at the reconfigurable.hpp migration"
  • "yes proceed with the migration on chore/upgrade-viam-deps"

🤖 Generated with Claude Code

viam-cpp-sdk removed viam::sdk::Reconfigurable in v0.35.0
(viamrobotics/viam-cpp-sdk#630). viam-server now rebuilds a resource on a
config change instead of calling reconfigure, so the interface, the override,
and the three includes of reconfigurable.hpp all go away. The include in
discovery.hpp was already unused.

The ordering is safe: ModuleService::ReconfigureResource calls
ResourceManager::replace_one, which runs do_remove(name) before
do_add(name, create_resource()), so the old instance is destructed before the
replacement is constructed. The constructor already redoes everything
reconfigure did -- configure, configureDevice, startDevice, and the
serial_by_resource mapping.

Two cleanups only reconfigure performed do not survive that on their own, so
move them into ~Orbbec():

  config_by_serial().erase(...)     otherwise a stale entry leaks whenever the
                                    serial_number attribute changes
  frame_set_by_serial().erase(...)  otherwise the rebuilt instance can find a
                                    frame from the previous configuration and
                                    report "no recent frame: check connection"

The second is a regression risk rather than hygiene: it was added by 77d353c
("RSDK-11302 bug fix reconfigure causes no recent frame error") precisely to
stop that error, so deleting reconfigure without moving it would reopen that
bug. The erase runs after stopDevice so the pipeline is already stopped and no
further frames can land.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more interface changes surfaced once the reconfigurable.hpp fatal include
error stopped masking them.

get_image is gone from the Camera interface, and CameraServer no longer serves
a GetImage RPC at all -- its handlers are DoCommand, GetImages, GetPointCloud,
GetGeometries, GetProperties and GetStatus. Nothing could reach our
implementation any more and nothing called it internally, so remove it rather
than keep it as an unreachable method. get_images already covers the color
stream and has since #45.

get_status is a new pure virtual on both Camera and Discovery, which is what
made Orbbec and OrbbecDiscovery abstract:

    error: invalid new-expression of abstract class type 'orbbec::Orbbec'
    error: invalid new-expression of abstract class type 'discovery::OrbbecDiscovery'

The SDK prescribes no schema -- its own mocks return an arbitrary struct -- so
Orbbec reports the identifying state it already tracks: serial number, detected
model, firmware version, and whether the pipeline is streaming. OrbbecDiscovery
has no state of its own and returns an empty struct rather than enumerating the
bus on every status poll.

Also corrects a log label in get_point_cloud that read "[get_image]", which now
names a function that no longer exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
viam-cpp-sdk removed Camera::get_image and its CameraServer no longer serves a
GetImage RPC, so DecodeImageFromCamera has nothing to reach on this module. Ask
Images() for just the color source instead, which keeps the test's intent --
retrieve one image and check it is the colour stream -- and additionally covers
filter_source_names, which nothing exercised before.

Source names are "color" and "depth", as documented in the README.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/module/orbbec.cpp
@NickPPC
Nicolas Palpacuer (NickPPC) merged commit e772695 into main Jul 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants