move MoveXYZ classes out of motor.py into drive.py

[dwalton76]

There was some discussion about this in #478

We now have several classes in motor.py that are dealing with driving two wheeled robots:

• MoveTank
• MoveJoystick
• MoveSteering
• MoveDifferential

These could all be moved to a drive.py file to provide some cleaner organization of code. Do we also need to move the Speed classes?

While we are at it we should look into combining some of these classes. MoveTank and MoveSteering may be separate because EV3-G does it that way? MoveDifferential could be moved into MoveTank but certain methods would only work if the wheel object is created and the wheel_distance_mm specified.

MoveJoystick is unique enough we should probably leave it on its own.

[WasabiFan]

On the topic of combining classes...

I see two main problems with the architecture of our drive classes as it stands now.

1. It's hard/unnatural to compose actions of multiple different drive styles. If one drive action is more natural as a tank operation and the next one is better as steering, I must create two drive objects and switch between them. Similarly for the new differential drive class. This isn't a huge issue, but I certainly think it's worth considering.
2. It isn't obvious which parameters are which. Even with using our speed unit classes, something like this is just confusing: steering_drive.on_for_rotations(-20, SpeedPercent(75), 10) I have to look up the documentation every time to figure out which parameters are speeds, which are steerings, and which are distances.

I'm trying to figure out if there is a sane way to improve on this. With the existing library my best suggestion to solve the second point is to include the keyword argument names, but that is something one must manually choose to do and I doubt it's common.

Here are some of my thoughts...

I think it would make a lot of sense to try to combine these classes into a generic "drive" class. That would at least solve the first point above.

Thinking more outside the box:

What if we could provide something like this?

drive = RobotDrive(OUTPUT_A, OUTPUT_B)

drive.with_steering(25).for_seconds(10)
drive.with_tank(50, 75).for_degrees(90)

The goal here would be to introduce some natural language so that a) there's no ambiguity about what each value does and b) it's natural to compose different types of operations. Some points of interest:

• The way I imagined this working above would mandate that the "quantity" (time/distance/etc.) comes after the operation specifier. That could be flipped, or perhaps made so that both are setters and there's a final .run() or similar that must be called.
• This style could be extended. If we thought of "with_steering" and friends as setters on a chained operation object (the precise class design would need some thought) then you might be able to have additional setters to configure non-blocking operations and stop actions. I'm not sure how useful/clear that would be.
• It would be really nice to have differential drive in here too. Sadly, that requires additional configuration. Maybe those parameters could be optionally passed to the constructor of this shared object? Or have a setter?

I truly don't want to be disruptive unless we really are sure it is a major improvement -- at this point, I'm sure many people are using our existing interfaces. Nonetheless, an interface like this feels like a significant improvement to me. It models the same EV3-G blocks as before but treats it like a natural Python construct. This would require further discussion.

Some other difficulties:

• Are there any perf issues that would need to be considered here? Function calls and object instantiations aren't free on our low-power hardware.
• Would this be a detriment to documentability? Depending on the concrete implementation, this interface might be confusing when looking at a method reference.

[dwalton76]

drive = RobotDrive(OUTPUT_A, OUTPUT_B)

drive.with_steering(25).for_seconds(10)
drive.with_tank(50, 75).for_degrees(90)

Trying to wrap my head around this....with_steering(25) would create an instance of MoveSteering (each time it is called) and then that object would call for_seconds(). Creating that many objects would feel weird. What if the user wanted to create some variable in their MoveSteering object, it would then disappear :(

Thinking outloud here....types of driving robots:

• tank
• holonomic
• motorcycle?

I'm sure there are others we have not thought of. So lets says RobotDrive were a base class and the three types above were child classes of RobotDrive. For holonomic and motorcycle I think we would have to know the wheel size and distance between the wheels for those to work. Could we simplify the API if we required the same info for tank robots? Maybe we could then have a single method move():

def move(self, movement_type, speed, distance, radius=None)

movement types would depend on the child class

• forward in straight line
• backward in straight line
• left in straight line (holonomic only)
• right in straight line (holonomic only)
• rotate clockwise in place (tank, holonomic only)
• rotate counter clockwise in place (tank, holonomic only)
• forward in arc....this would be a rough equivalent to what is done in MoveSteering, MoveDifferential and MoveJoystick today. radius would be needed.
• backward in arc

move() could also be made private and we implement "move_forward()", "move_backward()", etc classes that call move().

For the part about requiring wheel size and wheel distance for tanks we could default to the EV3Tire for the wheel...would need to think some on what default to pick for wheel distance and what the impact would be for it being incorrect for some robots.

[WasabiFan]

with_steering(25) would create an instance of MoveSteering (each time it is called) and then that object would call for_seconds().

I think it would make more sense to restructure the MoveSteering code out of its current form. That being said, the first call in the chain would need to create some new operation object to represent the call.

What if the user wanted to create some variable in their MoveSteering object, it would then disappear :(

I don't think the user would ever interact with a MoveSteering object; this is an alternative API structure which I'm suggesting in an attempt to solve issues with the Move* classes.

Could we simplify the API if we required the same info for tank robots? Maybe we could then have a single method move():

What problems is this trying to solve? At the moment we have individual methods for the different drive actions and I think that's still the most clear way to do it. Otherwise, you hit a worse version of the same issue with unclear parameter meanings that we have now.

[dwalton76]

This might be one of those topics where it would be faster to do a video call one evening and figure out what (if anything) we want to change.

[bpmerkel]

I'd like to participate in the video call too... as a typical challenge to EV3 robots is a "jerk" motion on start and at stop--as motor torque and robot momentum can adversely affect accuracy and potentially trigger other problems. As a "teachable moment", I plan on showing FLL kids how to craft a "speed slope function" to pass in to a move function that speeds up/slows down gradually based on the wheel rotations... and "clamping" the speed such that they don't send too much power nor too little (which causes robot stalls). I've got a cosine-based function now for that perfect ramp-up/ramp-down curve over the span of a move (and a similar proportional slope for gyro-based pivots, so the robot slows its turn as it nears its target heading to compensate for the gyro lag--to catch up when its most important).

[dwalton76]

@WasabiFan any objections to closing this one? I don’t think it would be worth a breaking change to move this code around.

[WasabiFan]

The more in-depth API changes I mentioned are probably not going to happen. That being said, right now we have what I'd say is a structure issue:

• MoveSteering, MoveJoystick and MoveDifferentiall extend MoveTank
• MoveTank includes optionally-enabled color sensors, gyros (soon), etc. which are otherwise unnecessary in that class

Given time and design input, I would personally like to restructure those into something more composition-based. I don't think it's likely for that to happen soon but I think it's worth keeping the issue open for discussion.