feat(skills): typed 2D Odometry state for skills — quaternions kept as legacy - #516
Merged
Conversation
LAST_ODOM used to inject a raw-message dict (nested pose + quaternion orientation, with a bolted-on theta_degrees). Skills on a diff-drive base only ever want the flat 2D pose, and the docs promised velocity data that was never provided. - new innate.Odometry frozen dataclass: x, y, theta (rad) + theta_degrees/position shorthands, body velocities from the twist, stamp and frame ids. ROS-free module so it tests without rclpy. - RobotStateProvider injects Odometry instead of the dict - move_straight / turn_in_place use the attributes - dict-style access from <=0.6.x skills still works via a deprecated __getitem__ shim (legacy shape reconstructed, incl. the quaternion); pinned in test_odometry_state.py in the fast no-ROS bucket
Contributor
Greptile SummaryThis PR changes odometry state for skills from a raw dict to a typed 2D object. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (8): Last reviewed commit: "fix(skills): enforce theta wrap in __pos..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The flat 2D pose covers the common case; skills doing their own filtering or fusion get the complete nav_msgs/Odometry as plain data (real quaternion, z, covariances, full twist) on Odometry.raw. The legacy dict shim now serves that real data instead of a yaw-reconstructed quaternion (reconstruction kept only for raw-less hand-built instances).
…0.3.0) The legacy dict shape was introduced with the RobotState descriptor API in 0.3.0 and shipped unchanged through 0.6.0; name that range in the deprecation warning, shim comment, test, and docs instead of the vaguer 'up to 0.6.x'.
…oval) Per decision to keep backwards compat indefinitely: state explicitly in the shim docstring and the docs Note that dict-style access is not scheduled for removal, so a future maintainer doesn't delete it and old skills never break.
…tems/values
Review follow-up (Greptile): the old LAST_ODOM value was a real dict, so
skills could use defensive access (odom.get('pose'), 'theta_degrees' in
odom, .keys()) that bracket-only __getitem__ didn't cover. All mapping
methods now delegate to one _legacy_mapping() (raw or reconstructed +
theta_degrees) and warn once per call. __iter__ deliberately omitted --
an iterable dataclass invites accidental tuple-unpacking. Also clarified
that the ROS-free property is this module's, not the innate namespace's.
…/__bool__ Review follow-up: old skills can also enumerate the odom dict directly (for k in odom, list(odom), dict(odom)), so provide the full read-only mapping protocol rather than a partial one. Explicit __bool__ keeps the documented 'if self.odom:' None-check warning-free -- __len__ would otherwise define truthiness and fire the deprecation warning at 50 Hz.
theo-michel
changed the title
…acy shape Review fixes for PR #516: - raw is now a cached_property built lazily from raw_source (the ROS message, duck-typed so the module stays ROS-free). The 50 Hz injection path no longer materializes the full rosbridge dict (twist + two 36-float covariance copies) that no current skill reads. - raw_source is compare=False, so ==/hash use only the 2D snapshot fields; production instances (which carry a dict/message) are no longer unhashable while hand-built ones were hashable. - Legacy dict access now projects to exactly the 0.3.0-0.6.x injected shape ({header, child_frame_id, pose.pose, theta_degrees}) on both construction paths, instead of leaking twist/covariance keys the old dict never had when raw was present. Memoized, so dict(odom)/{**odom} builds the mapping once rather than once per key.
vignesh-anand
approved these changes
Jul 10, 2026
theta's docstring promised a wrapped angle but the dataclass stored whatever it was given, so a hand-built Odometry(theta=10.0) silently violated the contract that turn_in_place's heading math relies on. __post_init__ now normalizes out-of-range values via atan2(sin, cos) (guarded, so the 50 Hz injected path -- already atan2 output -- stays trig-free and bit-exact). Range documented as [-pi, pi], which is what atan2 actually returns: the old (-pi, pi] claim was wrong even on the real path (atan2 can return -pi).
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.
Why
RobotState(RobotStateType.LAST_ODOM)handed skills a raw-message dict: nestedpose.posewith a quaternion orientation, plus a bolted-ontheta_degrees. For a differential-drive base on flat ground that's the wrong shape — every consumer immediately dug out(x, y, yaw)— and the docs promised velocity data that was never actually injected (and showed attribute access that never worked on the dict).What
innate.Odometryfrozen dataclass — the state a skill author actually wants:x,y(meters, odom frame),theta(radians, CCW, wrapped to(-pi, pi])theta_degreesproperty (consistent withnavigate_to_position/turn_in_placedegree-based APIs) andposition→(x, y)linear_velocity/angular_velocityfrom the twist — newly exposedstamp(seconds) for staleness checks,frame_id/child_frame_idbrain_client/skills/odometry.py) so it imports and tests without rclpyodom.rawcarries the completenav_msgs/Odometryas plain data with rosbridge-style keys — real quaternion,z, pose/twist covariances, full twist — so power users (filtering, fusion, non-flat ground) aren't boxed in by the flat 2D viewRobotStateProviderbuilds anOdometryfrom the ROS message instead of serializing the dictmove_straight/turn_in_placeread the attributesodom["theta_degrees"],odom["pose"]["pose"]["position"]) still works via a deprecated__getitem__shim, served fromraw(the real message data); a yaw-only reconstruction covers hand-built instances withoutrawTesting
rawpassthrough, and the legacy mapping protocol (all passed); not kept in the treeruff check/ruff format/ pre-commit cleanDocs PR: innate-inc/docs#19