[RSDK-8849] Change low rpm error to warning and limit zeroRpm tracker to make logs less noisy #4386

randhid · 2024-09-24T11:50:18Z

Make it a bit less alarming and opt to warn users that we're going a slow motor speeds.

Tell users them what rpm the motor is going at compared to what they requested.

Reset the zerorpm tracker so it doesn't continuously spam after the first time this condition is hit. cc @npentrel.

randhid · 2024-09-24T11:51:20Z

components/motor/gpio/encoded.go

 		} else {
 			currentRPM = deltaPos / deltaTime
 			if currentRPM == 0 {
 				zeroRPMTracker++
 			}
 		}
-		if zeroRPMTracker > 10 {
-			m.logger.Errorf("error running motor %v, desired rpm is too low for stable motion: %v", m.Name().Name, goalRPM)
+		if zeroRPMTracker > 20 {


Do we like 20 or should I make it 10 again?

I like 20 since we're increasing the tracker in another location now as well

martha-johnston · 2024-09-24T13:39:41Z

components/motor/gpio/encoded.go

-		if zeroRPMTracker > 10 {
-			m.logger.Errorf("error running motor %v, desired rpm is too low for stable motion: %v", m.Name().Name, goalRPM)
+		if zeroRPMTracker > 20 {
+			m.logger.Warnf("error running motor %v, current rpm is too low for stable motion: %v, asked to go at %v rpm",


[nit - don't change this if you don't feel the need to] I don't know how I feel about this log change. I think it's kind of confusing because if I was a user I would think, why is the motor not going the speed I requested? but the point is that the speed requested causes unstable motion, which could cause the current speed to be something unexpected. I guess I don't know if we need to expose what the unexpected current speed is to the user. Also this will just be the most recent currentRPM after 20 iterations of instability, which is just a single point of the overall movement, which I would expect to always be 0 anyways?

That's a good discussion point, if we're still going at near zero, and we're told the user their desired rpm is too low, which in the case naomi was seeing was calculated at 60 rpm from a base goign at 100 degspersecond for a 90 degree spin and she reported that it also happened when requesting more speed, then we've actually hit a motor's stall point or soemthing is topping it from moving and the calculated rpm is near zero because of a lack of position change. This could occur because of a bad jumper, or busted encoder line, or actually stalling against something. Then we are actually confusing the user more.

I do want to test the drive in a square script if there are cases where this is happening, but the motor not moving along with an error makes it seems like something is returning in an erroneous state. I will consider other options once I come in and look at this with the rover performing.

randhid · 2024-09-24T19:48:22Z

@martha-johnston let me know if you have any comments on the new error message, emoji react to this comment if not.

components/motor/gpio/encoded.go

randhid · 2024-09-24T21:36:53Z

components/motor/gpio/encoded.go

-		if zeroRPMTracker > 10 {
-			m.logger.Errorf("error running motor %v, desired rpm is too low for stable motion: %v", m.Name().Name, goalRPM)
+		if zeroRPMTracker > 20 {
+			m.logger.Warnf("error running motor %v, current rpm is too low for stable motion: %v, asked to go at %v rpm",


Suggested change

m.logger.Warnf("error running motor %v, current rpm is too low for stable motion: %v, asked to go at %v rpm",

m.logger.Warnf(

"error running motor %v, current rpm is too low for stable motion: %v, asked to go at %v rpm",

components/motor/gpio/encoded.go

Change to warning and limit zeroRpm tracker to make it less noisy

391c508

randhid requested review from JohnN193 and martha-johnston September 24, 2024 11:50

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Sep 24, 2024

Fix typo

ef50ba5