Enable vehicle teleoperation#686
Open
jyue86 wants to merge 6 commits into
Open
Conversation
Contributor
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
Contributor
📝 WalkthroughWalkthroughThis PR introduces a full vehicle teleoperation feature. It adds two FlatBuffers schemas ( Sequence DiagramsequenceDiagram
participant SteeringWheelPlugin
participant OpenXRSession
participant IsaacRemoteSteeringWorker
participant VehicleControlRetargeter
participant ZMQ_PUB
participant KiaPandaWorker
participant PandaRunner
SteeringWheelPlugin->>OpenXRSession: push SteeringWheelOutput FlatBuffer at 90 Hz
IsaacRemoteSteeringWorker->>OpenXRSession: poll SteeringWheelTracker via DeviceIO session
OpenXRSession-->>IsaacRemoteSteeringWorker: SteeringWheelOutputTrackedT
IsaacRemoteSteeringWorker->>VehicleControlRetargeter: retarget(sample, sequence=N)
VehicleControlRetargeter-->>IsaacRemoteSteeringWorker: VehicleControlCommand(steer, accel, throttle, brake)
IsaacRemoteSteeringWorker->>ZMQ_PUB: JSON-encoded command at --rate-hz
ZMQ_PUB->>KiaPandaWorker: topic + JSON payload
KiaPandaWorker->>KiaPandaWorker: timeout check → neutral if stale
KiaPandaWorker->>PandaRunner: apply CarControl actuator values
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Justin Yue <jyue009@ucr.edu>
Signed-off-by: Justin Yue <jyue009@ucr.edu>
Signed-off-by: Justin Yue <jyue009@ucr.edu>
Signed-off-by: Justin Yue <jyue009@ucr.edu>
Signed-off-by: Justin Yue <jyue009@ucr.edu>
Signed-off-by: Justin Yue <jyue009@ucr.edu>
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for vehicle teleoperation by introducing a steering-wheel DeviceIO data path, a normalized vehicle control command schema, and an example that publishes commands over ZMQ to a Panda (DBW) worker.
Changes:
- Adds new FlatBuffer schemas + C++/Python bindings for
SteeringWheelOutputandVehicleControlCommand, including schema tests. - Introduces a Linux steering wheel native plugin plus live/replay tracker support and retargeting-engine source/tensor types.
- Adds a complete
examples/vehicle_teleopworkflow (remote steering worker, keyboard fallback, command logging/replay, Panda worker).
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Wires new example and plugin into top-level build options. |
examples/vehicle_teleop/.gitignore |
Ignores logs, third-party clones, and venv for the example. |
examples/vehicle_teleop/CMakeLists.txt |
Installs the vehicle teleop example (python + scripts + config). |
examples/vehicle_teleop/README.md |
Documents setup and remote/vehicle/replay workflows. |
examples/vehicle_teleop/config/steering_wheel_config.yaml |
Provides default axis mapping for a G920-style wheel. |
examples/vehicle_teleop/python/pyproject.toml |
Declares example Python dependencies (mcap/zmq/panda deps, etc.). |
examples/vehicle_teleop/python/vehicle_teleop/__init__.py |
Example package marker. |
examples/vehicle_teleop/python/vehicle_teleop/command_log.py |
MCAP JSON-schema logging + reader for command/sample records. |
examples/vehicle_teleop/python/vehicle_teleop/isaac_keyboard_control_worker.py |
Keyboard fallback publisher using the new vehicle retargeter. |
examples/vehicle_teleop/python/vehicle_teleop/isaac_remote_steering_worker.py |
Remote worker that spawns the native plugin, reads DeviceIO, retargets, publishes ZMQ, and optionally logs MCAP. |
examples/vehicle_teleop/python/vehicle_teleop/kia_panda_worker.py |
Vehicle-side ZMQ subscriber that writes commands to PandaRunner (or dry-run prints). |
examples/vehicle_teleop/python/vehicle_teleop/replay_command_mcap.py |
Replays MCAP logs and prints retargeted command values (optional realtime pacing). |
examples/vehicle_teleop/python/vehicle_teleop/vehicle_command.py |
Defines the example’s wire-format command dataclass and helpers. |
examples/vehicle_teleop/scripts/replay_command_mcap.sh |
Convenience launcher for MCAP replay. |
examples/vehicle_teleop/scripts/run_isaac_keyboard_control_worker.sh |
Convenience launcher for keyboard worker. |
examples/vehicle_teleop/scripts/run_isaac_remote_steering_worker.sh |
Convenience launcher for remote steering worker. |
examples/vehicle_teleop/scripts/run_kia_panda_worker.sh |
Convenience launcher for vehicle Panda worker (sets PYTHONPATH for thirdparty). |
src/core/deviceio_base/cpp/inc/deviceio_base/steering_wheel_tracker_base.hpp |
Adds the tracker-impl interface for steering wheel data. |
src/core/deviceio_trackers/cpp/CMakeLists.txt |
Adds steering wheel tracker sources/headers to the deviceio tracker library. |
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/steering_wheel_tracker.hpp |
New public tracker facade for steering wheel state. |
src/core/deviceio_trackers/cpp/steering_wheel_tracker.cpp |
Implements the new tracker facade. |
src/core/deviceio_trackers/python/deviceio_trackers_init.py |
Exposes SteeringWheelTracker in the Python tracker package. |
src/core/deviceio_trackers/python/tracker_bindings.cpp |
Adds pybind bindings for SteeringWheelTracker. |
src/core/live_trackers/cpp/CMakeLists.txt |
Adds live steering wheel tracker implementation to live trackers lib. |
src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp |
Adds factory support for live steering wheel tracker impl. |
src/core/live_trackers/cpp/live_deviceio_factory.cpp |
Creates live steering wheel tracker impls and registers extension requirements. |
src/core/live_trackers/cpp/live_steering_wheel_tracker_impl.cpp |
Implements live steering wheel schema tracker + optional MCAP recording channels. |
src/core/live_trackers/cpp/live_steering_wheel_tracker_impl.hpp |
Declares live steering wheel tracker implementation. |
src/core/mcap/cpp/inc/mcap/recording_traits.hpp |
Adds recording traits for steering wheel channels/schema naming. |
src/core/python/deviceio_init.py |
Exposes SteeringWheelTracker + SteeringWheelOutput in isaacteleop.deviceio. |
src/core/replay_trackers/cpp/CMakeLists.txt |
Adds replay steering wheel tracker implementation to replay trackers lib. |
src/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hpp |
Adds factory support for replay steering wheel tracker impl. |
src/core/replay_trackers/cpp/replay_deviceio_factory.cpp |
Registers replay steering wheel tracker creation in the dispatch table. |
src/core/replay_trackers/cpp/replay_steering_wheel_tracker_impl.cpp |
Implements replay steering wheel tracker from MCAP. |
src/core/replay_trackers/cpp/replay_steering_wheel_tracker_impl.hpp |
Declares replay steering wheel tracker implementation. |
src/core/retargeting_engine/python/deviceio_source_nodes/__init__.py |
Exposes SteeringWheelSource and its tensor types. |
src/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.py |
Adds tensor wrapper types for tracked steering wheel data. |
src/core/retargeting_engine/python/deviceio_source_nodes/steering_wheel_source.py |
New DeviceIO-to-retargeting-engine source node for steering wheel input. |
src/core/retargeting_engine/python/tensor_types/__init__.py |
Exports SteeringWheelInput + index enum. |
src/core/retargeting_engine/python/tensor_types/indices.py |
Adds SteeringWheelInputIndex. |
src/core/retargeting_engine/python/tensor_types/standard_types.py |
Defines SteeringWheelInput tensor group type. |
src/core/schema/fbs/steering_wheel.fbs |
New FlatBuffer schema for steering wheel output (+ tracked/record wrappers). |
src/core/schema/fbs/vehicle_control.fbs |
New FlatBuffer schema for normalized vehicle control commands (+ record wrapper). |
src/core/schema/python/CMakeLists.txt |
Adds new binding headers to the schema pybind module target. |
src/core/schema/python/schema_init.py |
Exposes new schema types in the Python package API. |
src/core/schema/python/schema_module.cpp |
Binds steering wheel and vehicle control types in _schema. |
src/core/schema/python/steering_wheel_bindings.h |
Implements Python bindings for steering wheel schema types. |
src/core/schema/python/vehicle_control_bindings.h |
Implements Python bindings for vehicle control schema types. |
src/core/schema_tests/cpp/CMakeLists.txt |
Adds C++ schema tests for the new generated types. |
src/core/schema_tests/cpp/test_steering_wheel.cpp |
C++ unit tests for generated steering wheel FlatBuffer types. |
src/core/schema_tests/cpp/test_vehicle_control.cpp |
C++ unit tests for generated vehicle control FlatBuffer types. |
src/core/schema_tests/python/test_vehicle_io.py |
Python unit tests for new schema bindings. |
src/plugins/steering_wheel/CMakeLists.txt |
Adds Linux-only steering wheel plugin build/install rules. |
src/plugins/steering_wheel/README.md |
Documents steering wheel plugin usage and axis mapping. |
src/plugins/steering_wheel/main.cpp |
Entry point for the steering wheel plugin binary. |
src/plugins/steering_wheel/plugin.yaml |
Plugin manifest metadata. |
src/plugins/steering_wheel/steering_wheel_plugin.cpp |
Implements joystick reading + schema pushing for steering wheel output. |
src/plugins/steering_wheel/steering_wheel_plugin.hpp |
Declares steering wheel plugin class and axis mapping. |
src/retargeters/__init__.py |
Exposes the new vehicle control retargeter in the retargeters package API. |
src/retargeters/vehicle_control_retargeter.py |
Adds steering wheel -> normalized vehicle control command retargeter. |
Comment on lines
+145
to
+152
| void SteeringWheelPlugin::close_device() | ||
| { | ||
| if (device_fd_ < 0) | ||
| return; | ||
|
|
||
| close(device_fd_); | ||
| device_fd_ = -1; | ||
| } |
Comment on lines
+52
to
+56
| def retarget( | ||
| self, sample: SteeringWheelLike | SteeringWheelOutput, *, sequence: int | ||
| ) -> VehicleControlCommand: | ||
| steer = self._apply_deadzone( | ||
| (sample.steering - self._steering_neutral) * self._config.steer_scale, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/vehicle_teleop/python/vehicle_teleop/command_log.py`:
- Around line 106-133: The context manager's resource cleanup is not
exception-safe because if an exception occurs during writer setup in the
__enter__ method or during finish() in the __exit__ method, the file descriptor
may never be closed, leaving resources open and potentially masking the original
error. Wrap the writer registration logic in __enter__ with a try/finally block
to ensure the file is closed if any exception occurs during setup, and wrap the
self._writer.finish() call in __exit__ with a try/finally block to ensure the
file close operation in the finally clause always executes even if finish()
throws an exception.
In
`@examples/vehicle_teleop/python/vehicle_teleop/isaac_keyboard_control_worker.py`:
- Line 119: Add validation for the self._rate_hz parameter to reject
non-positive values (zero or negative). Before the division operation at line
119 where period_s is calculated from 1.0 / self._rate_hz, ensure that
self._rate_hz is greater than zero by either validating it during initialization
of the class or immediately before it is used. This will prevent division by
zero crashes and ensure that sleep duration calculations at lines 149 and 198
receive valid positive rate values.
In
`@examples/vehicle_teleop/python/vehicle_teleop/isaac_remote_steering_worker.py`:
- Around line 192-193: The `--rate-hz` parameter stored in `self._rate_hz` is
not validated to be strictly positive, causing a ZeroDivisionError when
computing period_s at line 192 (division by zero) and potentially creating a
busy loop when negative values are used in operations at lines 230 and 339. Add
validation when the rate_hz parameter is parsed or set to ensure it is strictly
greater than 0, raising an appropriate error with a clear message if the
validation fails.
In `@examples/vehicle_teleop/python/vehicle_teleop/kia_panda_worker.py`:
- Around line 72-73: The ZMQ subscription at the setsockopt_string call is using
prefix-based matching which allows similarly-prefixed topics to pass through
unintended, and the message parsing logic (also referenced in the comment at
lines 92-95) lacks error handling for malformed frames or JSON payloads, causing
worker termination. Implement exact topic matching by filtering received
messages against the exact self._topic value instead of relying on ZMQ's prefix
subscription, and wrap all message frame parsing and JSON deserialization logic
in try-except blocks to gracefully handle and log malformed payloads without
crashing the worker loop.
In `@src/plugins/steering_wheel/steering_wheel_plugin.cpp`:
- Around line 145-152: In the close_device() method of the SteeringWheelPlugin
class, after closing the file descriptor with close(device_fd_), clear the
cached control state by resetting the member variables axes_, buttons_, and hat_
to their neutral/default states (typically empty or zero values). This prevents
push_current_state() from continuing to publish stale control commands after the
device has been disconnected or failed to open, which could cause stuck pedal or
button states.
In `@src/retargeters/vehicle_control_retargeter.py`:
- Around line 55-75: The issue is that non-finite values (NaN or infinity) in
the steering, throttle, and brake inputs are propagated through the _clamp and
_apply_deadzone functions unsafely, potentially saturating control commands. Add
validation checks for sample.steering, sample.throttle, and sample.brake to
ensure they are finite values before the current processing begins. If any input
is non-finite, replace it with 0.0 (neutral position) before applying the
deadzone and scaling operations to prevent NaN from being clamped to -1.0 which
would cause full steering deflection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d1297dbd-2b3a-45c4-a33b-cf18e841d491
📒 Files selected for processing (60)
CMakeLists.txtexamples/vehicle_teleop/.gitignoreexamples/vehicle_teleop/CMakeLists.txtexamples/vehicle_teleop/README.mdexamples/vehicle_teleop/config/steering_wheel_config.yamlexamples/vehicle_teleop/python/pyproject.tomlexamples/vehicle_teleop/python/vehicle_teleop/__init__.pyexamples/vehicle_teleop/python/vehicle_teleop/command_log.pyexamples/vehicle_teleop/python/vehicle_teleop/isaac_keyboard_control_worker.pyexamples/vehicle_teleop/python/vehicle_teleop/isaac_remote_steering_worker.pyexamples/vehicle_teleop/python/vehicle_teleop/kia_panda_worker.pyexamples/vehicle_teleop/python/vehicle_teleop/replay_command_mcap.pyexamples/vehicle_teleop/python/vehicle_teleop/vehicle_command.pyexamples/vehicle_teleop/scripts/replay_command_mcap.shexamples/vehicle_teleop/scripts/run_isaac_keyboard_control_worker.shexamples/vehicle_teleop/scripts/run_isaac_remote_steering_worker.shexamples/vehicle_teleop/scripts/run_kia_panda_worker.shsrc/core/deviceio_base/cpp/inc/deviceio_base/steering_wheel_tracker_base.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/steering_wheel_tracker.hppsrc/core/deviceio_trackers/cpp/steering_wheel_tracker.cppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/core/live_trackers/cpp/live_steering_wheel_tracker_impl.cppsrc/core/live_trackers/cpp/live_steering_wheel_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/replay_trackers/cpp/CMakeLists.txtsrc/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hppsrc/core/replay_trackers/cpp/replay_deviceio_factory.cppsrc/core/replay_trackers/cpp/replay_steering_wheel_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_steering_wheel_tracker_impl.hppsrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/steering_wheel_source.pysrc/core/retargeting_engine/python/tensor_types/__init__.pysrc/core/retargeting_engine/python/tensor_types/indices.pysrc/core/retargeting_engine/python/tensor_types/standard_types.pysrc/core/schema/fbs/steering_wheel.fbssrc/core/schema/fbs/vehicle_control.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cppsrc/core/schema/python/steering_wheel_bindings.hsrc/core/schema/python/vehicle_control_bindings.hsrc/core/schema_tests/cpp/CMakeLists.txtsrc/core/schema_tests/cpp/test_steering_wheel.cppsrc/core/schema_tests/cpp/test_vehicle_control.cppsrc/core/schema_tests/python/test_vehicle_io.pysrc/plugins/steering_wheel/CMakeLists.txtsrc/plugins/steering_wheel/README.mdsrc/plugins/steering_wheel/main.cppsrc/plugins/steering_wheel/plugin.yamlsrc/plugins/steering_wheel/steering_wheel_plugin.cppsrc/plugins/steering_wheel/steering_wheel_plugin.hppsrc/retargeters/__init__.pysrc/retargeters/vehicle_control_retargeter.py
Comment on lines
+106
to
+133
| def __enter__(self) -> "McapCommandLogger": | ||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._file = self._path.open("wb") | ||
| self._writer = Writer(self._file, compression=CompressionType.NONE) | ||
| self._writer.start() | ||
| schema_id = self._writer.register_schema( | ||
| name="vehicle_teleop.VehicleControlCommandRecord", | ||
| encoding="jsonschema", | ||
| data=json.dumps(VEHICLE_COMMAND_SCHEMA, separators=(",", ":")).encode( | ||
| "utf-8" | ||
| ), | ||
| ) | ||
| self._channel_id = self._writer.register_channel( | ||
| topic=VEHICLE_COMMAND_TOPIC, | ||
| message_encoding="json", | ||
| schema_id=schema_id, | ||
| ) | ||
| return self | ||
|
|
||
| def __exit__(self, _exc_type, _exc_value, _traceback) -> None: | ||
| if self._writer is not None: | ||
| self._writer.finish() | ||
| self._writer = None | ||
| self._channel_id = None | ||
| if self._file is not None: | ||
| self._file.close() | ||
| self._file = None | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make context-manager cleanup exception-safe.
If setup or finish() throws, Line 130-132 may never run, leaving the file descriptor open and potentially masking the original failure. Guard writer setup/finish with try/finally so file close always executes.
Proposed fix
class McapCommandLogger:
@@
def __enter__(self) -> "McapCommandLogger":
self._path.parent.mkdir(parents=True, exist_ok=True)
self._file = self._path.open("wb")
- self._writer = Writer(self._file, compression=CompressionType.NONE)
- self._writer.start()
- schema_id = self._writer.register_schema(
- name="vehicle_teleop.VehicleControlCommandRecord",
- encoding="jsonschema",
- data=json.dumps(VEHICLE_COMMAND_SCHEMA, separators=(",", ":")).encode(
- "utf-8"
- ),
- )
- self._channel_id = self._writer.register_channel(
- topic=VEHICLE_COMMAND_TOPIC,
- message_encoding="json",
- schema_id=schema_id,
- )
+ try:
+ self._writer = Writer(self._file, compression=CompressionType.NONE)
+ self._writer.start()
+ schema_id = self._writer.register_schema(
+ name="vehicle_teleop.VehicleControlCommandRecord",
+ encoding="jsonschema",
+ data=json.dumps(VEHICLE_COMMAND_SCHEMA, separators=(",", ":")).encode(
+ "utf-8"
+ ),
+ )
+ self._channel_id = self._writer.register_channel(
+ topic=VEHICLE_COMMAND_TOPIC,
+ message_encoding="json",
+ schema_id=schema_id,
+ )
+ except Exception:
+ if self._file is not None:
+ self._file.close()
+ self._file = None
+ self._writer = None
+ self._channel_id = None
+ raise
return self
def __exit__(self, _exc_type, _exc_value, _traceback) -> None:
- if self._writer is not None:
- self._writer.finish()
- self._writer = None
- self._channel_id = None
- if self._file is not None:
- self._file.close()
- self._file = None
+ try:
+ if self._writer is not None:
+ self._writer.finish()
+ finally:
+ self._writer = None
+ self._channel_id = None
+ if self._file is not None:
+ self._file.close()
+ self._file = None🧰 Tools
🪛 ast-grep (0.44.0)
[info] 113-113: use jsonify instead of json.dumps for JSON output
Context: json.dumps(VEHICLE_COMMAND_SCHEMA, separators=(",", ":"))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/vehicle_teleop/python/vehicle_teleop/command_log.py` around lines
106 - 133, The context manager's resource cleanup is not exception-safe because
if an exception occurs during writer setup in the __enter__ method or during
finish() in the __exit__ method, the file descriptor may never be closed,
leaving resources open and potentially masking the original error. Wrap the
writer registration logic in __enter__ with a try/finally block to ensure the
file is closed if any exception occurs during setup, and wrap the
self._writer.finish() call in __exit__ with a try/finally block to ensure the
file close operation in the finally clause always executes even if finish()
throws an exception.
| self._register_signal_handlers() | ||
| self._socket.setsockopt(zmq.LINGER, 0) | ||
| self._socket.bind(self._bind) | ||
| period_s = 1.0 / self._rate_hz |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-positive publish rates.
Line 119 divides by self._rate_hz; 0 crashes and negative values force near-zero sleep at Line 149.
Proposed fix
+def _positive_float(value: str) -> float:
+ parsed = float(value)
+ if parsed <= 0.0:
+ raise argparse.ArgumentTypeError("value must be > 0")
+ return parsed
+
def build_parser() -> argparse.ArgumentParser:
@@
- parser.add_argument("--rate-hz", type=float, default=50.0, help="Publish rate.")
+ parser.add_argument(
+ "--rate-hz",
+ type=_positive_float,
+ default=50.0,
+ help="Publish rate (Hz, must be > 0).",
+ )Also applies to: 149-149, 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/vehicle_teleop/python/vehicle_teleop/isaac_keyboard_control_worker.py`
at line 119, Add validation for the self._rate_hz parameter to reject
non-positive values (zero or negative). Before the division operation at line
119 where period_s is calculated from 1.0 / self._rate_hz, ensure that
self._rate_hz is greater than zero by either validating it during initialization
of the class or immediately before it is used. This will prevent division by
zero crashes and ensure that sleep duration calculations at lines 149 and 198
receive valid positive rate values.
Comment on lines
+192
to
+193
| period_s = 1.0 / self._rate_hz | ||
| print( |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate --rate-hz as strictly positive.
Line 192 divides by self._rate_hz; 0 raises ZeroDivisionError, and negative values create a busy loop at Line 230.
Proposed fix
+def _positive_float(value: str) -> float:
+ parsed = float(value)
+ if parsed <= 0.0:
+ raise argparse.ArgumentTypeError("value must be > 0")
+ return parsed
+
def build_parser() -> argparse.ArgumentParser:
@@
- parser.add_argument("--rate-hz", type=float, default=50.0, help="Publish rate.")
+ parser.add_argument(
+ "--rate-hz",
+ type=_positive_float,
+ default=50.0,
+ help="Publish rate (Hz, must be > 0).",
+ )Also applies to: 230-230, 339-339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@examples/vehicle_teleop/python/vehicle_teleop/isaac_remote_steering_worker.py`
around lines 192 - 193, The `--rate-hz` parameter stored in `self._rate_hz` is
not validated to be strictly positive, causing a ZeroDivisionError when
computing period_s at line 192 (division by zero) and potentially creating a
busy loop when negative values are used in operations at lines 230 and 339. Add
validation when the rate_hz parameter is parsed or set to ensure it is strictly
greater than 0, raising an appropriate error with a clear message if the
validation fails.
Comment on lines
+72
to
+73
| self._socket.setsockopt_string(zmq.SUBSCRIBE, self._topic) | ||
| self._socket.connect(self._connect) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Harden inbound ZMQ parsing (topic filter + malformed payload handling).
Current subscription is prefix-based, so similarly-prefixed topics can pass through, and malformed frames/JSON will raise and terminate the worker loop.
Proposed fix
def _open_socket(self) -> None:
self._socket.setsockopt(zmq.LINGER, 0)
self._socket.setsockopt(zmq.CONFLATE, 1)
- self._socket.setsockopt_string(zmq.SUBSCRIBE, self._topic)
+ self._socket.setsockopt_string(zmq.SUBSCRIBE, f"{self._topic} ")
self._socket.connect(self._connect)
self._poller.register(self._socket, zmq.POLLIN)
@@
def _poll_once(self) -> bool:
events = dict(self._poller.poll(timeout=self._poll_ms))
if self._socket not in events:
return False
- message = self._socket.recv_string()
- _topic, payload = message.split(" ", 1)
- self._last_command = VehicleControlCommand.from_dict(json.loads(payload))
+ try:
+ message = self._socket.recv_string()
+ topic, payload = message.split(" ", 1)
+ if topic != self._topic:
+ return False
+ parsed = VehicleControlCommand.from_dict(json.loads(payload))
+ except (ValueError, json.JSONDecodeError, KeyError, TypeError):
+ return False
+
+ self._last_command = parsed
self._last_received = time.monotonic()
self._neutral_sent_after_timeout = False
return TrueAlso applies to: 92-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/vehicle_teleop/python/vehicle_teleop/kia_panda_worker.py` around
lines 72 - 73, The ZMQ subscription at the setsockopt_string call is using
prefix-based matching which allows similarly-prefixed topics to pass through
unintended, and the message parsing logic (also referenced in the comment at
lines 92-95) lacks error handling for malformed frames or JSON payloads, causing
worker termination. Implement exact topic matching by filtering received
messages against the exact self._topic value instead of relying on ZMQ's prefix
subscription, and wrap all message frame parsing and JSON deserialization logic
in try-except blocks to gracefully handle and log malformed payloads without
crashing the worker loop.
Comment on lines
+145
to
+152
| void SteeringWheelPlugin::close_device() | ||
| { | ||
| if (device_fd_ < 0) | ||
| return; | ||
|
|
||
| close(device_fd_); | ||
| device_fd_ = -1; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Clear cached control state on device loss to prevent stuck commands.
close_device() only drops the FD, but push_current_state() continues publishing cached axes_/buttons_/hat_. If disconnect happens while a pedal is pressed, non-zero control can keep being sent after unplug/open failure.
Suggested fix
void SteeringWheelPlugin::close_device()
{
if (device_fd_ < 0)
return;
close(device_fd_);
device_fd_ = -1;
+ std::fill(axes_.begin(), axes_.end(), 0.0f);
+ std::fill(buttons_.begin(), buttons_.end(), 0);
+ hat_ = { 0, 0 };
}Also applies to: 154-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/steering_wheel/steering_wheel_plugin.cpp` around lines 145 - 152,
In the close_device() method of the SteeringWheelPlugin class, after closing the
file descriptor with close(device_fd_), clear the cached control state by
resetting the member variables axes_, buttons_, and hat_ to their
neutral/default states (typically empty or zero values). This prevents
push_current_state() from continuing to publish stale control commands after the
device has been disconnected or failed to open, which could cause stuck pedal or
button states.
Comment on lines
+55
to
+75
| steer = self._apply_deadzone( | ||
| (sample.steering - self._steering_neutral) * self._config.steer_scale, | ||
| self._config.steering_deadzone, | ||
| ) | ||
| throttle = self._apply_deadzone( | ||
| axis_to_pedal(sample.throttle) * self._config.throttle_scale, | ||
| self._config.pedal_deadzone, | ||
| ) | ||
| brake = self._apply_deadzone( | ||
| axis_to_pedal(sample.brake) * self._config.brake_scale, | ||
| self._config.pedal_deadzone, | ||
| ) | ||
| accel = _clamp(throttle - brake, -1.0, 1.0) | ||
|
|
||
| return VehicleControlCommand( | ||
| sequence=sequence, | ||
| steer=_clamp(steer, -1.0, 1.0), | ||
| accel=accel, | ||
| throttle=accel if accel > 0.0 else 0.0, | ||
| brake=-accel if accel < 0.0 else 0.0, | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
def clamp(value, lo, hi):
return min(hi, max(lo, float(value)))
nan = float("nan")
print("steer clamp:", clamp(nan, -1.0, 1.0)) # currently -1.0
print("pedal clamp:", clamp(nan, 0.0, 1.0)) # currently 0.0
PYRepository: NVIDIA/IsaacTeleop
Length of output: 191
🏁 Script executed:
# First, explore the repository structure and locate the file
git ls-files | grep -E "(vehicle_control|retargeter)" | head -20Repository: NVIDIA/IsaacTeleop
Length of output: 1412
🏁 Script executed:
# Check if the file exists and get its size
wc -l src/retargeters/vehicle_control_retargeter.py 2>/dev/null || echo "File not found"Repository: NVIDIA/IsaacTeleop
Length of output: 205
🏁 Script executed:
# Map the file structure with ast-grep to understand context
ast-grep outline src/retargeters/vehicle_control_retargeter.py --view expandedRepository: NVIDIA/IsaacTeleop
Length of output: 890
🏁 Script executed:
# Read the actual code at the specified lines
sed -n '1,10p' src/retargeters/vehicle_control_retargeter.pyRepository: NVIDIA/IsaacTeleop
Length of output: 477
🏁 Script executed:
# Read lines 52-90 to see the full context of retarget() and helper functions
sed -n '52,90p' src/retargeters/vehicle_control_retargeter.pyRepository: NVIDIA/IsaacTeleop
Length of output: 1537
🏁 Script executed:
# Check for any existing NaN/finite validation in the file
rg -i "isfinite|nan|math\.is" src/retargeters/vehicle_control_retargeter.pyRepository: NVIDIA/IsaacTeleop
Length of output: 156
🏁 Script executed:
# Check if math module is already imported
rg "^import|^from" src/retargeters/vehicle_control_retargeter.py | head -10Repository: NVIDIA/IsaacTeleop
Length of output: 327
Guard against non-finite inputs in vehicle control retargeting to prevent saturated commands.
The current _clamp() function propagates NaN values through min/max comparisons in an unsafe way. Testing shows _clamp(NaN, -1.0, 1.0) yields -1.0, which could saturate steering to full left turn. Validate steering, throttle, and brake inputs as finite before applying deadzone/clamp, defaulting invalid values to 0.0 (neutral).
🔧 Proposed fix
+import math
from dataclasses import dataclass
from typing import Protocol
@@
def retarget(
self, sample: SteeringWheelLike | SteeringWheelOutput, *, sequence: int
) -> VehicleControlCommand:
+ steering_in = _finite_or_zero(sample.steering)
+ throttle_in = _finite_or_zero(sample.throttle)
+ brake_in = _finite_or_zero(sample.brake)
+
steer = self._apply_deadzone(
- (sample.steering - self._steering_neutral) * self._config.steer_scale,
+ (steering_in - self._steering_neutral) * self._config.steer_scale,
self._config.steering_deadzone,
)
throttle = self._apply_deadzone(
- axis_to_pedal(sample.throttle) * self._config.throttle_scale,
+ axis_to_pedal(throttle_in) * self._config.throttle_scale,
self._config.pedal_deadzone,
)
brake = self._apply_deadzone(
- axis_to_pedal(sample.brake) * self._config.brake_scale,
+ axis_to_pedal(brake_in) * self._config.brake_scale,
self._config.pedal_deadzone,
)
@@
+def _finite_or_zero(value: float) -> float:
+ v = float(value)
+ return v if math.isfinite(v) else 0.0
+
+
def _clamp(value: float, lower: float, upper: float) -> float:
return min(upper, max(lower, float(value)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/retargeters/vehicle_control_retargeter.py` around lines 55 - 75, The
issue is that non-finite values (NaN or infinity) in the steering, throttle, and
brake inputs are propagated through the _clamp and _apply_deadzone functions
unsafely, potentially saturating control commands. Add validation checks for
sample.steering, sample.throttle, and sample.brake to ensure they are finite
values before the current processing begins. If any input is non-finite, replace
it with 0.0 (neutral position) before applying the deadzone and scaling
operations to prevent NaN from being clamped to -1.0 which would cause full
steering deflection.
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
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.
Description
The goal of this PR is to enable vehicle teleoperation via a drive-by-wire (DBW) solution. Input controls are from steering wheel/pedals or a keyboard setup for easier local testing, and sent to Comma.ai's Panda device. The Panda device takes care of sending the input control data as CAN messages to the car's CAN bus.
Type of change
Testing
Testing done on physical vehicle.
TBD: Add demo videos and commands after I've added example code
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit
New Features
Tests