motors API: speed_pct vs speed_dps vs speed_rps

[dwalton76]

While working on #353 #360 one thing we did not address was the API for specifying the motor speed in something other than speed_pct. The other two we want are degrees per second and rotations per second.

Some ideas that were floated:

Option 1
How about we make the default units % just like EV3-G, plus introduce scaling factor constants for those who want scientific units.

# run the motor at 50% of max speed
m.run(50)
# run the motor at 400 degrees/second
m.run(400 * m.DPS)
# run the motor at 60 RPM
m.run(60 * m.RPM)

Option 2
why not m.run(speed_pct=50), m.run(dps=400), and m.run(rpm=60)? If we define run as

def run(self, speed_pct=None, dps=None, rpm=None)

We have other parameters that need to be passed in though (speed) so basically everything becomes a positional arg. So the user would do one of

m.on_for_degrees(speed_pct=40, degrees=720)
m.on_for_degrees(speed_dps=90, degrees=720)
m.on_for_degrees(speed_rps=0.25, degrees=720)

[ddemidov]

Option 3
(copied from #360 (comment))
Introduce proxy properties for various units. For example, we could have

@property
def degrees_sp(self):
    return self.position_sp * self.POS2DEG_FACTOR
@degrees_sp.setter
def degrees_sp(self, value):
    self.position_sp = value * self.DEG2POS_FACTOR

and then same thing for rotations_sp, power_sp (percentage of max_speed_sp), etc.

[dwalton76]

@ddemidov can you give an example m.on_for_degrees(...) call with option 3?

[ddemidov]

I would just call our existing m.run_to_rel_position(degrees_sp=360).

[dwalton76]

How would it be used for m.on_for_degrees(...) though?

[ddemidov]

I think m.run_to_rel_position(degrees_sp=360) is exactly m.on_for_degrees(360), no?

[dwalton76]

Say I'm a user and I want to rotate a motor for 720 degrees at 360 degrees per second, I'm asking how does that happen with option 3 via a call to m.on_for_degrees(). Today if you call m.on_for_degrees(360) it is going to assert because 360 is speed_pct and it is greater than 100.

[ddemidov]

m.run_to_rel_position(degrees_sp=720, dps_sp=360) should do it, where dps_sp is another proxy property, but for speed_sp. We can rename (or create an alias) the method to on_for_degrees (I would prefer even simpler and generic on_for though: it would take kwargs dict and it would be useable with degrees_sp or seconds_sp or what not).

[dwalton76]

Ignore run_to_rel_position, that is what is happening behind the scenes :) So you would have m.on_for_degrees(dps_sp=360, degrees=720)? If so that looks the same as option 2

[ddemidov]

Yes, it looks very much like option 2, but the implementation should is easier. Instead of

def on_for_degrees(self, degrees=None, dps=None, speed=None, power=None):
  if dps is not None:
    self.speed_sp = dps * dps2speed
  elif power is not None:
    self.speed_sp = dps * power2speed
  self.position_sp = degrees * deg2pos
  self.run_to_rel_position()    

we can do

def on_for_degrees(self, degrees=None, **kwargs):
  for key,val in kwargs:
    setattr(self, key, val)
  self.position_sp = degrees * deg2pos
  self.run_to_rel_position()    

or even

def on_for(self, **kwargs):
  for key,val in kwargs:
    setattr(self, key, val)
  if degrees_sp in kwargs: # this is tricky, need to check for other positional units...
    self.run_to_rel_position() 
  elif time_sp in kwargs:
    self.run_timed()
  ...

[ddemidov]

We can come up with a name more generic than on_for_degrees that would mean 'turn for some rotations units'. rotate_for? Then we can do rotate_for(degrees_sp=720, dps_sp=360), or rotate_for(turns_sp=2, rpm_sp=1), and it would mean the same thing.

EDIT1: And the implementation for it is as simple as

def rotate_for(self, **kwargs):
  for key,val in kwargs:
    setattr(self, key, val)
  self.run_to_rel_position()    

EDIT2: And that is exactly the implementation of our current run_to_rel_position.

[dwalton76]

To me using kwargs makes the API much less readable/obvious if you are a new user looking at this for the first time...and that is really who we are trying to target here. Today you can look at the EV3-G motor block and look at def on_for_degrees(self, speed_pct, degrees, brake=True, block=True): and connect the dots pretty quickly on how they are equivalent. If we use kwargs we lose that.

[ddemidov]

We can put positional arguments into the documentation, but implement it with keyword args.

[dwalton76]

That doesn't solve the problem of having readable/obvious code though :)

[ddemidov]

Well, for me the code with kwargs is more readable because it is more concise, and the resulting API is better because it gives you more freedom: you can use whatever meaningful combination of attributes you need, and you are not constrained by the set of positional arguments a developer thought would be enough.

[dwalton76]

it gives you more freedom: you can use whatever meaningful combination of attributes you need, and you are not constrained by the set of positional arguments a developer thought would be enough

Agreed but we already have that with the current run_XYZ API, that isn't the problem we are trying to solve. To me we don't want crazy amounts of freedom, we want a nice clean/obvious 1:1 mapping with the EV3-G motor blocks.

Just my 2c though...lets see what the others think since you and I are just going back and forth on this.