Pack an arm's state once, for every arm that states the same things - #721
Open
DarksaCY wants to merge 3 commits into
Open
Pack an arm's state once, for every arm that states the same things#721DarksaCY wants to merge 3 commits into
DarksaCY wants to merge 3 commits into
Conversation
`YamState` is `PackedState(n_joints)` in `state.py`: the joints, their velocities, the end effector pose and the status, in one float32 array. Five of the six state classes in the repository are that layout with a different joint count, and only the Franka's differs -- it carries a wrench. The offsets are the instance's, computed from the joint count, and `instantiation_params` names that count: shared memory rebuilds the payload on the far side from it, and a state that did not name it would come back the wrong size. The YAM reads it; `python -m positronic.drivers.roboarm.yam --fake` passes. Kinova, SO-101 and the MuJoCo sim can follow.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb3614d2fc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`n_joints` was a second copy of what the slices already say, and `instantiation_params` reads it: one that disagreed with the layout would rebuild a receiver of another size. It is a property of the layout now. `_q`, `_dq`, `_ee` and `_status` held slices and an index rather than the values they name; they say which.
A test that assigns `n_joints` to prove it cannot be assigned needs a suppression to type-check, which says the same thing twice.
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
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.
YamStatebecomesPackedState(n_joints)inpositronic/drivers/roboarm/state.py: the joints, their velocities, the end effector pose and the arm's status, in one float32 array.Five of the six state classes in the repository are that layout with a different joint count -- Kinova 7, SO-101 5, the MuJoCo sim 7, YAM 6, Trossen 6. Only
FrankaStatediffers: it carries a wrench.The offsets are the instance's, computed from the joint count, and
instantiation_paramsreturns that count. Shared memory rebuilds the payload on the far side withdata_type(*instantiation_params), so a state that did not name its joint count would come back the wrong size and read another arm's numbers;test_state.pypins that, along with the round trip and the fact that a reading is a copy rather than a view of the buffer that is rewritten every tick.The YAM is the only driver this changes, and there the change is a rename:
python -m positronic.drivers.roboarm.yam --fakepasses. Kinova, SO-101 and the sim can adopt it later -- happy to open an issue for that if you want it tracked.This is one of the changes the Trossen stack recommendations ask for; #683 drops
TrossenStatefor it once this lands.