EV3-G API: motors by dwalton76 · Pull Request #360 · ev3dev/ev3dev-lang-python

dwalton76 · 2017-09-20T03:17:06Z

No description provided.

dwalton76 · 2017-09-20T03:19:07Z

For issue #353
I'm still testing everything (so far so good) but figured I would go ahead and post it so everyone can start giving feedback before I spend too much time testing.

WasabiFan · 2017-09-20T04:44:38Z

+            self.position_sp = self.position - int(rotations * self.count_per_rot)
+
+    def set_position_degrees(self, power, degrees):
+        rotations = float(degrees / self.count_per_rot)


This codepath will read from count_per_rot twice. Can we restructure this to avoid that?

I could cache it here or we could do something in the Device class (or maybe the Motor class) where it is aware of what properties will never change and cache it there. If we want to do the latter I'll work on that as a separate PR.

RE: properties that never change. See:

ev3dev: cache invariant values for motors ev3go/ev3dev#53

ev3dev: cache invariant values for sensors ev3go/ev3dev#54

ev3dev: cache invariant values for ports ev3go/ev3dev#55

Filed issued #365 to track this

WasabiFan · 2017-09-20T04:45:08Z

+    def set_power(self, power):
+        self.speed_sp = power
+
+    def set_position_rotations(self, power, rotations):


What are these methods (setting the position_sp, time_sp, etc.) for? They don't seem to match up with the EV3-G blocks in the spec image.

Originally in on_for_degrees, etc I just had a run_to_abs_pos() call where I passed in all of needed kwargs. This worked fine for a single motor but for MoveTank and MoveSteering if I called

self.left_motor.on_for_degrees(...) self.right_motor.on_for_degrees(...)

it added delay between when the two motors start. We want to follow the model of "set as many parameters as you can for both motors, then start the motors" so that they start as close together as possible. So that is why set_power(), etc are broken out like this, it is so I can set all of the parameters first when working with pairs of motors.

How about making them private (i.e. leading underscore)?

yep can make them private

WasabiFan · 2017-09-20T04:46:57Z

+        """
+        Rotate the motor at 'power' for 'rotations'
+        """
+        self.set_power(power)


These powers will be in tacho counts per second. Do we not want to take input on a [-100,100] scale as LEGO does?

Shouldn't the scale be [-1 * self.max_speed, self.max_speed]?

The stock Lego interface measures speed/power in percentage of max_speed, and the max_speed is never exposed.

ah I see now...will clean this up

I share the same sentiments as @laurensvalk that we should be using scientific units where possible in places where LEGO uses 0..100 or -100..100

ev3dev/ev3dev#668 (comment)

So, in this case, our speed parameter should be in degrees per second or RPM.

Also, I wouldn't mined a default value for the brake parameter here as True since this is what gets used most of the time.

wait if the EV3-G on_for_rotations block only takes power as a percentage of max_speed and we take power as the scientific unit then our attempt at a 1:1 API isn't going to match

WasabiFan · 2017-09-20T04:47:59Z

+    def get_power(self):
+        return self.speed
+
+    def rotations_greater_than(self, threshold):


I think we should leave the users to do these comparisons themselves. I feel like this obfuscates the logic without any real gain.

Yeah they seem pretty silly to me too but David had screenshots for these blocks so I coded them up. I am fine with chopping them.

dlech · 2017-09-20T16:25:27Z

+        else:
+            self.stop_action = self.STOP_ACTION_COAST
+
+    def set_time(self, ms):


Function calls in python are expensive. Here and in several other places, your are just setting an attribute. It would be more efficient to just set the attribute directly.

ack will chop set_power() and set_time()

dlech · 2017-09-20T16:26:17Z

+
+    def set_brake(self, brake):
+        if brake:
+            self.stop_action = self.STOP_ACTION_BRAKE


As mentioned elsewhere, brake in EV3-G == hold in ev3dev

ack will change this

dlech · 2017-09-20T16:35:18Z

+        self.set_brake(brake)
+        self.run_to_abs_pos()
+
+    def on_for_seconds(self, power, seconds, brake):


All of the on_for_* methods should wait for completion before returning.

e.g., this method should not use self.run_time(), it should

self.run_forever() time.sleep(seconds) self.stop()

On second thought, my suggestion would not be a good implementation when there are multiple threads.

Unfortunately, we don't currently have a way to be notified by the motor driver that the current operation was interrupted.

We could have a wait_until_stopped arg and if it is True we call self.wait_until_not_moving(). But then our API would no longer match the EV3-G API exactly. We could just document how to use self.wait_until_not_moving()

Why can't we wait until 'running' not in self.state? Does the driver consider itself "running" while holding?

In EV3-G if you tell a motor to run for 3 rotations does it start the motor and move on to the next block or does it wait for the three rotations to finish? If it moves on then I think we are good with what we have now, it if blocks though then we need to add something like

self.wait_until('running', timeout=100) self.wait_until_not_moving()

It's a blocking call. It starts the motors and only returns to the caller once the motor has turned to the specified position. EV3-G programs are very commonly just a single sequence of motor blocks with some branching (drive forward, turn, drive forward, check sensor, etc.).

ok lets have it block by default but have an optional arg that can be passed to not block

