pybricks / support

Pybricks support and general discussion
MIT License
109 stars 7 forks source link

[Feature] Allow distance parameter in DriveBase.curve() #1157

Open laurensvalk opened 1 year ago

laurensvalk commented 1 year ago

(...)

Originally posted by @FLL-Team-24277 in https://github.com/pybricks/support/discussions/1156#discussioncomment-6417252

This feature is a generalization of a specific feature requested above.

Given a radius, an arc or curve can be described using either the length or the angle.

We currently allow only an angle, but a length (distance) makes more sense in some applications. This includes driving a given length with a slight curve.

afarago commented 9 months ago

My FLL team definitely +1's this as this came up several times during this year's competition.

FLL-Team-24277 commented 2 months ago

Not sure if anyone has started work on this, but we noticed one other problem with using curve() right now. The speed and acceleration for curve are set using settings(). We think of curve as an extension of straight, so we think the speed and acceleration should be set with settings(straight_speed = s, straight_acceleration = a), where speed is expressed in mm/s and acceleration is mm/s^2. Instead it seems that curve uses deg/s for speed and deg/s^2 for acceleration. Since deg/s is dependent on both motor speed and radius of the curve, this is rather counter-intuitive, especially for middle school kids. We'd really like to see an option where we can set the speed with straight_speed and acceleration with straight_acceleration.

We do think deg/s and deg/s^2 make sense for turn, which is obviously a turn-in-place, so thinking in terms of degrees makes sense.

laurensvalk commented 2 months ago

We think of curve as an extension of straight (...) We do think deg/s and deg/s^2 make sense for turn

As a thought experiment, what about a curve with a small radius, approaching zero?

Instead it seems that curve uses (...)

It uses all of the settings. Basically, it works out what the path would be for the straight distance, and for the in-place turn. Then it works out which of those two takes the longest, and then stretches out the other movement so that everything completes at the same time.

So for a shallow curve, I think you'll find that it will use the settings for straight, as you were hoping.

I plan to be looking at drive bases again in the relatively near future, so I do hope to get started on the request discussed above as well.

FLL-Team-24277 commented 2 months ago

Wow, I didn't think of that. I did some testing:

from pybricks.pupdevices import Motor
from pybricks.parameters import (
    Port,
    Direction,
)
from pybricks.robotics import DriveBase

TIRE_DIAMETER = 56  # mm
AXLE_TRACK = 103  # distance between the wheels, mm

leftDriveMotor = Motor(Port.E, Direction.COUNTERCLOCKWISE)
rightDriveMotor = Motor(Port.A)
robot = DriveBase(
    leftDriveMotor,
    rightDriveMotor,
    TIRE_DIAMETER,
    AXLE_TRACK,
)

robot.use_gyro(True)
robot.settings(
    straight_acceleration=150, straight_speed= 300,
    turn_acceleration=150, turn_rate=360
) 
robot.curve(radius=500, angle=90)

Here, with the radius = 500, the two straight parameters control the speed. If I reduce the radius to 5, the two turn parameters control the speed.

Without a doubt, curve() is a strange animal! I see your point though. At some point, it makes sense to use degrees rather than mm. Perhaps if the radius is < track use turn, otherwise use straight???

We will need to put some thought into how we implement it in our BaseRobot() class.

laurensvalk commented 2 months ago

You have a good eye for detail :) For most use cases, I think this does seem like the most intuitive solution.

Note that it doesn't quite arbitrarily use one setting or the other. Both are used. But depending on the curve you drive, one isn't the limiting factor, so it isn't so apparent. If you veer only slightly, the angular acceleration and angular velocity are almost negligible, so they never hit the values/limits that you specified in the settings.

laurensvalk commented 2 months ago

Given a radius, an arc or curve can be described using either the length or the angle.

We currently allow only an angle, but a length (distance) makes more sense in some applications. This includes driving a given length with a slight curve.

Can @FLL-Team-24277 and @afarago please describe the types of moves where this will be most useful?

One application is to veer slightly off a straight line, so with a large radius. If this the main one, then radius + length is probably still not the most useful parametrization. In this case, curvature + length may be more suited?

I am currently working through as many drive base issues and suggestions as I can, so now is a good time to share your wish list :)

FLL-Team-24277 commented 2 months ago

We use curve() to navigate around the FLL table. We almost always use curve with wait=False to keep the robot moving quickly.

Right now we have gotten pretty comfortable using radius and angle, but we are getting quite confused with speed. Maybe it will help that I would estimate that we almost always use a curve with a radius at least equal to the wheelbase dimension (that is, over 103mm). I can't ever remember a time using curve with a diameter less than that. We really think of curve() as an extension of straight(), so we expect the speed to be handled the same as straight(), and we expect that if we pass the same speed parameter to curve() and straight() that it would appear that the robot is going the same speed for both maneuvers. We always use gyro=True for our curve maneuvers. We would also like to be able to use mm as the distance parameter, perhaps measured at the center of the robot?

