motorutil: add steering block equivalent #50

kortschak · 2017-09-16T23:37:52Z

@dlech Please take a look.

It has never been entirely clear to me the behaviour of the steering block at the ends of the steering range for EV-G, so this is a fudge. If you have a better suggestion I'll be happy to include it.

dlech · 2017-09-17T03:42:06Z

motorutil/steer.go

@@ -0,0 +1,276 @@
+// Copyright ©2016 The ev3go Authors. All rights reserved.


it's 2017 now 😉

Yes, but the code it ripped from the znap example, which was written last year.

dlech · 2017-09-17T03:47:47Z

motorutil/steer.go

+// specified speed. The valid range of direction is -1 (hard left) to +1 (hard right).
+// For directions between -1 and 1 neither motor will run backwards, but for -1 and 1
+// the motors will run in opposite directions to allow a turn on the spot in the given
+// direction.


In the EV3-G software, the range is -100 to 100 for steering. For these values, the robot will basically turn in place (left runs forward and right runs backwards at same speed).

I think this is what you are getting at, but saying "neither motor will run backwards" doesn't sound right.

Another thing, the EV3 does not have a floating point unit, so anything that uses floats will be less efficient. I don't know that there would be a significant (noticeable) difference here. It is just something to consider. I generally try to avoid float just on principal.

Another more useful thing to think about is one feature of the official LEGO firmware that is frequently requested in ev3dev is that motors are "synchronized", meaning that if one has a higher load than the other, the faster one will wait for the other to catch up, so to speak. I think the leJOS guys have a blog about this for an arbitrary number of motors. LEGO does this in the kernel drivers for 2 motors, but ev3dev doesn't to it at all, so it would have to be implemented in Go.

I think this is what you are getting at, but saying "neither motor will run backwards" doesn't sound right.

When I've experimented, there is forward progression except with very tight turns, where you see a complete spin. The comment about no reverse motor for the other cases is true (at least for this code).

Another thing, the EV3 does not have a floating point unit, so anything that uses floats will be less efficient. I don't know that there would be a significant (noticeable) difference here. It is just something to consider. I generally try to avoid float just on principal.

I don't think there is a huge issue here since it's only a very minor part of the code path.

I'll look into synchronising the motors, but it's not a juge priority.

From looking at the leJOS article, it's not clear that I'd be able to do very much better than what is there now without significant added complexity and running the motors with run-direct. Fortunately, because the Go code is natively compiled there is no JIT warmup.

I have changed direction to take an int argument; it simplifies the API and removes the need for a new error interface type.

You're right about the turns; I can't read my own code which is upsetting. I have clarified the docs and added tests to demonstrate the expected behaviour.

This is useful for future reference.

dlech

I would like to take this for a test drive to really get a feel for the behaviors here. But I would like to do it when I am a bit more awake (in the next day or two), but here are a couple other suggestions for now.

dlech · 2017-09-17T22:37:18Z

motorutil/steer.go

+	"github.com/ev3go/ev3dev"
+)
+
+// Steering implements a paired-motor steering unit similar to an EV-G steering block.


EV-G > EV3-G

dlech · 2017-09-17T22:39:57Z

motorutil/steer.go

+	Timeout time.Duration
+}
+
+// SteerCounts steers in the given turn for the given tacho counts and at the


