RSDK-7268 RSDK-7329 Make StreamTicks take in a list of DigitalInterrupts #3816
Conversation
viambot
added
the
safe to test
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
oliviamiller
changed the title
There was a problem hiding this comment.
A few nits, but looking really good.
For the server/client implementation, is there any way we could pass in nil objects to the digital interrupt client? By the time we get to stream ticks, the request should have given us all the info for the helper function to populate the struct with everything for value and Name?
Can we add tests for Name in the client/server files, since it is new and introduces a new workflow for using the apis.
| @@ -98,6 +98,11 @@ func (i *BasicDigitalInterrupt) RemoveCallback(c chan board.Tick) { | |||
| } | |||
| } | |||
|
|
|||
| // Name returns the name of the digital interrupt. | |||
| func (i *BasicDigitalInterrupt) Name() string { | |||
| return i.cfg.Name | |||
There was a problem hiding this comment.
Same question for the pinwrappers on nil-ness of the config.
components/board/server.go
Outdated
| @@ -229,7 +238,7 @@ func (s *serviceServer) StreamTicks( | |||
| return err | |||
| } | |||
|
|
|||
| defer utils.UncheckedErrorFunc(func() error { return RemoveCallbacks(b, req.PinNames, ticksChan) }) | |||
| defer utils.UncheckedErrorFunc(func() error { return RemoveCallbacks(b, dis, ticksChan) }) | |||
There was a problem hiding this comment.
nice! I'm glad this was so changeable.
| @@ -229,7 +228,7 @@ func (e *Encoder) Start(ctx context.Context, b board.Board, interrupts []string) | |||
|
|
|||
| utils.ManagedGo(func() { | |||
| // Remove the callbacks added by the interrupt stream. | |||
| defer utils.UncheckedErrorFunc(func() error { return board.RemoveCallbacks(b, interrupts, ch) }) | |||
| defer utils.UncheckedErrorFunc(func() error { return board.RemoveCallbacks(b, []board.DigitalInterrupt{e.A, e.B}, ch) }) | |||
There was a problem hiding this comment.
Can we now use the Name() function further down in this code instead of also storing the encAName in the encoder struct?
There was a problem hiding this comment.
Im not sure if we can stop storing the encAName due to how its being used in reconfigure. We use the encAName and compare it to the name in the config to check if the name has changed. the Name() function returns the name in the config so we would have no way to know if the name changed without storing it in the struct.
There was a problem hiding this comment.
lets leave it for another pr. Can we compare the config name with the stored pin's name in Reconfigure directly?
viambot
added
safe to test
| if err != nil { | ||
| return errors.Wrap(err, "error getting digital interrupt ticks") | ||
| } | ||
|
|
||
| c.activeBackgroundWorkers.Add(1) | ||
| utils.ManagedGo(func() { | ||
| // Remove the callbacks added by the interrupt stream once we are done. | ||
| defer utils.UncheckedErrorFunc(func() error { return board.RemoveCallbacks(brd, []string{intName}, tickChan) }) | ||
| defer interrupt.RemoveCallback(tickChan) |
There was a problem hiding this comment.
remember we're goign to remove these functions from the digital interrupt interface as per the scope doc, so we shouldn't increase use of them from the main digital interrupt interface.
There was a problem hiding this comment.
Rand is right, but removing their use is a separate task for a separate PR. If it's a lot of work right now to fix this, just leave it as-is and fix it later.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
| if err != nil { | ||
| return errors.Wrap(err, "error getting digital interrupt ticks") | ||
| } | ||
|
|
||
| c.activeBackgroundWorkers.Add(1) | ||
| utils.ManagedGo(func() { | ||
| // Remove the callbacks added by the interrupt stream once we are done. | ||
| defer utils.UncheckedErrorFunc(func() error { return board.RemoveCallbacks(brd, []string{intName}, tickChan) }) | ||
| defer interrupt.RemoveCallback(tickChan) |
There was a problem hiding this comment.
Rand is right, but removing their use is a separate task for a separate PR. If it's a lot of work right now to fix this, just leave it as-is and fix it later.
viambot
added
safe to test
viambot
added
safe to test
This covers changing the
StreamTicksandRemoveCallbackfunctions to take in a list of interrupts instead of a list of strings, as well as adding theNamefunction to the digitialInterrupt interface.