-
Notifications
You must be signed in to change notification settings - Fork 111
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
Remove non rpc interface methods AnalogNames
and DigitalInterruptNames
from board
#4626
base: main
Are you sure you want to change the base?
Remove non rpc interface methods AnalogNames
and DigitalInterruptNames
from board
#4626
Conversation
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
can modular boards still optionally implement these functions? or will it break them all, including pi? |
@@ -92,22 +92,6 @@ func (c *client) GPIOPinByName(name string) (GPIOPin, error) { | |||
}, nil | |||
} | |||
|
|||
func (c *client) AnalogNames() []string { | |||
if len(c.info.analogNames) == 0 { |
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.
can boardInfo be removed? seems like we only used it for storing all the names
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.
Will check it out.
modules should be unaffected by this. They would just have extra exported methods on their structs |
This is in fact what happens with an interface removal in go and it is harmless for board modules. With this change I'm no longer forcing module authors to implement these methods that could not be accessed. You can consider the addition of an API to be a forward breaking change for modules. The module will need to implement the new method. Removing a method does not break a module, as the |
Going to wait till I'm back from vacation to merge @martha-johnston good remote work is doing the corresponding changes in sdks - do go ahead and make tickets for this. |
We can now do this cleanup chore since
Status
andStatusStream
are completely unused and the reason for these rpcs to exist is now gone.Tested with Viam server running on a mac with a few fake modular boards.
Will probably wait to merge until SDK work is complete.