[Feature] Improve drive base ability to make turns - drive_base.curve(), drive_base.drive()

[afarago]

Is your feature request related to a problem? Please describe.
Based on this year's FLL season experiences my team would suggest a few improvements on how the drivebase handles robot turns. This affects drive() and curve() methods.

Describe the solution you'd like

Merging radius and turn_rate

I would suggest an few optional (or mutually exclusive) parameter to add a radius to the drive method.

drivebase.drive(speed=100, turn_rate=10)
drivebase.drive(speed=100, radius=100)

Target condition angle, distance, time, stalled and reporting end criteria

Additionally would suggest extending / maybe even merging the functions with an optional distance #1157 parameter.

drivebase.drive(speed=100, radius=100, distance=100)
drivebase.curve(speed=100, radius=100, distance=100)
drivebase.drive(speed=100, radius=100, angle=100)
drivebase.curve(speed=100, radius=100, angle=100)

Probably for backwards compatibility it would be best to keep both functions with a same signature.

Additionally would suggest extending functions with an optional time parameter, to make drive_base similar to motor handling with motor.run_time() function. This is quite good in making an alignment to a model. (e.g. this FLL year we solve M07 Hologram Performer by curving and aligning to the model. Here we cannot use curve as the robot might get stuck and wait forever.)

drivebase.drive(speed=100, radius=100, time=1000)
drivebase.curve(speed=100, radius=100, time=1000)

Additionally would suggest extending a stall detection parameter.

drivebase.drive(speed=100, radius=100, until_stalled=True)

The end condition parameters should either be mutually exclusive or even better allow specifying multiple conditions that would each end/break the action.
This could extend the .done() to an enum returning how the movement ended instead of a boolean (function return value is MaybeAwaitable, therefore is not an option to use)

drivebase.drive(speed=100, radius=100, distance=100, until_stalled=True)
drivebase.drive(speed=100, radius=100, angle=100, until_stalled=True)

drivebase.drive(speed=100, radius=100, angle=90, time=1000, distance=100, until_stalled=True)
# drivebase.done() returns Done.STALLED --> movement was stopped being stalled
# drivebase.done() returns Done.TIME --> movement was stopped as 1000ms passed
# drivebase.done() returns Done.ANGLE --> movement was stopped as robot turned 90 degrees
# drivebase.done() returns Done.DISTANCE --> movement was stopped as robot moved 100 mms along the track

Default axle_track

Additionally would default the radius (when used) to a measure that is connected with axle_track. Here I do not have a suggestion for a right signature, yet leave this here for commenting.
The current mm is not obvious for kids.

The current behavior is like:

• spin turn (in place turn equivalent) (turn())
  radius = 0
• pivot turn (one motor turn equivalent) (as I recall WordBlock move turn steering=90)
  radius = drivebase_axle_track/2

Would suggest an alternative where these two options are easily accessible without the need of an external variable of repeating the axle_track value many times in the code. (DRY)
Possible solution would be an additional enum and loading radius with a composition of Number this enum types.

motorA = Motor(Port.A, Direction.COUNTERCLOCKWISE)
motorE = Motor(Port.E, Direction.CLOCKWISE)
drive_base = DriveBase(motorA, motorE, 56, 114)

drivebase.drive(speed: Number, radius: Number or Turn enum)
drivebase.drive(100, 0)
drivebase.drive(100, Turn.SPIN)
drivebase.drive(100, 114/2)
drivebase.drive(100, Turn.PIVOT)

Room for a good discussion.

Merged signature, naming

This would leave both functions with the same signature.
Additionally to ease comprehension I would name the parameter groups accordingly.

drivebase.drive(speed: Number, turn_radius: Union[Number, Turn]=0, turn_rate: Number=0, to_angle: Number=0, to_distance: Number=0, to_time: Number=0, to_stalled: bool=False, then: Stop=Stop.HOLD, wait: bool=True)

• turning option group with radius and turn_rate mutually exclusive and optional.
  Default zero results in moving straight.
• end conditions group with angle, distance, time and until_stalled optional as a group.
  Defaults mean no end condition.
• obviously then and wait only makes sense if any of the end conditions were used.
  Otherwise could yield to an error or be ignored

