Servo #15

brentru · 2017-08-01T16:54:23Z

Added easy control for hobby servos with variable constraints. Added an writeMicroseconds method, along with writeAngle.

Addressing Issue #3 - servo control

tannewt

Thank you for picking this up as well! Please rebase this onto the latest master, it looks like you merged it instead. As a rule of thumb, only merge when combining two concurrently modified shared branches. Otherwise a rebase leads to a cleaner branch history by pretending your changes happened after the other changes.

tannewt · 2017-08-01T17:18:37Z

simpleio.py

+        # prevent weird values due to read before write calls
+        self.angle = 90
+
+    def writeAngle(self, angle):


Should be formatted as write_angle (https://www.python.org/dev/peps/pep-0008/#function-names). I think set_angle makes more sense too.

Modified to conform to the style guide. Write was being used because it's similar to the .write() in arduino

tannewt · 2017-08-01T17:19:22Z

simpleio.py

+            time.sleep(1)
+    """
+    def __init__(self, servoPin, servoMin = 0.5, servoMax = 2.5):
+        self.myServo = pulseio.PWMOut(servoPin, frequency = 50)


I'd call this self.pwm instead of myServo because Servo is also the name of the class. It feels like it could be self-referential which would be weird.

myServo was to keep consistency with the Arduino code. self.pwm works as well and makes more sense, changed:
self.pwm = pulseio.PWMOut(pin, frequency = 50)

tannewt · 2017-08-01T17:23:15Z

simpleio.py

@@ -101,6 +102,67 @@ def shift_out(dataPin, clock, value, msb_first=True):
            tmpval = bool((value & (1 << i)))
            dataPin.value = tmpval

+class Servo:


Please follow the Google Python Style (based on PEP8) for variable naming. https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming

I changed servoMin/servoMax to

min_pulse

and

max_pulse

to conform with the Google Python Style Guide

tannewt · 2017-08-01T17:25:24Z

simpleio.py

+            myServo.writeMicroseconds(5500)
+            time.sleep(1)
+    """
+    def __init__(self, servoPin, servoMin = 0.5, servoMax = 2.5):


The servo prefix isn't useful here. I'd rename them pin, min_pulse and max_pulse.

Agreed, revised to:
def __init__(self, pin, min_pulse = 0.5, max_pulse = 2.5):

Signed-off-by: brentru <robots199@me.com>

tannewt

Looks much better! Just a couple more things.

tannewt · 2017-08-04T00:37:42Z

simpleio.py

+        dutyPercent = pulseWidth / 20.0
+        self.pwm.duty_cycle = int(dutyPercent * 65535)
+
+    def set_Microseconds(self, uS):


Should this set self.angle as well in case its read back? If so, maybe it makes more sense to make an angle property instead since its both set and read back.

Microseconds should be lowercase too.

set ms as self.angle so it can be read back by read(). Microseconds switched to microseconds to match public functions in the style guide.

tannewt · 2017-08-04T00:40:02Z

simpleio.py

+        dutyPercent = ms / 20.0
+        self.pwm.duty_cycle = int(dutyPercent * 65535)
+
+    def detach(self):


deinit is the standard name for this in CircuitPython: https://circuitpython.readthedocs.io/en/stable/docs/design_guide.html#lifetime-and-contextmanagers

changed to
self.pwm.deinit()

I meant the Servo method itself too.

tannewt · 2017-08-04T00:41:32Z

simpleio.py

+    """
+    Easy control for hobby (3-wire) servos
+
+    :param ~pulseio.PWMOut pin: PWM pin where the servo is located.


This should be microcontroller.Pin I believe. (Thats what items in board are.)

tannewt

A couple more things.

tannewt · 2017-08-04T16:30:01Z

simpleio.py

+        dutyPercent = ms / 20.0
+        self.pwm.duty_cycle = int(dutyPercent * 65535)
+
+    def detach(self):


I meant the Servo method itself too.

tannewt · 2017-08-04T16:31:15Z

simpleio.py

+        """Writes a value in microseconds to the servo"""
+        ms = (uS/1000)
+        ms = max(min(self.max_pulse, ms), self.min_pulse)
+        self.angle = ms


I don't think this is what you want because you aren't doing the reverse of line 147. The range of ms will be very different from angle.

Agreed! I used angles as my base unit, such that .setter can use uS as well as Degrees. microseconds_to_angle helps to convert uS to degrees, which is used by the setter.

tannewt · 2017-08-04T16:32:38Z

simpleio.py

+        # prevent weird values due to read before write calls
+        self.angle = 90
+
+    def set_angle(self, angle):


I'd suggest making angle a property:

@property def angle(self): return self.angle @angle.setter def angle(self, angle): <contents of set_angle>

Ok! I made angle a property. The implementation provided caused a "Maximum Recursion Depth Exceeded" error, so it was changed to:
@property def angle(self): return self._angle
The example was modified to reflect these changes.

Also, def angle(self, angle) was changed to def angle(self, degrees) to avoid confusion with having both a variable and a method named angle.

tannewt

A couple more things.

tannewt · 2017-08-08T18:05:48Z

simpleio.py

+
+    :param ~microcontroller.Pin pin: PWM pin where the servo is located.
+    :param int min_pulse: Minimum amount of microseconds allowed. Varies depending on type of servo.
+    :param int max_pulse: Maximum amount of microseconds allowed. Varies depending on type of servo.


You might want to state what min_pulse and max_pulse correspond to angle wise.

Changed to state how the uS correspond to the angle

:param int min_pulse: Pulse width (microseconds) for the absolute minimum angle on the servo.

:param int max_pulse: Pulse width (microseconds) for the absolute maximum angle on the servo.

You are assuming that the servo ranges from 0 to 180 degrees correct? Why not just say that?

Good point. Reworded as:

Pulse width (microseconds) corresponding to 180 degrees
and
Pulse width (microseconds) corresponding to 0 degrees

tannewt · 2017-08-08T18:06:27Z

simpleio.py

+        """Converts microseconds to a degree value"""
+        return map_range(us, 500, 2500, 0, 180)
+
+    def detach(self):


Rename this to deinit please

tannewt · 2017-08-08T18:09:53Z

simpleio.py

+        self.min_pulse = min_pulse
+        self.max_pulse = max_pulse
+        self._angle = 90
+        self._microseconds = 0


This isn't needed anymore.

…artifacts from old code

tannewt · 2017-08-08T21:10:57Z

Looks good! Thank you!

brentru requested a review from tannewt August 1, 2017 16:54

tannewt requested changes Aug 1, 2017

View reviewed changes

brentru added 2 commits August 3, 2017 17:43

Added easy control for hobby servos

32924da

fixed per tannewt’s review

992c3b7

Signed-off-by: brentru <robots199@me.com>

tannewt requested changes Aug 4, 2017

View reviewed changes

small revisions

ae95573

tannewt requested changes Aug 4, 2017

View reviewed changes

tannewt self-assigned this Aug 6, 2017

added two converters and one property

1cb1b14

tannewt requested changes Aug 8, 2017

View reviewed changes

Brent Rubell added 2 commits August 8, 2017 15:48

small fixes - changed detach to deinit, edited comments, and removed …

a06fa57

…artifacts from old code

revised comments

1e9d44c

tannewt approved these changes Aug 8, 2017

View reviewed changes

tannewt merged commit f4d2736 into adafruit:master Aug 8, 2017

Servo #15

Servo #15

Uh oh!

Conversation

brentru commented Aug 1, 2017

Uh oh!

tannewt left a comment

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!

brentru Aug 3, 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!

tannewt left a comment

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!

tannewt left a comment

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!

tannewt left a comment

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!

tannewt commented Aug 8, 2017

Uh oh!

brentru Aug 3, 2017 •

edited

Loading