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

Add Lutra script #353

Merged
merged 13 commits into from
May 23, 2024
Merged

Add Lutra script #353

merged 13 commits into from
May 23, 2024

Conversation

chrisib
Copy link
Collaborator

@chrisib chrisib commented Mar 9, 2024

Adds a new syncable LFO script, similar to e.g. Harmonic LFO, but without the harmonics. Each output is a free-running LFO outputting one of 5 wave shapes (sine, square, triangle, saw, ramp) with a related-but-different clock speed; cv2 is slightly faster than cv1, cv3 is slightly faster than cv2, etc..., resulting in interesting phase shifting & drifting.

Inspired by Otterly by Expert Sleepers.

@chrisib chrisib added the new script Addition of a new contrib script label Mar 9, 2024
@chrisib
Copy link
Collaborator Author

chrisib commented Mar 23, 2024

I've made a small tweak to how the script works. Specifically the spread control now smoothly adjusts the speed of each channel relative to cv1:

  • cv1 goes from x1 to x1 of cv1 (surprise! it's the master channel, and always goes at the same speed as itself!)
  • cv2 will go from x1 to x1.2 of cv1
  • cv3 will go from x1 to x1.25 of cv1
  • cv4 will go from x1 to x1.333... of cv1
  • cv5 will go from x1 to x1.5 of cv1
  • cv6 will go from x1 to x2 of cv1

So at maximum spread each channel from cv2-6 is a nice harmonic interval of the speed of cv1: 1:1, 6:5, 5:4, 4:3, 3:2, and 2:1.

This fix makes the spread more predicable when changing the speed of cv1 via k1 and/or ain.

Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good to me. I like the addition of the multithreading documentation you put some good work into it.

I still want to actually run this script and play with it, and dig into the threading a little more, but here is an initial review.


## Configuration

The functionality of `ain` can be configured by creating/editing `/saved_state_Lutra.txt` on the module. This JSON file
Copy link
Collaborator

Choose a reason for hiding this comment

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

[discussion] Saved state is not meant to be user editable. It is meant to save the state of a running script so that it can pick up where it left off next time it runs. This sounds more like configuration and should use the configuration feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I interpreted things differently; I see the global config files (experimental_config & europi_config) as module-wide settings, while the saved_state files are effectively config files with a per-script focus. Adding a script-specific setting to e.g. experimental_config feels like overkill, since that setting would never be used anywhere else.

Creating separate saved-state & config files for each script also feels counter-intuitive to me. The saved state is the current configuration of the script. This particular feature is just a sneaky way of adding an extra bit of functionality that otherwise wouldn't be supported.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that's how you are interpreting these features then the documentation for them needs to be improved. The firmware level config files are certainly module wide settings, but individual scripts can also have configuration, see the turing-machine and diagnostic script for examples.

This particular feature is just a sneaky way of adding an extra bit of functionality that otherwise wouldn't be supported.

My assumptions about why this feature wouldn't otherwise be supported is that the limited number of hardware UI elements on the EuroPi make adding these features difficult with out making it very menu driven. This is explicitly one of the things that the configuration system was meant to address.

Please see the following issue and PRs for details of the motivations for these features. I will open a PR that pulls these details into the documentation.

#163
#220
#185

Copy link
Collaborator Author

@chrisib chrisib Mar 26, 2024

Choose a reason for hiding this comment

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

Fair enough. I'll move that one item out of the saved state & create a separate config file for it.

EDIT
Done


## Re-syncing

When `b1` or `din` receive a high voltage all CV outputs are temporarily halted. Once the input drops low again
Copy link
Collaborator

Choose a reason for hiding this comment

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

[required] This documentation is meant to be understood by non-developer users of the script. As such, I don't think that describing a button as either high or low voltage is clear. I think it would be more clear to describe it in terms of the button being pressed or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll reword this to make it less developery-sounding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

self.b1_high = False
self.b2_high = False

# MS ticks when events were last recorded
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] These timestamps don't seem to be read anywhere. Are the meant to be a part of the class's API? If so they should be documented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they're part of the API. They aren't used here but they are used in other multi-threaded scripts I have on the go. I'll add some additional documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an additional section in the class header to explain the additional fields.

depending on previous settings and random noise) they may be phase-shifted from each other.

Turning `k2`clockwise will gradually increase the spread of clock speeds. `cv1` will always stay locked to the base
clock speed, with `cv2-6` becoming progressively faster. At maximum spread, the outpts follow common harmonic intervals
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 haven't played with this script yet, but I think that this would be useful/fun: Have you considered having the spread be zero in the center, adding the ability to both slow down and speed up the related clocks?

Copy link
Collaborator Author

@chrisib chrisib Mar 25, 2024

Choose a reason for hiding this comment

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

An interesting thought. Probably wouldn't be too hard to change the knob behaviour to allow the spread to speed up or slow down cv2-6.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I played around with this a little bit. In the end, I don't think I actually like this change; with the current implementation it's very easy to "lock" the waves' relative phases by just turning the spread control all the way to the left. That feels like a useful feature that gets lost if you need to centre the knob to set the spread to zero; the slightest imprecision will cause the waves to start spreading over time.

I may try it out some more later, but for now I think I'm leaning towards keeping the behaviour as it is right now.

"ramp": 4,
}

WAVE_SHAPE_SINE = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

[optional] You could use these constants as the values in the above dictionary to reduce the number of places where the code has to match up a shape with its value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

software/contrib/lutra.py Show resolved Hide resolved
self.load()

def load(self):
"""Load and apply the saved configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

[required] The configuration file and the saved state file are two different things in the europi firmware. It's confusing to have the documentation here call it a configuration file and then read from the saved state file. Ditto for the save() method below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like I said above, the saved_state file (in my interpretation) is the same thing as a per-script configuration file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

…state for everything. Update the docs to reflect the changes
Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

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

I never ended up spending anymore time with this, but that’s not a good reason to hold it up.

@chrisib chrisib merged commit 71f178a into Allen-Synthesis:main May 23, 2024
3 checks passed
@chrisib chrisib deleted the lutra branch May 23, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new script Addition of a new contrib script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants