Closed BluePadraig closed 7 years ago
This is a great PR, thank you!
However there are a few things we'd need you to tweak before I can move on to testing it :D
Don't edit library/CHANGELOG.txt
, library/setup.py
or the __version__
string in library/fourletterphat/__init__.py
directly. These are changed automatically by our packaging tools when the final library release is built. While I totally appreciate the effort, let us worry about versioning and CHANGELOGs and all the boring stuff.
Don't explicitly credit yourself in the source files, the GitHub history already accounts for this, and nothing added directly into the code could ever tell the complete story.
I haven't updated all the DocStrings in the code we borrowed from Adafruit yet, but to fit with our code style they should broadly be in the following format (I'm just saying this for posteirty, I don't mind tidying up your DocStrings!):
def functionname(self, argument1, argument2):
"""Glow by cycling display brightness.
Cycles display brightness from low to high and back to low
at a frequency of 1/periodicity Hz and for a duration stated in seconds.
:param period: Period of glow effect. 1/periodicity Hz (default 4 for 0.25Hz)
:param duration: Duration of effect in seconds. (default 4 seconds)
"""
You've done a fantastic job anyway, thank you. I'm actually working on a set of contributor guidelines which would have outlined the stuff above. You can find them here: https://github.com/pimoroni/template-python/blob/master/.github/CONTRIBUTING.md
Thanks a lot for this feedback. I've updated comments, DocStrings and reverted to version 0.0.1 to take into account your suggestions.
Looking into this now. Good stuff. I had to tweak glow
slightly so it would work in Python 2. The integer division was causing a wait_between_transitions
value of 0
which caused all sorts of chaos and an infinite loop.
While I'll go with your solution for now, for posterity this is what I might have used:
def glow(self, period=4, duration=4):
period = float(period)
initial_brightness = self.get_brightness()
time_start = time.time()
time_end = time_start + duration
phase_offset = math.radians(90)
full_sweep = math.radians(359)
while time_end > time.time():
time_elapsed = time.time() - time_start
# Convert the period progression to radians
radians = (time_elapsed / period) * full_sweep
# Shift the phase so our sine will start at 1.0
radians = radians + phase_offset
# Convert degrees to a sine wave from -1.0 to 1.0
wave = math.sin(radians)
# Shift our wave up to 0.0 to 1.0
wave = (wave + 1) / 2
# Multiply by 15, and round to get 0 to 15
brightness = int(round(wave * 15, 0))
# Avoid unnecessary updates
if brightness != self.get_brightness():
self.set_brightness(brightness)
# Try to update the effect at 30fps
time.sleep(1.0 / 30.0)
self.set_brightness(initial_brightness)
By way of brief explanation for you, or anyone else who might stumble upon this thread, my reasons for doing this are:
Using "wall time" (IE: actually looking at what time it currently is when timing something in code) means that the effect will always appear to the same speed, no matter how long the code takes to execute. This isn't always the case, since slower devices might have a slower framerate, in some cases even to the point of dropping key points in the transition. But it's a good approach if you're dealing with something like the Pi where the performance gap between the Zero and the Pi 3 can result in a wide range of effect speeds. If you try to time something precisely with time.sleep()
it never adjusts to take into account the execution time of the code.
Basing the while
loop on wall timing alone, and separating duration
from any of the other logic means it's difficult or impossible to accidentally enter an infinite loop condition. Separation of logical concerns makes the code a little more robust!
math.sin
produces a sine wave which makes for really pleasing-to-the-eye transitions. Of course, you might not always want this.
I had indeed only tested with Python 3.
Your version of glow() is definitely more robust ! I'm looking forward to see it some day in the main branch :)
Patrick Lavernhe Creation of a version 0.0.2 with: