Ask a device for the setpoint it is streamed now - #702
Conversation
![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: 292bb0f115
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba193bb534
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cd052b60f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5d64a6e9
ℹ️ 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".
A follower parked with `h` left the pose it was put at and went back, in one step, to where its leader had been seconds earlier. A streamed setpoint crosses to the driver on a queue, which hands the oldest over first, and the driver takes one a tick. A device that reads slower than it is written to therefore falls behind by everything in between, and a move owns the device for its whole travel, in which nothing is read at all. What the driver did after the move was work through the setpoints the move had superseded, at a tick each. `Moves.streamed` reads the newest and lets go of the rest, and the setpoints streamed at a device a move owns go the same way. Every arm driver reads its commands through it, and the arm now stands where the last setpoint asks rather than where an older one did.
`Moves.streamed` reads like a property, and one of its two callers wants only what it drops and throws the value away. `take_newest_setpoint` says that the call takes the setpoints as well as reads them. Its docstring said which run the queue backlog was found on; it states the constraint the method is written against and stops there.
`next_request` drained the stream only while the driver was polling it, and the Franka and YAM loops stop polling for the whole travel of a `sync_move` -- they are held inside `yield from`. So the first poll after the arm arrived handed back a setpoint written on the way there, and drove the arm straight off the target its asker had just been told it reached. `Moves` now notes the tick a call it handed out stopped owning the device, and lets go of every setpoint stamped before it. `next_request` takes the time to note, as `accept` and `settle` already do. The same shape was fixed on the controller side by `_travel`, which drops what arrives while the arms move.
`next_request` inferred the end of a move from the first poll that found the device free. A driver that keeps the move in flight settles it at the end of a tick and polls again only after its limiter sleeps, so a setpoint written in between -- newer than the move -- was read as older and let go. A one-shot command was then lost outright. `settle` notes the tick it ends the move on, and `accept` gives up the inference for the moves it takes. What is left of the inference is the driver held inside the call for the whole travel, which polls again as soon as it returns.
7c5d64a to
7cae5da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cae5dad75
ℹ️ 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".
`next_request` inferred the end of a blocking move from the first poll that found the device free, and that inference was wrong three ways. Franka and YAM yield to their limiter before polling again, so a setpoint written after the travel but before the poll was stamped before the cutoff and thrown away. A call refused before the device moved -- a target out of reach -- still armed the cutoff, though nothing had travelled. And the drain ran before the `busy` check, so a setpoint that arrived while a settled move waited to be answered was read and dropped. `Moves.finished` takes the time instead, and the two drivers held inside the call report it when the travel returns. A device a settled move still owns is asked for nothing and its stream is left alone until the answer is out.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659eb74428
ℹ️ 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".
The cutoff read timestamps, and three things it could not read. A simulated evaluation gives every loop of one scheduler pass the same stamp, so a command written while a move was still active carried the exact cutoff and was kept. `Moves.finished` drops what is queued at the moment the move ends instead, which is the same fact without the arithmetic -- and `next_request` takes no time at all now. `move_to` that raises after the arm travelled skipped the report, so a setpoint from during the failed travel retargeted the arm on the next tick. Franka and YAM report in a `finally`, and refuse a target the arm cannot hold before they enter it. `next_request` drained the stream before it knew whether the call would be taken, so a setpoint queued behind a call the driver then refused was thrown away. The stream is read only where a setpoint is what comes back.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 932015e10c
ℹ️ 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".
The report sat in a `finally` around the whole travel, and both `move_to` bodies do work before the device is commanded -- Franka reads the arm and applies the mode, YAM reads the chain and takes its starting posture. A failure there discarded the streamed setpoints although nothing had moved. Each reports from inside its own travel, after the target is applied. `Moves.finished` said an event at the caller and did a destructive read. It is `discard_streamed_setpoints`, which is what it does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5d9a2e044
ℹ️ 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".
The YAM discard hung on the ramp having run, and a call that names where the chain already stands never enters it: the call answered and the next poll drove the chain away with a setpoint written before it. The chain is read before the ramp either way, so the discard follows that read. `arrived` held any `MoveStatus`, `GAVE_UP` included; it is `status`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0ba225744
ℹ️ 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".
| one would drive it straight back off that pose. ``settle`` does this for a move it ends; a driver | ||
| held inside the call for the whole travel does it when the travel is over. | ||
| """ | ||
| self.take_newest_setpoint() |
There was a problem hiding this comment.
Rule overspecific violated:
Moves assumes every streamed T is an absolute setpoint superseded by the synchronous target, but each arm instantiates it with CommandType, which includes CartesianDelta and JointDelta; these are one-shot relative motions evaluated from the pose when consumed. If either delta is streamed while a synchronous move owns the device, this blanket discard loses the motion instead of applying it relative to the completed pose. Restrict discarding to absolute setpoints or make the stream's replacement semantics explicit in its type.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| while (message := self._async_move.read()) is not None and message.updated: | ||
| latest = message.data |
There was a problem hiding this comment.
Rule hidden-dependency violated:
When an upstream process emits setpoints at least as quickly as this loop reads them, every read() can return another updated message, so take_newest_setpoint() depends on the producer eventually pausing and can remain here indefinitely. In that case the robot driver stops commanding, publishing, and checking its stop signal; use an atomic latest-value transport or impose a finite per-tick drain boundary.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
1 participant
What is wrong
Moves.next_requestreads one message per tick from the stream of setpoints. The transport under it is aqueue:
JointPositionis not shared-memory compliant, soWorld.connectgives the pair a queue, whichhands the oldest message over first. Two things follow.
between, and the gap only grows over a session.
sync_moveowns the device for its whole travel, and nothing reads the stream while it does.So the setpoints the move superseded are still waiting when it lands, and the driver then works through
them, one per tick.
What it does at a rig
Found on a Trossen station driven by a leader arm. The operator asked for the rest pose, the follower
parked, and then left the pose it was put at and went back — in one step — to where the leader had been
seconds earlier.
The change
Moves.streamedreads the newest setpoint and lets go of every one older than it. Setpoints streamed at adevice a move owns go the same way, on the arm path (
next_request) and on the gripper path(
grip_setpoint).Every arm driver reads its commands through
Moves— franka, kinova, so101, trossen, yam and the MuJoCosimulator — so all of them stand where the last setpoint asks rather than where an older one did.
Tests
positronic/drivers/tests/test_utils.py: the newest setpoint is what the device is asked for; a setpointthe device was moved away from is let go; setpoints streamed at a travelling device do not reach it when
the move lands.
test_a_streamed_grip_waits_for_the_call_queue_to_be_emptyis replaced by the first of those. Its premise— "a signal holds only its latest value" — is what this fixes: it does not hold for a queue transport.