Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LockableKnob init fix #193

Merged
merged 6 commits into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions software/contrib/knob_playground.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
from machine import ADC
from time import sleep

from europi import k1, k2, b1, b2, oled, MAX_UINT16
from europi_script import EuroPiScript
from experimental.knobs import KnobBank

"""
An example program showing the use of a KnobBank or LockableKnobs. This script is not meant to be merged into main,
it only exists so that PR reviewers can try out a LockableKnob in physical hardware easily.
"""


class KnobPlayground(EuroPiScript):
def __init__(self):
super().__init__()
self.next_k1 = False
self.next_k2 = False

self.kb1 = (
KnobBank.builder(k1)
.with_locked_knob("p1", initial_uint16_value=0, threshold_percentage=0.02)
.with_locked_knob("p2", initial_uint16_value=MAX_UINT16 / 5)
.with_locked_knob("p3", initial_uint16_value=MAX_UINT16 / 3)
.build()
)
self.kb2 = (
KnobBank.builder(k2)
.with_disabled_knob()
.with_locked_knob("p4", initial_percentage_value=0.5, threshold_from_choice_count=7)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] nit: If you define choice_p4 as a instance variable and use len(choice_p4) here, that will remove magic numbers and make the example more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is that this file isn't merged into the repo. I'll remove it after approval.

.with_locked_knob("p5", initial_percentage_value=1, threshold_from_choice_count=3)
.build()
)

@b1.handler
def next_knob1():
self.next_k1 = True

@b2.handler
def next_knob2():
self.next_k2 = True

def main(self):
choice_p4 = ["a", "b", "c", "d", "e", "f", "g"]
choice_p5 = ["one", "two", "three"]

while True:
if self.next_k1:
self.kb1.next()
self.next_k1 = False
if self.next_k2:
self.kb2.next()
self.next_k2 = False

p1 = "X" if self.kb1.index == 0 else " "
p2 = "*" if self.kb1.index == 1 else " "
p3 = "*" if self.kb1.index == 2 else " "
pd = "*" if self.kb2.index == 0 else " "
p4 = "*" if self.kb2.index == 1 else " "
p5 = "*" if self.kb2.index == 2 else " "
text = (
f"{p1} {self.kb1.p1.range(1000):4} {pd} {k2.range()} \n"
+ f"{p2} {int(round(self.kb1.p2.percent(), 2)*100):3}% {p4} {self.kb2.p4.choice(choice_p4):5}\n"
+ f"{p3} {self.kb1.p3.read_position():4} {p5} {self.kb2.p5.choice(choice_p5):5}"
)
oled.centre_text(text)


if __name__ == "__main__":
KnobPlayground().main()
3 changes: 3 additions & 0 deletions software/contrib/menu.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
# This is a fix for a USB connection issue documented in GitHub issue #179, and its removal condition is set out in GitHub issue #184
if usb_connected.value() == 0:
from time import sleep

sleep(0.5)

bootsplash()

from bootloader import BootloaderMenu

# from contrib.knob_playground import KnobPlayground
from contrib.bernoulli_gates import BernoulliGates
from contrib.coin_toss import CoinToss
from contrib.consequencer import Consequencer
Expand All @@ -37,6 +39,7 @@

