-
Notifications
You must be signed in to change notification settings - Fork 113
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-2496] Directly refresh webservice subtype services before modular resource loading #2133
Conversation
// Refresh causes the web service to reload it's subtype service maps from the actual robot resources. | ||
func (r *localRobot) Refresh(ctx context.Context) error { |
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.
Should I not be using this function? It felt like it fit in the paradigm, and my thinking was if MORE stuff needs to be refreshed here later, it can be added.
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.
@cheukt I think git said you were responsible for this line/func, so wanted to double check if you think this PR is the correct way to handle this or not. Thanks!
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.
I think this makes sense here
|
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.
Looks good to me. I have also confirmed that these changes also work for the camera server and allow the slam modules to start up correctly.
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.
LGTM for now. Would like @cheukt to chime in when he's back.
NP, will hold off on merging until then. @zaporter-work Let me know if this becomes a blocker before then. |
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.
LGTM
// Refresh causes the web service to reload it's subtype service maps from the actual robot resources. | ||
func (r *localRobot) Refresh(ctx context.Context) error { |
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.
I think this makes sense here
https://viam.atlassian.net/browse/RSDK-2496 describes the problem, but the short example is that it was impossible to use
motor.Stop()
during something likeNewBase()
This should fix that issue, but may not be the best approach. (See inline notes.)