I would rather see degrees or rotations here. Based on feedback from users, I have come to the conclusion that most people have a hard time grasping the concept of "tacho counts". It also makes the code more portable (On BrickPi, the hardware actually uses 720 counts per rotation, but we tweaked the driver to match the 360 counts on EV3 because people just don't understand.)

It can't be rotations unless I use a float64 since otherwise it precludes part rotation. I can add a comment in the documentation that the counts/rot can be obtained with a call to t.CountsPerRot for each motor, but I'd rather not put those calls into the SteerCounts method since the value can be cached by the user after an initial call (and as you say counts ≅ degrees).

It would be nicer if this library cached the CountsPerRot value and did the conversion so that the user doesn't have to.

That was a conscious design decision; none of the devices in ev3go save state that is held by the underlying driver since it may become invalid.

Ack. But, address, commands, count_per_rot, count_per_m, full_travel_count, driver_name, max_speed and stop_actions are fixed values and will never change, so they should be safe to read once and cache.

Yes, I think you might be right. The only case where those would be invalidated is if the motor is unplugged and replugged, but then the ID is also invalid, so all bets are off.

This is #53.

dlech · 2017-09-17T22:52:19Z

motorutil/steer.go

+
+// SteerCounts steers in the given turn for the given tacho counts and at the
+// specified speed. The valid range of turn is -100 (hard left) to +100 (hard right).
+// If counts is negative, the turn will be made in reverse. The sign of speed is


Ignoring the sign of speed is another thing that, based on user feedback, is non-intuitive.

Here is an example of something I've been playing around with lately that shows how you can implement this to hide the driver implementation details from the user and make things more intuitive. It is just for a single motor instead of steering, but is similar. The function converts rotations to tacho counts and handles negative speed values.

Ignoring the sign of speed is another thing that, based on user feedback, is non-intuitive.

This is merely following the underlying ev3dev speed_sp behaviour. If that changes, I'll update this.

I was thinking of taking that approach, but the documentation becomes more complex in that then it would need to say the direction of travel is the sign of the product of counts and speed.

The docs may be more complex, but users expect negative speeds to "just work" before ever consulting the documentation (at least in my experience).

What is the expected behaviour of -speed and -position in conjunction? This is the problem for me.

Just like multiplication in algerbra, two negative make a positive. Also, it works this way in EV3-G.

does the exact same thing as

Both drive forwards and then backwards

OK, it's a deal. I'll make that change tonight.

dlech · 2017-09-17T22:56:40Z

motorutil/steer.go

+}
+
+// Wait waits for the last steering operation to complete. A non-nil error will either
+// implements the Cause method, which may be used to determine the underlying cause, or


implements > implement

kortschak · 2017-09-17T23:29:57Z

Very happy to wait. I'll have those comments addressed by then hopefully.

dlech · 2017-09-18T18:52:05Z

drive := motorutil.Steering{Left: lMotor, Right: rMotor, Timeout: -1}
drive.SteerCounts(400, -50, -360)
drive.Wait()

Never returns from Wait().

By making -50 or -360 positive, it works as expected.

kortschak · 2017-09-18T20:37:58Z

The arithmetic works as I expect for those:

        {
                speed: 400, turn: 50, counts: -360,
                wantLeftSpeed: 400, wantLeftCounts: -360,
                wantRightSpeed: 0, wantRightCounts: 0,
        },
        {
                speed: 400, turn: -50, counts: -360,
                wantLeftSpeed: 0, wantLeftCounts: 0,
                wantRightSpeed: 400, wantRightCounts: -360,
        },
        {
                speed: 400, turn: -50, counts: 360,
                wantLeftSpeed: 0, wantLeftCounts: 0,
                wantRightSpeed: 400, wantRightCounts: 360,
        },

I can't set up a robot to look at this for the next several days, so does the drive.SteerCounts return an error, and if you set a long timout (say 30s), what is the returned error from Wait. (Always check errors).

Note that errors are not sticky here, which is a deviation from the ev3go/ev3dev API to normal idiomatic Go, but one that is necessary for allowing exposure of the motors in the struct, which allows greater control. I need to think about this further.

dlech · 2017-09-18T21:53:07Z

I am checking errors. I just left them out for brevity.

Here is the error after setting a 5 second timeout

2017/09/18 21:48:16 motorutil: multiple errors: ["motorutil: failed to wait for left motor (motor0) to stop (state=running|stalled): motorutil: wait timed out: longer than 5s" "motorutil: failed to wait for right motor (motor1) to stop (state=running|stalled): motorutil: wait timed out: longer than 5s"]

I suppose this could be a driver bug. If we are running the motor a 0 speed (or something close to it), it probably says running and not stalled, but if the position setpoint is not also 0, then we never reach the position. At steering = 50, right motor does not turn.

kortschak · 2017-09-18T22:02:51Z

Thanks. I guess I can not write if speed or counts is zero, but it seems to me that a zero write should not show up as a stall. I'm adding sticky errors now - at least a first draft, since it changes the Causer behaviour in a way that I need to sort out, but I'm heading to work (this will either be worse documentation or adding a steering error).