dlech · 2017-09-20T16:36:16Z

+        self.set_brake(brake)
+        self.stop()
+
+    def get_rotations(self):


How about using python properties for these getters?

I hate decorators :) Since we do use @property elsewhere though I guess it does make sense to use them here....will fix

def rotations(self): pass rotations = property(rotations)

No decorator. 😆

WasabiFan · 2017-09-20T23:56:43Z

            self.position_sp = self.position - int(rotations * self.count_per_rot)

-    def set_position_degrees(self, power, degrees):
+    def _set_position_degrees(self, power, degrees):


Can we have rotations call degrees instead? It makes more sense to me to do everything in terms of the "smaller" units and convert from the "larger" ones as needed. Or, it might be optimal to just make them operate independently so we avoid unnecessary nesting for execution speed.

1 rotation, 360 degrees....rotation is the smaller unit :)

If you need more degrees than rotations to represent the same quantity, the quantity represented by one degree is smaller than the quantity represented by one rotation.

WasabiFan · 2017-09-20T23:57:23Z

-        self.time_sp = ms
-
-    def on_for_rotations(self, power, rotations, brake):
+    def on_for_rotations(self, power, rotations, brake=True, block=True):


What do others think about calling it "speed" instead of "power"? This would only apply if we switched to using scientific units.

yeah the use of power is some places and speed in other is already making my head spin :) I think the EV3-G blocks use power?

WasabiFan · 2017-09-21T02:12:04Z

        """
        return self.wait(lambda state: s not in state, timeout)

+    def _set_position_rotations(self, max_speed_pct, rotations):


Why max_speed_pct? I read that to mean "the maximum speed (percentage)" as opposed to "percentage of the max speed". TBH, I think it's an unnecessary distinction as most people would just consider it "speed percentage".

How about 'speed_pct'?

I like that.

WasabiFan · 2017-09-21T02:13:57Z

We also need to consider when we're updating docs+demos for these changes. I want to make sure that we don't lose track of that work.

P.S. I'm hoping I can find time to take on a few of the pending features; we'll see if I can make that work over the weekend. I don't want to take something then delay it if I get busy.

ddemidov · 2017-09-22T11:08:58Z

 import array
 import mmap
 import ctypes
+import logging


I think you should put logging into a separate PR. Every import makes startup time a bit longer, so we should only do this if necessary.

Fair point, I'll back out the logging parts that are in here and file an issue to chat about what logging we want in core.py.

ddemidov · 2017-09-22T11:13:47Z


+# The number of milliseconds we wait for the state of a motor to
+# update to 'running' in the "on_for_XYZ" methods of the Motor class
+WAIT_RUNNING_TIMEOUT = 100


Do we really need it? Should not it be solved by #241 and the related changes in kernel drivers (ev3dev/lego-linux-drivers#57)?

It is, what I found is that if you do

m.run_to_abs_pos(...) m.wait_until_not_moving()

The run_to_abs_pos will update the command file but there is some finite amount of time before the kernel updates the state file to running. So you really have to wait until the motor starts moving before you can wait for it to stop moving.

ddemidov · 2017-09-22T11:23:57Z

I wonder if introducing properties for various units would make a lot of these functions easier (and may be even unnecessary). 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.

…thon into ev3g-api-motors

dwalton76 · 2017-09-22T12:10:37Z

I think the only thing left in this PR to address is how to specify speed_pct vs speed_dps vs speed_rps so I'm going to go ahead and merge this one in and open a new issue for the speed thing.

EV3-G API: motors

5ace527

WasabiFan reviewed Sep 20, 2017

View reviewed changes

dlech reviewed Sep 20, 2017

View reviewed changes

dwalton76 added 2 commits September 20, 2017 22:49

EV3-G API: motors - fix things from code review

6def6ce

Make MoveSteering a child class of MoveTank

22f63a8

WasabiFan reviewed Sep 20, 2017

View reviewed changes

dwalton76 added 2 commits September 21, 2017 01:55

rename power to speed, is a % of max_speed, must be between -100 and 100

81586d9

Remove _set_position_rotations call in _set_position_degrees

f5d436d

WasabiFan reviewed Sep 21, 2017

View reviewed changes

rename max_speed_pct to speed_pct

d4805b9

ddemidov reviewed Sep 22, 2017

View reviewed changes

dwalton76 added 2 commits September 22, 2017 12:05

Merge branch 'develop' of https://github.com/dwalton76/ev3dev-lang-py…

0737bc0

…thon into ev3g-api-motors

Comment out logging in core.py

f4a3367

dwalton76 merged commit 6f8f6af into ev3dev:develop Sep 22, 2017

dwalton76 mentioned this pull request Sep 22, 2017

motors API: speed_pct vs speed_dps vs speed_rps #370

Closed

WasabiFan mentioned this pull request Sep 15, 2018

Flexible relative movement and improved parameter handling #523

Merged

Conversation

dwalton76 commented Sep 20, 2017

Uh oh!

dwalton76 commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwalton76 Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlech Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dwalton76 Sep 20, 2017 •

edited

Loading

dlech Sep 20, 2017 •

edited

Loading

ddemidov commented Sep 22, 2017 •

edited

Loading