We have one other use case for curve, and that is wall following. For example, if the right side of the robot was placed on a wall surface, we would use curve with gyro=False and a very large diameter to keep the robot against the wall. We used this once last year and I expect that we might use it once this year. Hard to say since the season is just starting. This feels messy and I can't help but think there should be a better way.

FLL-Team-24277 commented 2 months ago

By the way, if you thought it could help, we could schedule a zoom meeting and we could show you our robot in action and how we use curve().

laurensvalk commented 2 months ago

Thanks for the additional details!


Indeed, allowing the curve to be specified by either angle or distance makes sense, as in the first post here.


We have one other use case for curve, and that is wall following.

We could introduce a dedicated method like veer(distance, turn_rate) that works like straight, but with a small turn_rate. Or, you can do it with existing code:

robot.use_gyro(False)
robot.reset(distance=0)
robot.drive(speed, turn_rate)
while robot.distance() < 500:
    wait(10)
robot.stop()
robot.use_gyro(True)

Since we're talking about driving along a wall, this slightly more manual approach can be quite helpful, since you can check things like "getting stuck" in the loop, rather than abstracting it all away.


By the way, if you thought it could help, we could schedule a zoom meeting and we could show you our robot in action and how we use curve().

Great idea. I'd like to release a beta version in the coming weeks with all existing fixes to have a consistent baseline. Maybe we can evaluate from there. Maybe @afarago wants to join too?

laurensvalk commented 2 months ago

The parameters for curve are potentially a bit confusing:

Right now, the curve is such that a radius=0 gives the identical behavior as turn, which makes sense. So the sign of angle determines the turn direction. Therefore, the radius is used to alter the drive direction.

But for curve(radius, distance) this would be confusing. It would be confusing to let radius determine the drive direction and distance the turn direction. The opposite makes more sense.

But that would mean introducing a different handling of radius, or changing the existing behavior (which we can't really do).


Other possibilities: Introduce a new method called arc to handle both, and deprecate curve but keep it available unchanged so existing code continues to work.

This gives us a bit more freedom to define it consistently and practically. Since we don't have to use the same convention used by turn in this case, we can explicitly disallow very small radii, so I think we can use David's interpretation.

A new method will also allow us to require a keyword argument to specifically choose angle or distance.

We also have the opportunity to change which speed parameters get used as requested by @FLL-Team-24277 .

All in all, a new method does seem to make sense. I'll create a experimental implementation for review.

Proposal:

image

FLL-Team-24277 commented 2 months ago

All, we are happy to be helping with this! The proposed arc() seems to be exactly what we are looking for.

How will the speed be set for this? Our expectation would be that the speed (mm/s) would be expressed the same as straight(), and the actual speed should match as well.

laurensvalk commented 2 months ago

Could you please give some examples where the current speed settings give unexpected or unintuitive results for the curve() method? I'd like to be sure that we're looking at the same thing, and possibly rule out technical issues.

Ultimately, a curve and arc involve both forward and turn speed. Depending on the radius, one will take the upper hand because of physics. It's also worth noting that in a curve the outer wheel has to travel quite a bit faster to keep your straight speed. This may sometimes give the appearance of driving fast, when it's mostly the same.

FLL-Team-24277 commented 2 months ago

I don't have an example on hand right now, but I will ask my team members this afternoon and get back to you.

If I recall correctly, the problem we were seeing was that the robot was going slower in the curve than we expected.

laurensvalk commented 2 months ago

Everyone: You can now try the new arc method using these instructions to try the latest version

FLL-Team-24277 commented 2 months ago

I have arc() working using the nightly instructions you posted. Initial test was satisfactory. Just a simple robot.arc(radius=600, distance=500)

I'll let the kids give it a good test this afternoon.

We use VS code, so we need to update the pybricks import but I don't remember how to do that. We need that for the code completion. pip install pybricks just says it's already up to date.

Version currently shows ('primehub', '3.6.0b1', 'ci-build-3565-v3.6.0b1-1-ga137834e on 2024-09-24')

laurensvalk commented 2 months ago

Thank you! Right, auto-code completion for this brand new feature is not released yet :smile:

FLL-Team-24277 commented 2 months ago

We couldn't recreate the speed problem, so don't hold up anything waiting for us. No telling what the issue was.

We have a baseRobot class that has all of the things our robot needs to do. Here is our first driveArcDist method:

    def driveArcDist(
        self,
        radius,
        dist,
        speedPct=DEFAULT_BIG_MOT_SPEED_PCT,
        accelPct=DEFAULT_BIG_MOT_ACCEL_PCT,
        gyro=True,
        then=Stop.BRAKE,
        wait=True,
    ):
        speed = RescaleStraightSpeed(speedPct)
        accel = RescaleStraightAccel(accelPct)
        self.robot.use_gyro(gyro)
        self.robot.settings(straight_speed=speed, straight_acceleration=accel)
        self.robot.arc(radius=radius, distance=dist, then=then, wait=wait)

and so far it seems to be working fine. The two "rescale" methods take a percentage and turn that into the numbers that correspond for that parameter (it's just a linear interpolation).