-
Notifications
You must be signed in to change notification settings - Fork 126
fix hanging issue on gantry shutdown #5599
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
base: main
Are you sure you want to change the base?
Conversation
ae86b51 to
7e6b5e2
Compare
7e6b5e2 to
655e9e7
Compare
655e9e7 to
e7662f2
Compare
Availability
Quality
Performance
The above data was generated by running scenes defined in the
|
| } | ||
|
|
||
| m, err := referenceframe.KinematicModelFromFile(newConf.Kinematics, g.Named.Name().String()) | ||
| m, err := referenceframe.KinematicModelFromFile(newConf.Kinematics, g.Named.Name().ShortName()) |
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.
changed this so the motion service can match the model name with the frame config
dgottlieb
left a comment
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.
Thanks for making improvements here. This is hard for me to review. There are many glaring, existing, lifetime bugs in this component, so it's hard for me to say that the patch is "correct" vs band-aiding some of the existing code.
But I wholly expect you're submitting this because it fixed real observed problems, and I have no intention of standing in the way of that.
Very happy to pair up post-merge on my other observations and/or work together on doing a deeper clean of the lifetime management code.
|
Thanks Dan (and John)! I should've given some context. This PR is just fixing some band-aids as you mentioned, and I really just made it so I could get unblocked on the 20%. There definitely needs to be more discussion on the other big issues with the gantry component. Happy to set up a meeting with you guys to discuss more. In that vein, I'll just leave this PR open and not merge as to add to the potential tech debt later. Thanks again! |
This PR fixes a bug that prevents the gantry resource from shutting down. Essentially, a combination of lock issues and context issues are resolved.