Rsdk 1020 - Investigate motor_encoder issues Part I: Initial refactor of RPM Monitor and Wheeled base. #2211
Conversation
Tried on the Orin on my desk, using the PWM to blink an LED: works great at lots of duty cycles and low frequencies (up to 5 Hz). When using it to drive a DC motor, things work fine at relatively high motor speeds (e.g., >50% power), but it stutters a lot at low speeds. Similarly, using it to drive a servo motor results in lots of shifting, wiggling movements. Using an oscilloscope, the waveform looks okay for frequencies up to about 50 Hz, but beyond that things become noisy, and by the time you hit 500 Hz, this is garbage. We'll aim to nail hardware PWM, and then you won't need to rely on this any more.
….go for Macs (viamrobotics#1967) Sorry about this! The non-Periph genericlinux GPIO pins use Linux-specific functions, so we use build tags to have a separate file that gets substituted in for non-Linux OSes. I changed the function signature on the "real" one, forgot to change the non-Linux one to match, and didn't notice the problem because neither I nor Github's CI has a non-Linux environment to test on.
…amrobotics#1959) The http -> grpc migration is long done now and those code paths are no longer needed.
- multiple gofor calls - overshooting - "jerk" with lots torque
…rvice from main (not remote) (viamrobotics#1872)
Co-authored-by: jeremyrhyde <34897732+jeremyrhyde@users.noreply.github.com>
…viamrobotics#2187) Co-authored-by: Nick Sanford <nick.sanford@viam.com> Co-authored-by: James Otting <james@viam.com>
viambot
added
safe to test
| @@ -20,7 +20,7 @@ import ( | |||
| "go.viam.com/rdk/config" | |||
| "go.viam.com/rdk/control" | |||
| "go.viam.com/rdk/operation" | |||
| rutils "go.viam.com/rdk/utils" | |||
| rdkutils "go.viam.com/rdk/utils" | |||
| @@ -156,6 +147,10 @@ func newEncodedMotor( | |||
| em.maxPowerPct = 1.0 | |||
| } | |||
|
|
|||
| if em.ticksPerRotation <= 0 { | |||
| em.ticksPerRotation = 1 | |||
There was a problem hiding this comment.
Suppose someone puts their encoder on backwards, so that the ticks per rotation honestly is negative. Would that be a problem? I wonder if that could cause trouble with rpmMonitor or something, where you spin the motor until the encoder shows the right value, but the encoder keeps going in the wrong direction because ticksPerRotation got set to 1 here instead of -100 or something.
I get setting it to some default if it's 0, but if it's negative, I kinda want to leave the value alone.
There was a problem hiding this comment.
hmmmm, I don't even want to think of the wild math that will result in rpm monitor with negative ticks per rotation. Also I have no clue what to set that default to in that case. WIll remove.
There was a problem hiding this comment.
actually there is a check and a default already built in, I changed the location to make it clearer in configuration.
| } | ||
| // We've been putting power to the motor, but it's not moving yet. Try increasing the power | ||
| // to it, and we'll start moving soon. | ||
| return m.computeRamp(lastPowerPct, lastPowerPct*2) | ||
| } | ||
| dOverC := desiredRPM / currentRPM * float64(m.flip) | ||
| dOverC := desiredRPM / currentRPM |
There was a problem hiding this comment.
This is not just refactoring, and isn't mentioned in the PR description. Why should we not multiply by m.flip here? I can't tell if this fixes an old bug or introduces a new one.
| @@ -644,6 +630,13 @@ func (m *EncodedMotor) GoTo(ctx context.Context, rpm, targetPosition float64, ex | |||
| } | |||
| moveDistance := targetPosition - curPos | |||
|
|
|||
| // don't want to move if we're already at target, and want to skip GoFor's 0 rpm | |||
| // move forever condition | |||
There was a problem hiding this comment.
I had to read this multiple times before I understood what it meant. Part of my trouble is that the subject of the sentence ("we") isn't present and must be inferred, and part of it is that the first half makes more sense if you're already thinking about the second half. (and then part of it is that personally I find it harder to read multi-line sentences that don't start with a capital letter or end with a period, because there might be multiple sentences in the middle that I haven't segmented out correctly.) I'd rephrase to something more like "If you call GoFor with 0 revolutions, the motor will spin forever. If we are at the target, we must avoid this by not calling GoFor."
Code is read much more often than it is written, so extra effort writing it in a way that makes it easy to read is a good trade-off.
components/motor/server.go
Outdated
| rVal := 0.0 | ||
| revolutions := req.GetRevolutions() | ||
| if revolutions != 0 { | ||
| rVal = revolutions |
There was a problem hiding this comment.
Heh, I'm amused. Very good call noticing this is a longwinded no-op!
viambot
added
safe to test
|
Holding off on merging this by request, but please add reviews as I add commits. |
viambot
added
safe to test
viambot
added
safe to test
Trying to push up the state variables that change to earlier functions, so that I can more effectively lock and unlock them in trying to fix the underlying race conditions which I cannot right now. If you see something that I can change in that respect, please suggest it.