kortschak · 2017-09-18T22:22:03Z

I suppose this could be a driver bug. If we are running the motor a 0 speed (or something close to it), it probably says running and not stalled, but if the position setpoint is not also 0, then we never reach the position. At steering = 50, right motor does not turn.

I'm confused by this. When the turn results in a non-turning motor, the relative position setpoint is also zero. This does seem like a driver bug.

If you do

echo 0 > /sys/class/tacho-motor/motor0/speed_sp
echo 0 > /sys/class/tacho-motor/motor0/position_sp
echo run-to-rel-pos > /sys/class/tacho-motor/motor0/command

what does cat /sys/class/tacho-motor/motor0/state print?

dlech · 2017-09-18T22:22:26Z

I found a bug that hides this problem for the case of positive 50 steering. Will comment on the relevant line.

dlech · 2017-09-18T22:23:29Z

motorutil/steer.go

+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			stat, ok, err := ev3dev.Wait(s.Left, ev3dev.Running, 0, 0, false, s.Timeout)


s.Left should be device.

Thanks. That clears up that bit of confusion.

Fixed.

dlech · 2017-09-18T22:25:58Z

what does cat /sys/class/tacho-motor/motor0/state print?

Same as the error message:

robot@explorer:/sys/class/tacho-motor/motor0$ cat position_sp
0
robot@explorer:/sys/class/tacho-motor/motor0$ cat speed_sp
0
robot@explorer:/sys/class/tacho-motor/motor0$ cat state

robot@explorer:/sys/class/tacho-motor/motor0$ echo "run-to-rel-pos" > command
robot@explorer:/sys/class/tacho-motor/motor0$ cat state
running stalled
robot@explorer:/sys/class/tacho-motor/motor0$

kortschak · 2017-09-18T22:32:04Z

Cool, that explains it, but it looks to me like surprising behaviour from the driver. I can paper over it here by doing a stop if the motor speed is 0, but it's a bit gross.

dlech · 2017-09-18T22:35:13Z

Although this could be considered a driver bug, I don't think it would be good to rely on driver behavior here. We basically have a divide by zero error here. Not literally, but if you think about it, we are trying to move 0 distance at 0 speed, so we will get there in 0/0 seconds. We are requesting something that is physically undefined. So there isn't really a "right" thing for the driver to do.

It would be simple enough to add a check here to say that if we have these conditions, then just send the stop command instead of run-to-rel-pos. We can try to do that in the motor drivers, but it is complicated by the fact that we are trying to be "smart" in the drivers and the the relative position setpoint is cumulative, meaning that it is relative to the previous relative setpoint. This way, if we overshoot by a degee or two, the error does not accumulate with consecutive calls to run-to-rel-pos.

dlech · 2017-09-18T22:39:13Z

Likewise, if you request a non-zero position setpoint, but a zero speed setpoint and expect the motor to move, that is a programming error. The status of running stalled is the expected output here because we can't turn the motor at zero speed.

kortschak · 2017-09-18T22:40:45Z

Yes, that's true, but if a run-to-rel-pos is received and position_sp is 0, the driver should just return without checking the speed_sp. This seems a little bit Achillean to me. If the programmer has sent a non-zero position_sp, then I agree that this is the correct behaviour, but the function discontinuity is unhelpful in this specific (0, 0) case.

dlech · 2017-09-18T22:42:53Z

Ack. I'll see what I can do in the driver. It's really probably pretty simple to fix, now that I have thought about it some more.

kortschak · 2017-09-18T22:55:37Z

I've added the zero relative position conditional stop for the SteerCounts method.

dlech · 2017-09-18T23:08:11Z

Now that that is sorted, back to general comments...

After having played around with this a bit, I actually don't mind the error handling the way it is now. I just wrote a must function that wraps each line so that my program aborts on any error.

And the steering value works just like the EV3-G software, so it is easy to get the hang of.

dlech · 2017-09-18T23:31:42Z

And a more general comment on ev3dev libraries in general...

I would really like for ev3dev libraries to hide all of the driver implementation details from users. Why?