# Scripts that are included in the menu
EUROPI_SCRIPT_CLASSES = [
# KnobPlayground,
BernoulliGates,
CoinToss,
Consequencer,
Expand Down
38 changes: 27 additions & 11 deletions software/contrib/turing_machine.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,23 @@ def __init__(
bit_count=DEFAULT_BIT_COUNT,
max_output_voltage=MAX_OUTPUT_VOLTAGE,
clear_on_write=True,
flip_probability=0,
scale=MAX_OUTPUT_VOLTAGE,
length=DEFAULT_BIT_COUNT,
):
"""Create a new TuringMachine with a shift register of the specified bit count. Default is 16, minimum is 8.
The maximum output voltage is also configurable and defaults to `europi.MAX_OUTPUT_VOLTAGE`"""
The maximum output voltage is also configurable and defaults to `europi.MAX_OUTPUT_VOLTAGE`
"""

if bit_count < 8:
raise ValueError(f"Specified bit_count ({bit_count}) is less than the minimum (8).")
self.bit_count = bit_count
self.bits = getrandbits(self.bit_count)
self._flip_probability = 0
self._flip_probability = flip_probability
self.max_output_voltage = max_output_voltage
self._scale = max_output_voltage
self._length = bit_count
self.clear_on_write = clear_on_write
self._scale = scale
self._length = length
self._write = False

self.flip_probability_getter = lambda: self._flip_probability
Expand Down Expand Up @@ -185,7 +189,8 @@ def length(self, length):
@property
def write(self):
"""Returns the current value of the 'write switch'. When true the least significant bit will be cleared during
rotation, regardless of the `flip_probability`. This allows for real-time user manipulation of the sequence."""
rotation, regardless of the `flip_probability`. This allows for real-time user manipulation of the sequence.
"""
return self.write_getter()

@write.setter
Expand All @@ -197,8 +202,17 @@ def write(self, value: bool):
class EuroPiTuringMachine(EuroPiScript):
def __init__(self, bit_count=DEFAULT_BIT_COUNT, max_output_voltage=MAX_OUTPUT_VOLTAGE):
super().__init__()

self.LENGTH_CHOICES = [2, 3, 4, 5, 6, 8, 12, 16] # TODO: vary based on bit_count?
initial_scale_percent = 0.5 # TODO: load from saved state
initial_length = 8 # TODO: load from saved state

self.tm = TuringMachine(
bit_count, max_output_voltage, clear_on_write=self.config["write_value"] == 0
bit_count=bit_count,
max_output_voltage=max_output_voltage,
clear_on_write=self.config["write_value"] == 0,
length=initial_length,
scale=MAX_OUTPUT_VOLTAGE * initial_scale_percent,
)
self.tm.flip_probability_getter = self.flip_probability
self.tm.scale_getter = self.scale
Expand All @@ -209,8 +223,12 @@ def __init__(self, bit_count=DEFAULT_BIT_COUNT, max_output_voltage=MAX_OUTPUT_VO
self.kb2 = (
KnobBank.builder(k2)
.with_disabled_knob()
.with_locked_knob("scale", initial_value=0)
.with_locked_knob("length", initial_value=0)
.with_locked_knob("scale", initial_percentage_value=initial_scale_percent)
.with_locked_knob(
"length",
initial_percentage_value=(self.LENGTH_CHOICES.index(initial_length) * 2 + 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[discussion] I think it would make more sense to take len(CHOICES) and do percent calculations internally. From a user perspective, I think using a percent as an input for selecting a choice from a list is less clear than configuring the lockable knob to choose from value from a range of len(CHOICES).

Why not accept either len(CHOICES) as the configuration input and do the percentage calculations internally?

Perhaps I don't fully understand the problem you encountered that inspired this change, but I do still see value in simplifying the API for usability sake with CHOICE knobs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I did start going down this road earlier in the development of LockableKnob but I backed off of it because of the flexibility that the Knob api allows. Specifically, the api allows the client to read the knob for any number of choices at any time. So given that, saving the value for 'choice 2' felt weird to me.

That said, thinking about your suggestion more and looking at this code here, I think that you are right and we could make a 'choices' api for lockable knob that would make sense and be way cleaner than this mess.

I think that this change would be an improvement beyond the scope of this PR however, would you be ok with it coming in a followup PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've found knobs to mostly represent "int", "percent", or "choice". If LockableKnob had an interface for each of those, I think that would help with the API clarity and usability.

/ (len(self.LENGTH_CHOICES) * 2),
)
.build()
)

Expand Down Expand Up @@ -253,9 +271,7 @@ def scale(self):

def length(self):
if self.kb2.current_name == "length":
return self.kb2.length.choice(
[2, 3, 4, 5, 6, 8, 12, 16] # TODO: vary based on bit_count?
)
return self.kb2.length.choice(self.LENGTH_CHOICES)
else:
return self.tm._length

Expand Down
77 changes: 56 additions & 21 deletions software/firmware/experimental/knobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,49 @@ class LockableKnob(Knob):

This class is useful for cases where you want to have a single physical knob control several
parameters (see also the :class:`KnobBank` class). Or where the value of a parameter needs to be
disassociated from the postition of the knob, as in after loading saved state.
disassociated from the position of the knob, as in after loading saved state.

This class accepts two different parameters to specify it's initial value,
`initial_uint16_value` and `initial_percentage_value`. Only one initial value may be specified.
If both are specified, `initial_percentage_value` is ignored. The percentage value is more
useful if you would like to hardcode a starting value for a knob in the code in a readable way.
The uint16 value uses the knob's internal representation and is more appropriate for use when
loading a saved knob position. If your script would like to read the internal representation of
current position of a `LockableKnob`, first lock the knob, then read it's value.::

lockable_knob.lock()
internal_rep = lockable_knob.value

:param knob: The knob to wrap.
:param initial_value: The value to lock the knob at. If a value is provided the new knob is locked, otherwise it is unlocked.
:param initial_uint16_value: The UINT16 (0-`europi.MAXINT16`) value to lock the knob at. If a value is provided the new knob is locked, otherwise it is unlocked.
:param initial_percentage_value: The percentage (as a decimal 0-1) value to lock the knob at. If a value is provided the new knob is locked, otherwise it is unlocked.
:param threshold: a decimal between 0 and 1 representing how close the knob must be to the locked value in order to unlock. The percentage is in terms of the knobs full range. Defaults to 5% (0.05)
"""

STATE_UNLOCKED = 0
STATE_UNLOCK_REQUESTED = 1
STATE_LOCKED = 2

def __init__(self, knob: Knob, initial_value=None, threshold_percentage=DEFAULT_THRESHOLD):
def __init__(
self,
knob: Knob,
initial_percentage_value=None,
initial_uint16_value=None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[optional] nit: This is Python, just int is fine, since there is no other int type. If the concern is clearly communicating a max value of uint16, that could be in the documentation. That might make the api interface a little more pythonic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of this comes directly from the MAX_UINT16 constant in europi.py. And yes the intention is to describe this parameter as much more constrained than an int. I agree that the name is verbose, however the problem that this PR is trying to resolve is the confusion over exactly what this value represents. Putting it in the parameter name is my way of solving that.

threshold_percentage=DEFAULT_THRESHOLD,
):
super().__init__(knob.pin_id)
self.pin = knob.pin # Share the ADC
self.value = initial_value if initial_value != None else 0
if initial_value == None:
self.state = LockableKnob.STATE_UNLOCKED
else:

if initial_uint16_value != None:
self.value = initial_uint16_value
self.state = LockableKnob.STATE_LOCKED
elif initial_percentage_value != None:
self.value = (1 - initial_percentage_value) * MAX_UINT16
self.state = LockableKnob.STATE_LOCKED
else:
self.value = MAX_UINT16 # Min value
self.state = LockableKnob.STATE_UNLOCKED

self.threshold = int(threshold_percentage * MAX_UINT16)

def __repr__(self) -> str:
Expand Down Expand Up @@ -74,7 +98,7 @@ class DisabledKnob(LockableKnob):
:param knob: The knob to wrap."""

def __init__(self, knob: Knob):
super().__init__(knob, initial_value=MAX_UINT16)
super().__init__(knob, initial_uint16_value=MAX_UINT16)

def request_unlock(self):
"""LockedKnob can never be unlocked"""
Expand All @@ -92,7 +116,7 @@ class KnobBank:
KnobBank.builder(k1)
.with_disabled_knob()
.with_unlocked_knob("x", threshold=0.02)
.with_locked_knob("y", initial_value=1)
.with_locked_knob("y", initial_percentage_value=1)
.build()
)

Expand All @@ -117,9 +141,9 @@ def __init__(self):

self.kb1 = (
KnobBank.builder(k1)
.with_locked_knob("p1", initial_value=1, threshold_percentage=0.02)
.with_locked_knob("p2", initial_value=1)
.with_locked_knob("p3", initial_value=1)
.with_locked_knob("p1", initial_percentage_value=1, threshold_percentage=0.02)
.with_locked_knob("p2", initial_percentage_value=1)
.with_locked_knob("p3", initial_percentage_value=1)
.build()
)

Expand Down Expand Up @@ -182,7 +206,8 @@ def with_disabled_knob(self) -> "Builder":
def with_locked_knob(
self,
name: str,
initial_value,
initial_percentage_value=None,
initial_uint16_value=None,
threshold_percentage=None,
threshold_from_choice_count=None,
) -> "Builder":
Expand All @@ -191,17 +216,23 @@ def with_locked_knob(
`threshold_from_choice_count` is a convenience parameter to be used in the case where
this knob will be used to select from a relatively few number of choices, via the
:meth:`~europi.Knob.choice()` method. Pass the number of choices to this parameter and
an appropriate threshhold value will be calculated.
an appropriate threshold value will be calculated.

:param name: the name of this virtual knob
:param threshold_percentage: the threshold percentage for this knob as described by :class:`LockableKnob`
:param threshold_from_choice_count: Provides the number of choices this knob will be used with in order to generate an appropriate threshold.
"""
if initial_value is None:
raise ValueError("initial_value cannot be None")
if initial_uint16_value is None and initial_percentage_value is None:
raise ValueError(
"initial_percentage_value and initial_uint16_value cannot both be None"
)

return self._with_knob(
name, initial_value, threshold_percentage, threshold_from_choice_count
name,
initial_percentage_value=initial_percentage_value,
initial_uint16_value=initial_uint16_value,
threshold_percentage=threshold_percentage,
threshold_from_choice_count=threshold_from_choice_count,
)

def with_unlocked_knob(
Expand All @@ -216,7 +247,7 @@ def with_unlocked_knob(
`threshold_from_choice_count` is a convenience parameter to be used in the case where
this knob will be used to select from a relatively few number of choices, via the
:meth:`~europi.Knob.choice()` method. Pass the number of choices to this parameter and
an appropriate threshhold value will be calculated.
an appropriate threshold value will be calculated.

:param name: the name of this virtual knob
:param threshold_percentage: the threshold percentage for this knob as described by :class:`LockableKnob`
Expand All @@ -226,7 +257,7 @@ def with_unlocked_knob(
if self.initial_index != None:
raise ValueError(f"Second unlocked knob specified: {name}")

self._with_knob(name, None, threshold_percentage, threshold_from_choice_count)
self._with_knob(name, None, None, threshold_percentage, threshold_from_choice_count)

self.initial_index = len(self.knobs_by_name) - 1

Expand All @@ -235,7 +266,8 @@ def with_unlocked_knob(
def _with_knob(
self,
name: str,
initial_value,
initial_percentage_value,
initial_uint16_value,
threshold_percentage,
threshold_from_choice_count=None,
):
Expand All @@ -254,7 +286,10 @@ def _with_knob(
threshold_percentage = DEFAULT_THRESHOLD

self.knobs_by_name[name] = LockableKnob(
self.knob, initial_value=initial_value, threshold_percentage=threshold_percentage
self.knob,
initial_percentage_value=initial_percentage_value,
initial_uint16_value=initial_uint16_value,
threshold_percentage=threshold_percentage,
)

return self
Expand Down
Loading