RSDK-7753: Remove non-blocking GoFor functionality on 9/1 #4335
Conversation
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
viambot
added
the
safe to test
There was a problem hiding this comment.
There are a bunch of division-by-zero cases everywhere now. Please add tests for the math that explicitly test for that and do not pass if the numbers resulting from terminal actuating calls (GoFor/GoTo), do not return infinite or NaN float returns from their calcualtions.
martha-johnston
force-pushed
the
rsdk-7753
branch
from
fc73392
to
45e8019
Compare
viambot
added
safe to test
| @@ -1,15 +1,16 @@ | |||
| package dimensionengineering_test | |||
| package dimensionengineering | |||
There was a problem hiding this comment.
I changed this package to easier test goForMath because a lot of other motor packages don't use this _test convention, but I am open to changing it back if there's a good reason to do so. It's not fully clear to me what this is useful for.
There was a problem hiding this comment.
This is actually a convention for making sure your test file is in a separate package which forces you to write good tests of the functionality of your exported APIs from that package. If you don't need any of the private functions in the dimensionengineering package to be tested now, please change it back.
There was a problem hiding this comment.
Sure I can revert this and remove the goForMath tests if you'd prefer
There was a problem hiding this comment.
If you don't need any of the private functions in the dimensionengineering package to be tested now, please change it back.
You are testing private function now, so don't change it back.
There was a problem hiding this comment.
sorry I missed this before changing it, but I added GoFor tests so I think it should be fine now
viambot
added
safe to test
viambot
added
safe to test
| if rpm*revolutions == 0.0 { | ||
| dir = 0.0 |
There was a problem hiding this comment.
not familiar with this motor, but is it okay for the GoFor call to not do anything?
There was a problem hiding this comment.
seems fine to me? but now I did add a check to gofor, which is before this point, so we shouldn't get here anyways, but I thought I should leave the 0 case in case goForMath is called directly somewhere else at a future point in time?
| return powerPct, 0 | ||
| dir := 1.0 | ||
| if rpm*revolutions == 0.0 { | ||
| dir = 0.0 |
There was a problem hiding this comment.
similar recommendation, we can return early here, especially because the logic below breaks when rpm == 0
components/motor/fake/motor.go
Outdated
| dir := 1.0 | ||
| if rpm*revolutions == 0.0 { | ||
| dir = 0.0 | ||
| } else if rpm*revolutions < 0.0 { | ||
| dir = -1.0 | ||
| } | ||
|
|
||
| if revolutions == 0 { | ||
| powerPct := rpm / maxRPM | ||
| return powerPct, 0, 1 | ||
| if rpm == 0 { | ||
| return 0, 0, dir |
There was a problem hiding this comment.
let's make the checking functions helpers so we don't have to update them everywhere, we already have Errors and error helpers in motor.go, the check for 0 rpm and 0 revolutions is now the same., so we shouldn't copy/past identical code.
There was a problem hiding this comment.
This comment was not addressed.
EDIT: I was not re-requested for review, you only committed, sorry! Was in a github PR review zone this morning.
martha-johnston
changed the title
viambot
added
safe to test
viambot
added
safe to test
viambot
removed
the
safe to test
viambot
added
the
safe to test
|
I added a new check for if the revolutions is 0, and I added a new helper for getting the direction to reduce repeated code. I think this covers a lot of the similar comments but I'll go through it again and let me know if there's more |
There was a problem hiding this comment.
kk still not the biggest fan of having math/codepaths within goForMaths that could potentially give invalid outputs, but it shouldn't be an issue that users hit.
maybe at a future date we could try unifying goForMath functions into a generic set of helpers? like adding a GoForMathDuration and GoForMathEncoded helpers, that way we won't need to change every function in drivers we support
I don't think it should be an issue because still the outputs aren't invalid, so if for some reason they are hit, it should just result in no movement
I agree that would be best, a lot have differences but I'm sure there's a way we could combine them. if that's something we're interested in seeing in this PR I can try and make it happen but it might just be a different ticket |
| @@ -1,15 +1,16 @@ | |||
| package dimensionengineering_test | |||
| package dimensionengineering | |||
There was a problem hiding this comment.
This is actually a convention for making sure your test file is in a separate package which forces you to write good tests of the functionality of your exported APIs from that package. If you don't need any of the private functions in the dimensionengineering package to be tested now, please change it back.
components/motor/motor.go
Outdated
| // GetDirection returns the direction based on the rpm and revolutions | ||
| func GetDirection(rpm, revolutions float64) float64 { | ||
| dir := 1.0 | ||
| if rpm*revolutions == 0.0 { | ||
| dir = 0.0 | ||
| } else if rpm*revolutions < 0.0 { | ||
| dir = -1.0 | ||
| } | ||
| return dir | ||
| } |
There was a problem hiding this comment.
Please rename this because GetDirection implies I'm getting the motor's direction as it is is moving, not getting the commanded Direction from the rpm and revolution requests.
viambot
added
safe to test
viambot
added
safe to test
viambot
added
safe to test
Removed revolutions == 0 functionality from all the motors.
Now I'm wondering if there should be a check that errors if you pass in 0 revolutions? As I was going through I checked there would be no divided by zero issues, and it seems fine (but let me know if I missed any). I don't feel strongly about adding an error for every GoFor but I see the benefit of it so let me know what you think.