As it stands now, literally all ev3dev language libraries (that I know of) are basically thin wrappers around the hardware drivers. This means if we need to make breaking changes to improve the drivers or get them into the mainline Linux kernel, then it breaks literally every ev3dev user program ever written. If the libraries don't pass through the driver implementation details to the user, then we only have to fix the libraries, and everyone's programs keep working after a breaking kernel change.
The driver implementation (motors in particular) is not very nice for new users. I have had way more conversations on IRC and in email than I care too explaining to people who just want to make a motor turn how to do it and can't figure it out because it takes multiple steps and it is not obvious which setpoints affect which commands.

This is why I am hoping that you will take my suggestions here of using rotations and/or degrees here instead of "counts" and also allow for a negative speed parameter even though it works differently in the hardware drivers.

Sure, people can figure these things by reading the documentation, but programming is way more fun when things just work as expected without having to read the docs at all. And I wouldn't be making a big deal out of it, but these two things in particular have come up repeatedly over the last 4 years. If my recollections can be trusted, I can safely say that users, in general, expect negative speed values to work and they expect motor rotations in degrees.

dlech · 2017-09-18T23:42:30Z

One other thing that would be convenient and help future-proof would be to add a parameter to SteerCounts and SteerDuration for the stop action.

kortschak · 2017-09-18T23:51:59Z

After having played around with this a bit, I actually don't mind the error handling the way it is now.

While the original error handling was my natural first approach (the Go-idiom) it differed from the rest of the device methods, and so would have been surprising to users of those types. Are you OK with the new error-stateful error handling?

kortschak · 2017-09-18T23:52:43Z

I'll handle to comments in #50 (comment) and #50 (comment) later today if I get some free time.

dlech · 2017-09-19T00:15:20Z

Are you OK with the new error-stateful error handling?

Yes.

If I'm understanding correctly, I think it means that I will be able to write (sans error handling)

drive := motorutil.Steering{Left: lMotor, Right: rMotor, Timeout: -1}
drive.SteerCounts(400, -50, -360).Wait()

which is nice.

And I can still use the must() idiom, by just changing the parameter type.

kortschak · 2017-09-19T00:19:42Z

drive.SteerCounts(400, -50, -360).Wait() with error handling </grin>.

kortschak · 2017-09-29T09:00:23Z

Is there anything further that you would like done here?

dlech · 2017-09-29T16:00:43Z

did you change the speed to allow a negative value?

kortschak · 2017-09-29T21:33:31Z

Sorry, I have now, thank you for reminding me.

PTAL

BTW Is there any plan to change speed_sp behaviour to include sign consideration when executing run-to-*-pos on the basis of what you've said here?

dlech · 2017-09-29T21:37:00Z

I don't like to make breaking changes to the drivers at this point, but for this case, yes it is tempting to make the change in the driver...

dlech · 2017-09-29T21:48:48Z

You are too fast. 😄

Here is my new test program. It works nicely.

package main

import (
	"log"
	"time"

	"github.com/ev3go/ev3dev"
	"github.com/ev3go/ev3dev/motorutil"
)

func main() {
	lMotor, err := ev3dev.TachoMotorFor("ev3-ports:outB", "lego-ev3-l-motor")
	if err != nil {
		log.Fatalln("Missing large motor on output port B")
	}
	rMotor, err := ev3dev.TachoMotorFor("ev3-ports:outC", "lego-ev3-l-motor")
	if err != nil {
		log.Fatalln("Missing large motor on output port C")
	}

	steer := motorutil.Steering{Left: lMotor, Right: rMotor, Timeout: 5 * time.Second}
	if steer.Err() != nil {
		log.Fatalln(steer.Err())
	}

	must(lMotor.SetStopAction("hold").Err())
	must(rMotor.SetStopAction("hold").Err())

	must(steer.SteerCounts(400, 0, 360).Wait())
	must(steer.SteerCounts(-400, 0, 360).Wait())

	must(steer.SteerCounts(400, 100, 360).Wait())
	must(steer.SteerCounts(-400, 100, 360).Wait())

	must(steer.SteerCounts(400, -50, 360).Wait())
	must(steer.SteerCounts(-400, -50, 360).Wait())
}

func must(err error) {
	if err != nil {
		log.Fatalln(err)
	}
}

One thing I was going to suggest was to have a parameter to allow setting the stop action for both motors.