The above signature is a bit flawed as it includes many optional/and group set parameters and thus several aspects are not accessible with only positional arguments. Tuples would be an option, yet it is too complicated for kids. Room for a good discussion.

Describe alternatives you've considered
I have checked pervious FRs and currently we are using robot_base.drive(); wait(); robot_base.stop() for emulating the run_time() behavior (e.g. #1191, #1157).
My team currently does not use the .drive(turn_rate) as we cannot really translate the turn_rate to any metrics on the mat.
I would assume that stall detection would be an alternative option for the time parameter in alignments - team has not explored it yet.

Additional context
N/A

[laurensvalk]

Thanks for the suggestions! Meanwhile one quick input:

Here we cannot use curve as the robot might get stuck and wait forever.

See https://github.com/orgs/pybricks/discussions/1399 for a technique that lets you do this.

You could essentially limit the curve to a maximum time, to avoid getting stuck.

[afarago]

Thank you, this is genius!
Seems a bit too complex for the kids for now - robot_base.drive(); wait(); robot_base.stop() seems easier and more straightforward.

Additionally this would add also a great end option for the drive_base: run/drive/curve until stalled.

drivebase.drive(speed: Number, radius: Union[Number, Turn], turn_rate: Number, angle: Number, time: Number, until_stalled: bool, then: Stop=Stop.HOLD, wait: bool=True)

As an example:
drivebase.drive(100, Turn.PIVOT, until_stalled=True)

[laurensvalk]

The current behavior is like (...) spin turn (in place turn equivalent) (turn()) radius = drivebase_axle_track /4

Not sure I follow this, or why there would currently be a need to repeat the axle track at all?

A curve with a 0 radius is the same as an in-place turn, no matter the axle track value.

As a curiosity, if you occasionally want to drive with one wheel, how about left_motor.run_angle(...)?

motor.run_time()

This one has always bothered me a bit 😄 We have it because people seem to expect it and for backwards compatibility, but it was intentionally not added to Drive Base. It also hasn't been added to the block language so far for the same reason.

With time-based functions, some users intuitively assume that time x speed = distance. This isn't true since acceleration takes time.

Looking at it the other way, run_time is usually used as you describe, to reset against an endpoint. This was the only way to do it in the LEGO apps, so people got used to it. But we can detect stalling and slowdown much more accurately, so it seems like we shouldn't need such a "hack". For single motors, we introduced run_until_stalled for this purpose.

Perhaps there is also a drive base method that does what people actually want. Something like robot.align_with_wall(speed, timeout)...? This could also take care of temporarily turning off the gyro, since you don't want it to interfere with the reset.

[afarago]

Not sure I follow this, or why there would currently be a need to repeat the axle track at all?
As a curiosity, if you occasionally want to drive with one wheel, how about left_motor.run_angle(...)?

When working with kids 10-13 it is a great simplification if we can address all aspects of the robot with a single object and can forget motor_left and motor_right once we abstracted and defined the drive_base. (moving into object oriented approach for them -- when the robot of two motors needs to perform a navigation action - it is always your robot_base object and nothing else)
Pivot turn is quite a usual maneuver in robotics curriculum. That is currently axle_track/2, right?

A curve with a 0 radius is the same as an in-place turn, no matter the axle track value.

I made a mistake to use axle_track/4 for this. Corrected that in the FR.

Looking at it the other way, run_time is usually used as you describe, to reset against an endpoint. This was the only way to do it in the LEGO apps, so people got used to it. But we can detect stalling and slowdown much more accurately, so it seems like we shouldn't need such a "hack". For single motors, we introduced run_until_stalled for this purpose.

run_until_stalled is rather a safety fuse in several cases and at best is only an addition to another target such as time/distance/target angle -- see the updated FR above.

Perhaps there is also a drive base method that does what people actually want. Something like robot.align_with_wall(speed, timeout)...? This could also take care of temporarily turning off the gyro, since you don't want it to interfere with the reset.

Yes, the point of gyro usage in that case is a very good point. I am not sure what makes the more sense though.
I think alignment typically is a side aspect of the movement and generally needs to be considered as and end condition to keep teams from getting stuck while moving and aligning.
In my experience the movement is the key aspect for team (thus turn/straight aspect and target distance or angle).