-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSDK-9798 - ensure machine status is ready #831
base: main
Are you sure you want to change the base?
RSDK-9798 - ensure machine status is ready #831
Conversation
stuqdog
commented
Jan 28, 2025
- Ensures machine status is ready when initially creating a robot client.
- Adds a background loop to the robot client that checks machine status, and takes the lock when status is initializing.
async def GetMachineStatus(self, stream: Stream[GetMachineStatusRequest, GetMachineStatusResponse]) -> None: | ||
request = await stream.recv_message() | ||
assert request is not None | ||
await stream.send_message(GetMachineStatusResponse(state=GetMachineStatusResponse.STATE_RUNNING)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to flag this, for the purposes of the documentation tests we need the service to have a GetMachineStatus
implementation. But, I'm leaving it fairly barebones (it just always returns that we're running) because the issue we're trying to fix is one that exposes itself when using RDK as a server specifically.
66ad108
to
01058e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks for looking into this!
Note that this should not be merged until we've had an opportunity to discuss with team netcode and ensure both teams are on the same page as to when and how to use |
with self._lock: | ||
await self._refresh_inner() | ||
|
||
async def _refresh_inner(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has gone from being an important thing to a flyby. I still think that there's value abstractly in being able to call refresh
from within a process that already has the lock so I'm leaving this, but I don't feel strongly and am happy to undo if others do.