[RSDK-746] Return the power percent when checking if a motor is powered #1492
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR is larger than I'd prefer, but I didn't see an obvious way to split it up into smaller pieces. Next time, what should I do differently? |
|
👀 Running |
npmenard
reviewed
Oct 14, 2022
components/motor/collectors.go
Outdated
| @@ -58,7 +58,7 @@ func newIsPoweredCollector(resource interface{}, params data.CollectorParams) (d | |||
| } | |||
|
|
|||
| cFunc := data.CaptureFunc(func(ctx context.Context, _ map[string]*anypb.Any) (interface{}, error) { | |||
| v, err := motor.IsPowered(ctx, nil) | |||
| v, _, err := motor.IsPowered(ctx, nil) | |||
There was a problem hiding this comment.
Are we not collecting the powerPct in the collector?
There was a problem hiding this comment.
I plead ignorance! What's a collector? I didn't understand what this file was for, so made the minimum change that wouldn't affect any behavior.
There was a problem hiding this comment.
Well, I've taken a stab at updating the collector to track the new value, too. LMK if I should have done something different.
npmenard
reviewed
Oct 14, 2022
npmenard
approved these changes
Oct 14, 2022
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm a little nervous that there are so many different implementations of this feature across different motors, and maybe there was a simpler or more consistent thing to do. The general idea here is that the
Motorinterface'sIsPowerednow returns the power level in addition to a boolean and a possible error. This is then used to populate the newpowerPctfield in theIsPoweredResponseprotobuf.The changes to go.mod and go.sum are intended to pick up viamrobotics/api#70.
Most of the motors didn't have an obvious way to get the power percent, so I started storing that in the relevant structs, updating that on a
SetPowercall, and returning whatever had been set.Despite all the tests passing, I have low confidence that I've done this right. What if I forgot to update the stored percent in a certain place? What if I updated it but a subsequent error means it's not actually set in the hardware? What if this PR is so big that everyone's eyes glaze over before they reach the end and we all miss an important bug?
What can I do to gain confidence that this change was done correctly?