This repository was archived by the owner on May 29, 2026. It is now read-only.
Add dcmotor and remotized actuators#5
Merged
jvonmuralt merged 8 commits intoMar 13, 2026
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds two new public actuators (ActuatorDCMotor, ActuatorRemotizedPD), extends the PD controller kernel with optional velocity-dependent and angle-dependent torque limiting, updates PD control-law docs to remove multiplicative G factors, adjusts actuator/kernel call sites, enhances USD parsing for DC motor, and adds comprehensive tests and examples. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
newton_actuators/_src/actuators/dc_motor.py (1)
57-66: Consider validating thatvelocity_limitis positive.The
resolve_argumentsmethod requiresvelocity_limitbut doesn't validate it's positive. A zero or negative value would cause division by zero in the kernel (line 97 of kernels.py) or produce incorrect torque-speed behavior.♻️ Proposed validation
if "velocity_limit" not in args: raise ValueError("ActuatorDCMotor requires 'velocity_limit' argument") + if args["velocity_limit"] <= 0: + raise ValueError("ActuatorDCMotor requires 'velocity_limit' > 0") return { "kp": args.get("kp", 0.0),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/actuators/dc_motor.py` around lines 57 - 66, The resolve_arguments function currently requires "velocity_limit" but doesn't check it's > 0; update resolve_arguments in the ActuatorDCMotor implementation to validate that args["velocity_limit"] is a positive number (coerce to float if needed) and raise a ValueError with a clear message if velocity_limit <= 0 (e.g., "ActuatorDCMotor requires positive 'velocity_limit'"). Keep the rest of the returned dict the same and ensure you reference the "velocity_limit" key when performing the check.newton_actuators/_src/actuators/remotized_pd.py (1)
130-137: Consider validating thatlookup_anglesis sorted.The
_interp_1dfunction in kernels.py assumesxsis sorted (per docstring: "sorted sample arrays"). The class docstring mentions the lookup should be "sorted by angle" but there's no runtime validation. A mis-ordered array would produce incorrect interpolation results.♻️ Optional validation
if len(lookup_angles) != len(lookup_torques): raise ValueError( f"lookup_angles length ({len(lookup_angles)}) must match lookup_torques length ({len(lookup_torques)})" ) + # Validate angles are sorted (required for interpolation) + angles_np = lookup_angles.numpy() + if not np.all(np.diff(angles_np) >= 0): + raise ValueError("lookup_angles must be sorted in ascending order") self.lookup_angles = lookup_angles🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton_actuators/_src/actuators/remotized_pd.py` around lines 130 - 137, The constructor currently checks lengths but doesn't verify that lookup_angles is sorted, which _interp_1d (kernels.py) requires; in the RemotizedPD initializer (where self.lookup_angles, self.lookup_torques, self.lookup_size are set) add a runtime validation that lookup_angles is monotonically non-decreasing (e.g., each element >= previous); if the check fails raise a ValueError explaining "lookup_angles must be sorted by angle" (include lengths/first offending indices for clarity) so mis-ordered inputs fail fast rather than producing incorrect interpolation results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton_actuators/_src/kernels.py`:
- Around line 94-101: Add validation to ensure every entry in velocity_limit is
> 0 before any division occurs: in ActuatorDCMotor.resolve_arguments (and/or
prior to the block using saturation_effort in newton_actuators/_src/kernels.py)
check the velocity_limit array values and raise/throw a clear error if any value
is <= 0; this prevents division-by-zero in the saturation computation that uses
vel / vel_lim (variables referenced: velocity_limit, saturation_effort,
current_vel, max_force, max_torque) and keeps existing array-length checks
intact.
- Around line 35-38: The loop in the interpolation (for k in range(n - 1)) can
divide by zero when xs[k + 1] == xs[k] because of the denominator (xs[k + 1] -
xs[k]); add a guard inside that loop to detect a zero denominator and handle it
(either raise a clear ValueError stating lookup angles must be strictly
increasing, or return a sensible value such as ys[k] / ys[k + 1] or their
average) so the code never performs (x - xs[k]) / (xs[k + 1] - xs[k]) when the
difference is zero; update the interpolation branch that computes t = (x -
xs[k]) / (xs[k + 1] - xs[k]) accordingly.
In `@README.md`:
- Line 61: The README bullet currently uses the actuator class name
`ActuatorRemotizedPD` where the other items list the state type; update the text
to reference the state type `ActuatorRemotizedPD.State` so it matches the
pattern used for other entries and removes ambiguity—edit the bullet that reads
"`ActuatorRemotizedPD` - Inherits `ActuatorDelayedPD.State` (same delay
buffers)" to instead use "`ActuatorRemotizedPD.State` - Inherits
`ActuatorDelayedPD.State` (same delay buffers)".
- Around line 42-43: Update the ActuatorDCMotor documentation to describe that
torque is clamped to both a velocity-dependent lower and upper bound (τ_min(v)
and τ_max(v)) rather than only τ_max(v); replace the current single-bound
formula with a short description showing that τ_limit = clamp(τ, τ_min(v),
τ_max(v)) where τ_min(v) and τ_max(v) are computed from the motor torque-speed
curve (or explicitly state if the kernel uses symmetric sign handling), and
reference ActuatorDCMotor and the τ_max(v) expression so readers know to treat
both bounds when integrating.
---
Nitpick comments:
In `@newton_actuators/_src/actuators/dc_motor.py`:
- Around line 57-66: The resolve_arguments function currently requires
"velocity_limit" but doesn't check it's > 0; update resolve_arguments in the
ActuatorDCMotor implementation to validate that args["velocity_limit"] is a
positive number (coerce to float if needed) and raise a ValueError with a clear
message if velocity_limit <= 0 (e.g., "ActuatorDCMotor requires positive
'velocity_limit'"). Keep the rest of the returned dict the same and ensure you
reference the "velocity_limit" key when performing the check.
In `@newton_actuators/_src/actuators/remotized_pd.py`:
- Around line 130-137: The constructor currently checks lengths but doesn't
verify that lookup_angles is sorted, which _interp_1d (kernels.py) requires; in
the RemotizedPD initializer (where self.lookup_angles, self.lookup_torques,
self.lookup_size are set) add a runtime validation that lookup_angles is
monotonically non-decreasing (e.g., each element >= previous); if the check
fails raise a ValueError explaining "lookup_angles must be sorted by angle"
(include lengths/first offending indices for clarity) so mis-ordered inputs fail
fast rather than producing incorrect interpolation results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b88be0a-a809-477c-8adf-7dba196877dd
📒 Files selected for processing (9)
README.mdnewton_actuators/__init__.pynewton_actuators/_src/actuators/__init__.pynewton_actuators/_src/actuators/dc_motor.pynewton_actuators/_src/actuators/delayed_pd.pynewton_actuators/_src/actuators/pd.pynewton_actuators/_src/actuators/remotized_pd.pynewton_actuators/_src/kernels.pynewton_actuators/tests/test_actuators.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton_actuators/_src/usd_parser.py`:
- Around line 98-99: The parser currently infers "DCMotorAPI" when either
"saturationEffort" or "velocityLimit" exists which can cause
ActuatorDCMotor.resolve_arguments to raise a ValueError due to a missing
velocity_limit; update the schema detection in usd_parser.py so you only append
"DCMotorAPI" to schemas when both "saturationEffort" and "velocityLimit" are
present, or alternatively add a defensive check in validate_kwargs (or
ActuatorDCMotor.resolve_arguments) that raises a clear, early error if
"DCMotorAPI" is in schemas but velocity_limit is missing; refer to the schemas
list, the string "DCMotorAPI", and
ActuatorDCMotor.resolve_arguments/validate_kwargs when making the change.
In `@newton_actuators/tests/test_actuators.py`:
- Around line 883-903: The test
test_parse_dc_motor_only_saturation_effort_infers_schema documents that
parse_actuator_prim currently infers ActuatorDCMotor when only saturationEffort
is present but ActuatorDCMotor.resolve_arguments requires velocity_limit and
will raise ValueError; update the test to reflect the intended parser behavior:
either assert that parse_actuator_prim raises/returns an error when
velocity_limit is missing, or change the expectation to validate that
resolve_arguments is not called and an appropriate error is surfaced,
referencing parse_actuator_prim and ActuatorDCMotor.resolve_arguments (or adjust
usd_parser.py decision logic) so the test and parser agree on whether
velocity_limit is mandatory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92516162-9112-4986-b35e-40cc84365cf9
📒 Files selected for processing (4)
README.mdnewton_actuators/_src/kernels.pynewton_actuators/_src/usd_parser.pynewton_actuators/tests/test_actuators.py
Merged
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Tests