-
Notifications
You must be signed in to change notification settings - Fork 84
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
Improved Gates & Triggers Script, Overhauled screensaver #309
Conversation
Hey @chrisib, thanks for all of the work you are doing. Any chance that you can slim this PR down a bit, or possibly spit it up? It has two main ideas (the new script and the screensaver changes). And then on top of that there are several files with changes that don't have anything to do with either of these ideas. Lately it's been hard for me to find enough time to give these PRs the attention that they deserve, and it seems that the same is true for the other reviewers. I apologize for that. Anything that you can do to reduce the size of your PRs will go a long way to help. In the meantime, I'll try to work through this as is, but it's going to take me a few sessions |
Because all the changes were made in branches off of branches I think the amount of work it would be to de-couple the two ideas may wind up being more than I can necessarily take on right now. The new script was written with the screensaver changes already made. I could move the new script out into its own MR, but we'd still have a situation like #311 where the screensaver changes are present anyway. (Assuming this MR gets approved I plan to rebase #311 and update that MR to reduce it.) I know this is a pretty big MR, so there's definitely no expectation on my end for it to get reviewed & merged right away. Definitely take your time. I will go ahead and revert a few files that only git linting changes; it looks like while I was applying the new screensaver changes & modifying the |
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.
I've looked over the new script in isolation and it looks pretty good. Just a couple of comments. I'll look over the screensaver in my next session.
I've just reverted the changes I made to some of the other scripts (pams, logic, etc...). That should narrow the scope of this MR. I'll make a new branch and re-apply those changes there. Edit: That should reduce the scope of what you need to look at for this MR. Thanks! |
I think I'm caught up with the changes this PR is making, but let me know if I'm clearly missing something already discussed: I can't quite work out the benefit of a screensaver that's only based on time, especially as this also adds the complexity of users needing to check for user input and manually call the notify_user_interaction function. This would also fix my other issue with the screensaver, which is that some scripts won't get touched but would like to be seen - I often leave harmonic LFOs running, not touching any controls, but producing a cute display in the corner of my rack, and this implementation would turn it into a screensaver. I think maybe I'm misunderstanding how programmers of new scripts can implement this screensaver and whether once moved from experimental to main, it'll be 'compulsory'. I know that in a lot of my scripts I'd rather risk a little bit of screen burn-in than have to touch a control to wake up the screen, because often touching a control will also alter the actual script, without there being a dedicated 'wake screen' button |
My thinking was that the screensaver-supported class would exist alongside the existing one, not replace it completely. Several scripts (mostly ones I've written) have duplicate code & logic to provide a basic screensaver (Pam's, Logic, G&T, Euclid, Quantizer, Seq. Switch). This MR is mainly to consolidate those and cut down on code duplication. For scripts that have a continuous, useful display (e.g. Harmonic LFOs, Particle Physics) the old Oled class can be used since there's no point in them having a screensaver. But for scripts where burn-in is a potential hazard, the new class exists as an option, not as a requirement. The goal is absolutely not to make this class mandatory for all future scripts, but rather make it a convenient parallel to the lower-level, no-screensaver New or existing scripts wanting to use the screensaver would simply need to:
Most real-world screensavers I've run into are time-based, with user interactions waking them up/resetting the timer before they cut in. Yes, checking the underlying frame buffers for pixel differences instead of a static timer could work, but that's a lot of CPU overhead. |
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.
I just started my review over, since it had been a while.
I only had a few minor suggestions to point out, but over all this looks pretty good.
I just sync'd my fork with the latest main & rebased this branch, and the CI seems to be failing now. I don't have time to look into it immediately, but it looks like an issue with the bootloader menu. |
…stants" This reverts commit 327c3cc.
…ver the gate output, add a toggle output, and two new end-of-fall outputs (for the dynamic gate & toggle outs)
…nsaver to G&T. Move the screensaver activation & blank timeouts into the Screensaver class itself instead of redefining them externally
…r & screen-blanking. Use this wrapper instead of europi.oled in the 3 scripts that use the screensaver
…ake up the G&T screensaver if either knob is turned OR if either button is pressed
…nsaver" This reverts commit 0f21705.
…irst and auto_show arguments to centre_text, modify all extant calls to centre_text to use the correct auto_show behaviour
…he next one or not" This reverts commit e8d878a.
…sociated changes to other scripts
…iagram. Add B2 behaviour to the timing diagram
Looks like it was just something weird in the rebase. I copied |
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 looks good to me now. Good work.
Add the `clear_first` and `auto_show` keyword arguments to `centre_text`. These are needed by the changes merged as part of #309
Add the `clear_first` and `auto_show` keyword arguments to `centre_text`. These are needed by the changes merged as part of #309
New Script: Gates & Triggers
Implementation of the modified G&T script as described in #307 (comment).
din
orb1
is an incoming gate or trigger signal that is processed as follows:cv1
outputs a gate whose rising edge corresponds with the rising edge ondin
. The length of this gate is determined byk1
,k2
, andain
cv2
outputs a 10ms, 5V trigger on the rising edge ofcv1
/din
/b1
(since those all share the same rising edge)cv3
outputs a 10ms, 5V trigger on the falling edge ofdin
/b1
cv4
outputs a 10ms, 5V trigger on the falling edge ofcv1
cv5
outputs a gate whose state toggles every time a rising edge is detected ondin
/b1
/b2
cv6
outputs a 10ms, 5V trigger on the falling edge ofcv5
Screensaver Overhaul
Added the new
experimental.screensaver.OledWithScreensaver
class, which acts as a 1:1 replacement foreuropi.oled
but with automatic support for activating a screensaver or blanking the screen.Whenever the user presses a button, turns a knob, or otherwise interacts with the module in a way that should clear the screensaver, the user should call the
notify_user_interaction()
method to reset the timer. Otherwise after 5 minutes of idle time, the.show()
method will show the screensaver instead of whatever is in theoled
frame buffer. After 20 minutes of idle time, the screen will go blank.As part of making this work as a 1:1 replacement the
centre_text
method has been modified; it no longer automatically callsself.show()
at the end, but the old behaviour can be re-enabled by passing theauto_show=True
argument to the method. All existing calls tocentre_text
have been modified accordingly to ensure no regressions.The new
OledWithScreensaver
class is used in Pams, Logic, Euclid, and G&T.