Add ssh push command for cam_streamer and expose camera outputs#241
Add ssh push command for cam_streamer and expose camera outputs#241
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded SSH-based remote push and config-push commands to examples/camera_streamer/camera_streamer.sh, changed default config to config/single_camera.yaml, and exposed decoded camera outputs via TeleopCameraSubgraph.camera_output_names backed by _camera_output_names. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Workstation
participant Script as camera_streamer.sh
participant Robot as Remote Robot (SSH)
participant Build as Remote Build
participant Deploy as Remote Deploy/Container
User->>Script: run `push --ip <robot_ip> --user <user> [--skip-build] [--no-deploy]`
Script->>Script: parse_remote_opts (ip,user,config,flags)
Script->>Robot: create REMOTE_BUILD_DIR & rsync source/config
Robot-->>Script: ack source/config received
alt skip-build not set
Script->>Robot: trigger remote build in REMOTE_BUILD_DIR
Robot->>Build: build artifacts
Build-->>Robot: build complete
Robot-->>Script: build artifacts ready
end
alt no-deploy not set
Script->>Robot: restart/start sender container with config & receiver-host
Robot->>Deploy: start container
Deploy-->>Robot: container running
Robot-->>Script: deploy complete
end
Script-->>User: operation finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/camera_streamer/camera_streamer.sh`:
- Around line 20-21: DEFAULT_ROBOT_USER is set to "nvidia" and REMOTE_BUILD_DIR
is "/tmp/isaac-teleop-camera-build", which causes cmd_deploy_sender() to mount
an ephemeral host path; change REMOTE_BUILD_DIR to a persistent directory (e.g.,
under the robot user's home using DEFAULT_ROBOT_USER, such as
/home/${DEFAULT_ROBOT_USER}/isaac-teleop-camera-build or another durable path)
so the container's bind-mount source survives reboots and --restart
unless-stopped can recover the sender; update any usage of REMOTE_BUILD_DIR in
cmd_deploy_sender() and related deploy logic to reference the new persistent
path.
- Around line 231-251: The cmd_push_config flow can leave the container bound to
a different config path; update cmd_push_config to SSH into the remote and
inspect the sender container's mounts (use docker inspect on
SENDER_CONTAINER_NAME) and extract the container-side mounted file path, compare
that mount destination with the requested $CONFIG under $REMOTE_DIR; if they
differ, either refuse the push with an error message explaining the mismatch (do
not scp/restart) or perform a full redeploy of the sender (remove and recreate
the container using the requested mount) — implement this check after
parse_remote_opts and before scp/ssh restart so you either abort or redeploy
based on the mount comparison.
- Around line 105-114: In parse_remote_opts(), each case that dereferences $2
for options --ip, --user, --receiver-host, and --config must first validate that
a value exists and is not another option; follow the same guard used in
cmd_deploy_sender: check [[ $# -lt 2 || "$2" == -* ]] and call log_error with an
appropriate message before shifting and assigning the variable. Update the cases
for _REMOTE_IP, _REMOTE_USER, RECEIVER_HOST, and CONFIG to perform this
validation and only then set the variable and shift 2; leave the boolean flags
(--skip-build, --no-deploy) unchanged.
In `@examples/camera_streamer/teleop_camera_subgraph.py`:
- Around line 304-312: camera_outputs currently returns monitor paths (from
monitored_outputs/VideoStreamMonitorOp.frame_out) which causes downstream
consumers to get substituted placeholder frames; instead expose the real
pre-monitor frame sources via a separate mapping (e.g. _source_outputs) and have
camera_outputs return that. Add and populate _source_outputs in the compose
helpers: in _compose_local_sources set source_outputs[display_key] = (src_op,
src_port) and in _compose_rtp_sources set source_outputs[display_key] =
(decoder, "frame"); keep monitored_outputs as-is for visualization but ensure
camera_outputs (the `@property` camera_outputs) returns self._source_outputs (or
the new mapping) so downstream data-collection gets the decoder/source frames
rather than VideoStreamMonitorOp.frame_out.
- Around line 304-312: The camera_outputs property currently returns the live
backing dict _camera_outputs which allows external mutation; change
camera_outputs to return a shallow copy or an immutable view instead (e.g.,
dict(self._camera_outputs) or types.MappingProxyType(self._camera_outputs)) so
callers cannot modify internal state after compose() has run; update the
property implementation in the camera_outputs getter to return the
copied/readonly mapping while still documenting that the mapping is available
after compose().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 660a01b8-dee3-432e-9fd9-a8dc867faa4d
📒 Files selected for processing (2)
examples/camera_streamer/camera_streamer.shexamples/camera_streamer/teleop_camera_subgraph.py
Adds
pushandpush-configfor rtp camera streamingExposes camera output in subgraph for data collection purposes
Summary by CodeRabbit
New Features
Documentation