-
Notifications
You must be signed in to change notification settings - Fork 76
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
experimental: LockableKnob and KnobBank #155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, I'm really excited about this enhancement.
What do you think about moving the "KnobPlayground" out of a python file and into a MD or docstring similar to what we did with Menu and Save/Load State demos?
software/contrib/knob_playground.py
Outdated
self.kb2 = ( | ||
KnobBank.builder(k2) | ||
.with_disabled_knob() | ||
.with_locked_knob("p4", initial_value=1, threshold=1 / 7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[discussion] It looks like threshold
is an optional ratio relative to the max number of choices? What if instead we had a parameter like max_choices
that handled the mathy stuff internally?
.with_locked_knob("p4", initial_value=1, max_choices=7)
...
# LockableKnob.__init__(...)
self.threshold = int((1/max_choices) * MAX_UINT16)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Threshold is just a percentage. (maybe the parameter name should be threshold_percentage
?) Here I'm using the number of choices to calculate a percentage that 'feels right'. The LockedKnob
just like the regular Knob
doesn't care about what method is used to read the position (choice(), percent(), etc) so I'd like to avoid going down a setup road that makes the user think that the knob can only be read in one way.
That said, this does sound really convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this feature to the builder.�
software/firmware/europi.py
Outdated
@@ -98,6 +98,7 @@ class AnalogueReader: | |||
""" | |||
|
|||
def __init__(self, pin, samples=DEFAULT_SAMPLES): | |||
self._pin_id = pin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] I think this would be useful outside of this class, so I'd recommend dropping the implied private underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
assert round(knob_bank.param1.percent(), 2) == 0.50 | ||
assert round(knob_bank.param2.percent(), 2) == 0 | ||
assert round(knob_bank.current.percent(), 2) == 0.50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice interface for dealing with several parameters controlled by a single knob. Well done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Yeah, that style of documentation is a good idea. I think I'll leave the playground where it is though (and still remove it before merge) |
1d986c6
to
24cc9cf
Compare
Done. |
also adds the experimental dir for code that we're not sure of yet adds the pin_id field to AnalogueReader so that we can wrap knobs more easily.
a collection of LockableKnobs to simulate several virtual knobs on the same physical knob
not intended to be merged into main
also: - add some more tests - cleanup threshold - name the Disabled Knob
add experimental.knobs to the api docs also add a missing parameter and remove an unused parameter
9e43e12
to
b9231f3
Compare
ce372f3
to
e3534b5
Compare
This PR consists of three main additions:
LockableKnob
andKnobBank
KnobPlayground
, to demonstrate these experimental classesThe idea with the 'experimental' package is that we can release code that we think might change or might not be fully thought out or feature rich enough to include in the firmware proper. We would do this for the purpose of allowing other developers to write scripts against it and provide feedback. Once we feel that it is stable enough we can promote it out of the experimental package and update calling scripts.
In this spirt, this PR includes two experimental classes, LockableKnob and KnobBank. I developed these for use in PR #114, the Turing Machine, but thought that they would be applicable to other scripts. They provide a means for controlling multiple parameters with a single knob, as well as disabling and re-enabling a knob. In addition a threshold mechanism is used to prevent large jumps in the value when switching between parameters. More information can be found in the classes' documentation.
Finally, in oder to provide a means of actually interacting with these new knob types, I've included the KnobPlayground. It is not my intention that this Script ever gets merged into main. It only exists here to provide an easy way to try these new classes out, and to see what code that uses them might look like.