-
Notifications
You must be signed in to change notification settings - Fork 126
[RSDK-11093] Make a vision.NewService() function that isn't deprecated #5096
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-11093] Make a vision.NewService() function that isn't deprecated #5096
Conversation
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.
Made small comments, otherwise LGTM!
|
Very good feedback! I've done all 3 suggestions. |
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.
One thing I'd like you to test out is making sure that the mlmodel vision service (our most used vision service) still work as expected.
If you could locally compile and run a viam-server instance and check that our vision service runs as expected (both mlmodel and color-detector) then it will look good to me
|
Tried locally: yes, the mlmodel vision service still appears to work normally! |
@AGanguly13 and I pair programmed this; thanks for his help!
The main changes are:
vizModelstruct no longer stores arobot.Robotwithin it. Instead, it stores alogging.Loggerand a function that takes camera names and returns camera objects.NewService()function has been renamedDeprecatedNewService(). Like before, it takes arobot.Robotas an argument, and can be used withDeprecatedRobotConstructorwhen registering a service.NewService(). It takes aresource.Dependenciesas an object, and can be used withConstructorwhen registering a service.deprecated_vision_service_builder.go(created in split vizModel to a separate file #5094) has been renamed to non-deprecatedvision_service_builder.goThis compiles and the tests pass, but I haven't tried it out in more detail than that. In particular, nothing (yet) uses the non-deprecated
NewService(). That can happen in future PRs, unless you want me to migrate some of the services over immediately.We have created a new function with a new type signature using an old name. However, we don't need to worry about someone outside of RDK using this function in their module and having the module break when they upgrade RDK versions. The reason RSDK-11093 was filed in the first place was that the old code was not usable in modules at all, because
DeprecatedNewService()doesn't play well with modules!