Either that, or just make "hold" the default when you initialize a TachoMotor since that is what I tend to use 90% of the time anyway.

kortschak · 2017-09-29T22:04:09Z

Sorry.

One thing I was going to suggest was to have a parameter to allow setting the stop action for both motors.

This is part of the design which flows from Go philosophy. The motors are exposed so that they are obviously reconfigurable while held by the Steerer. I'd prefer to keep composability over adding configuration options.

kortschak · 2017-09-29T22:12:24Z

Though, I do see merit in doing that. I'll add a method StopActions (string, error), StopActions() ([]string, error) and SetStopAction(action string) *Steerer. StopAction and StopActions will error if the returned values do not match. #57

kortschak force-pushed the steering branch from 6168239 to cd9ea8a Compare September 17, 2017 01:48

dlech reviewed Sep 17, 2017

View reviewed changes

kortschak force-pushed the steering branch 4 times, most recently from 31ab05e to 88a02bc Compare September 17, 2017 21:42

dlech reviewed Sep 17, 2017

View reviewed changes

dlech mentioned this pull request Sep 17, 2017

ev3dev-stretch #52

Open

kortschak force-pushed the steering branch from 88a02bc to cee6939 Compare September 18, 2017 08:30

motorutil: add steering block equivalent

c3eff83

kortschak force-pushed the steering branch from cee6939 to c3eff83 Compare September 18, 2017 08:37

motorutil: add sticky errors and call chaining to Steering

1accf7d

dlech reviewed Sep 18, 2017

View reviewed changes

motorutil: fix device for wait cycle

9616c8c

dlech mentioned this pull request Sep 18, 2017

tacho motor run-to-rel-pos has status of running stalled when position setpoint and speed setpoint are both zero ev3dev/ev3dev#963

Closed

kortschak force-pushed the steering branch from 98ffac9 to 50b1ad2 Compare September 18, 2017 22:58

motorutil: add conditional stop command for zero relative position case

8b65272

kortschak force-pushed the steering branch from 50b1ad2 to 8b65272 Compare September 18, 2017 23:00

kortschak mentioned this pull request Sep 19, 2017

ev3dev: cache invariant values for motors #53

Closed

8 tasks

motorutil: make speed a vector for count-based steering

9a52f10

kortschak merged commit 817fce6 into master Sep 29, 2017

kortschak mentioned this pull request Sep 29, 2017

motorutil: add stop action handling to Steerer #57

Closed

kortschak deleted the steering branch September 30, 2017 13:58

		@@ -0,0 +1,276 @@
		// Copyright ©2016 The ev3go Authors. All rights reserved.

motorutil: add steering block equivalent #50

motorutil: add steering block equivalent #50

Conversation

kortschak commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortschak Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortschak Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlech Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlech Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortschak commented Sep 17, 2017

dlech commented Sep 18, 2017

kortschak commented Sep 18, 2017

dlech commented Sep 18, 2017

kortschak commented Sep 18, 2017

kortschak commented Sep 18, 2017

dlech commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlech commented Sep 18, 2017

kortschak commented Sep 18, 2017

dlech commented Sep 18, 2017

dlech commented Sep 18, 2017

kortschak commented Sep 18, 2017

dlech commented Sep 18, 2017

kortschak commented Sep 18, 2017 • edited Loading

dlech commented Sep 18, 2017

dlech commented Sep 18, 2017

dlech commented Sep 18, 2017

kortschak commented Sep 18, 2017

kortschak commented Sep 18, 2017

dlech commented Sep 19, 2017

kortschak commented Sep 19, 2017

kortschak commented Sep 29, 2017

dlech commented Sep 29, 2017

kortschak commented Sep 29, 2017

dlech commented Sep 29, 2017

dlech commented Sep 29, 2017 • edited Loading

kortschak commented Sep 29, 2017

kortschak commented Sep 29, 2017

kortschak Sep 17, 2017 •

edited

Loading

kortschak Sep 18, 2017 •

edited

Loading

dlech Sep 19, 2017 •

edited

Loading

dlech Sep 17, 2017 •

edited

Loading

kortschak commented Sep 18, 2017 •

edited

Loading

dlech commented Sep 29, 2017 •

edited

Loading