Euclidean Rhythm App

[Bridgee]

Added Euclidean Rhythm app after the previous merging. (a minor typo fix for the document)

[awonak]

Thanks for separating this into its own PR. This script was a little more challenging to review (any by extension maintain) due to some of the organization of code which we can improve with a few tweaks. I'll do my best to describe where I faced challenges and how we can improve the code to make it more readable and easier to maintain.

1. Separation of UI code from SingleEuclideanGate class.

I think this is the root cause of the complexity of the script. If you look at other larger scripts, you'll notice that they have done a good job of separating the script logic from the display logic.

https://github.com/Allen-Synthesis/EuroPi/blob/main/software/contrib/strange_attractor.py

Other examples have successfully decoupled all UI into a single function based on the current state of the script's behavior.

https://github.com/Allen-Synthesis/EuroPi/blob/main/software/contrib/cvecorder.py

For EuclideanRhythm, there's no reason for each SingleEuclideanGate instance to have any instance variables associated with displaying on the UI. That behavior should be the responsibility of your EuroPiScript class, and the SingleEuclideanGate should only be responsible for the data associated with a Euclidean Rhythm (steps, pulses, offset) and possibly other state variables like the current sequence step. Ideally the EuroPiScript should be responsible for a) handling the interaction between the peripheral components and your script b) script state (what changes with each step / handling user interaction) c) displaying state to the oled UI.

Think about that advice on how to organize your script and you'll find that refactoring your script to match that organization will make your code much easier to read.

2. Reduce magic numbers

There are a lot of instance of "magic numbers" that have no clear meaning. It's difficult for a reader to understand what the meaning of these numbers are, which also makes things difficult to troubleshoot when something isn't working as expected. If these values are derived from other numbers, assign them to a descriptive variable with a formula that uses those other values. That makes it clear what the intention is. Alternatively, add clear inline comments that explain what these numbers mean. Example:

if len(self.notes_list) < 10:
...
elif len(self.notes_list) < 14:
...
elif len(self.notes_list) < 20:

Even reading the surounding code, it's not clear what 10, 14 or 20 mean and why it changes the code behavior.

3. Code Reuse

Specifically with the method draw_notes(self): there are branching if/else statements that have a lot of duplicate code with a few small changing values. It would be much more readable and maintainable if you used the if/else statements to only change the values that change and assign them to a descriptive variable.

Consider this example refactoring. Instead of repeating calculations like this:

if len(self.notes_list) < 10:
    if i != self.step:
        oled.fill_rect(self.box_position[0] + 2 + i * 4, self.box_position[1] + 5, 3, self.box_size[1] - 5, 1)
    else:
        oled.fill_rect(self.box_position[0] + 2 + i * 4, self.box_position[1] + 8, 3, self.box_size[1] - 8, 1)
        oled.fill_rect(self.box_position[0] + 2 + i * 4, self.box_position[1] + 5, 3, 2, 1)
elif len(self.notes_list) < 14:
    if i != self.step:
        oled.fill_rect(self.box_position[0] + 2 + i * 3, self.box_position[1] + 5, 2, self.box_size[1] - 5, 1)
    else:
        oled.fill_rect(self.box_position[0] + 2 + i * 3, self.box_position[1] + 8, 2, self.box_size[1] - 8, 1)
        oled.fill_rect(self.box_position[0] + 2 + i * 3, self.box_position[1] + 5, 2, 2, 1)

You should instead use if/else statements to only change values that change, and assign equasions to variables and then change those base values inline:

# Set multiplier based on note length.
if len(self.note_list) < 10:
    multiplier = 4
elif len(self.notes_list) < 14:
    multiplier = 3
else:
    multiplier = 2

# Display box for current step
x1 = self.box_position[0] + 2 + i * multiplier
y1 = self.box_position[1] + 5
x2 = 2
y2 = self.box_size[1] - 5
if i != self.step:
    oled.fill_rect(x1, y1, x2, y2, 1)
else:
    oled.fill_rect(x1, y1 + 2, y2, y2 - 2, 1)
    oled.fill_rect(x1, y1, x2, y2, 1)

4. [Optional] I would suggest considering removing the internal clock since we have the digital in to receive clock, it's most likely that folks will have other master clock sources that they will want to use instead of using Euclidean Rhythms as a master clock. The internal clock adds quite a bit of complexity to the code that may not be necessary given likelihood that external clocks would be preferred.

Feel free to send any questions to the #programming channel on the Discord group, there are lots of helpful folks that will be eager to help with any additional questions you have on Python pro tips and best practices.

[Bridgee]

Hey @awonak, Thanks for all those valuable comments above, and I'll try to improve the code this weekend and get back to you soon. Bridge.

[Bridgee]

Hi @awonak, Thanks again for your comments. I noticed most of the comments are related to the GUI logic, so before I push the updated code, I want to clarify the code and get feedback from you. My current code separates the individual gate display logic and system function display logic:

draw_notes() and draw_box() are the methods of SingleEuclideanGate() draw_tempo_setting() are the methods of the main script (EuroPiScript)

This setup is mainly because the GUI has two pages, and the second page is only for the tempo setting, which is the core function. As to the gates visualization, it's logically a method of each gate. Also, if the user has a bigger/smaller display or more/fewer ports, they can always easily add or reduce the number of gates without changing the code structure (the gate visualization position and size are all parameterized). Of course, I can wrap the gate display part as another method of the EurPiScript for better readability, but it would only move the code out of the main() function.

You may notice that I couple some of the gate port setup call with the visualization call in from line 330 to 353. This is because I want to reduce the number of for-loop. Again, wrapping up each function block is always doable.

To respond to your second comment, those "magic numbers" are also GUI-related., The maximum number of steps of each gate is 19. So, for better visualization, if the length of steps is short, you will have a bigger step bar visualization and vice versa. For now, we have three different sizes of GUI, and they will change automatically, corresponding to the length of each gate.

The third comment makes a lot of sense to me. I've parameterized those "magic numbers" to avoid code reuse.

For the last comments, I plan to keep the internal clock and add a step reset function taking the signal from the digital-in port. The step reset function is essential for a sequencer. However, due to the limited IO, it's hard to add this function when using digital-in as the external clock.

[Bridgee]

One quick question, for the EuciPiScript class, I noticed some of the apps call super().init() but some don't. Is it necessary?

[mjaskula]

One quick question, for the EuciPiScript class, I noticed some of the apps call super().init() but some don't. Is it necessary?

It may not strictly be necessary for your script as it stands today. However, it’s a good idea to add it now, as it will more easily allow your script to support any features that may get added to EuroPi script in the future.

PR reviewers should be asking for the init call in scripts if it is missing, but it is easy for both sides to forget.

[roryjamesallen]

Sorry it's taken me so long to have a look at this PR! The script in its function does look great, and a Euclidian pattern generator is definitely something I would use!

I would repeat @awonak 's comments about the UI (and the code formatting). I have actually been working, albeit pretty slowly, on a Euclidian pattern generator myself. It is missing many features that yours has but I do believe it's a bit more atomic, i.e. it would be pretty trivial to add the functionality that you've come up with to it without having to edit anything fundamental. I think the UI with this script is slightly confusing/difficult to adapt, as it relies on a very specific layout on the OLED, so there isn't any room for editing how the script looks or works (I think the draw_notes method is the most obvious example of this; the UI and 'magic numbers' are all baked into the script, so it would be a pretty massive job if you wanted to change how the script looked, or add functionality!)

I've got my script on my fork of the repo, so perhaps you could have a look at what I've come up with and see what you think? It would be nice to be able to merge the best of both scripts, because yours does have a lot of functionality that I would love for mine to have (multiple simultaneous patterns, adjustable pattern length, a very well written .md file etc)!

https://github.com/roryjamesallen/EuroPi/blob/main/software/contrib/euclidian_circles.py

[awonak]

Thanks for the comments clarifying your design decisions and asking for further clarification. Let me dig in a bit.

My current code separates the individual gate display logic and system function display logic:

This is a perfectly logical design decision that sounds reasonable in theory, but the way it is implemented actually creates more problems that it solves. I really like the UI design of the fork @roryjamesallen shared. If you decided to adopt that into your code, that would mean a lot of rewriting of both draw_notes() and draw_box() of SingleEuclideanGate(), as well as the all_box_position and several other variables of EuclideanRhythm. Lots of code changes throughout the entire script! If you follow my suggestion to remove the UI code from SingleEuclideanGate class and make EuclideanRhythm solely responsible for drawing the UI, then that reduces the code changes required and makes everything much more readable and maintainable.

Here is another example to help illustrate that point.

I noticed an opportunity for improvement in a line of code:

self.euclidean_gates[self.gate_idx].notes_para[self.para_idx] += 1
self.euclidean_gates[self.gate_idx].bound_notes_para()

I understand that the intention of this code is to increment the current selected length, fill, or offset parameters of a Euclidean Rhythm, and then ensure the new value is within the appropriate boundaries.

This looks like a great opportunity to help improve the code by using the EuroPi helper method clamp which assigns a value within a given range.

However when I started digging in to propose how to change the code to implement this, I started seeing several design decisions that make this difficult to implement.

1. notes_para() holds several different parameters defined by the self.para_idx so at line 215 we don't know what we're changing because that detail is hidden by the SingleEuclideanRhythm class.
2. the next line calls bound_notes_para() which then changes all parameters boundaries when the previous line only changes a single parameter value.
3. bound_notes_para_final() logic seems to identically duplicate the bound_notes_para() logic, and it's not clear why.

Consider an alternative structure. Earlier I suggested that you would benefit by having a class dedicated to a single Euclidean Rhythm state that only holds the state of length, fill, offset, etc. You could build a class like the following:

Note: I am omitting validation for the sake of a simple example

class Pattern:
    MAX_LENGTH = 20
    def __init__(self, cv, length, fill, offset):
        self.cv = cv
        self.length = length
        self.fill = fill
        self.offset = offset
        self.pattern = self.bjorklund(length, fill)
...

Now lets rewrite the increment button handler using our new class:

@b2.handler_falling
def para_add():
    # Gate Index 6 controls internal/external clock.
    if self.gate_idx == 6:
        self.external_clock = True
        return

    # Increment the current selected parameter
    pattern = self.euclidean_gates[self.gate_idx]
    if self.para_idx == 0:
        pattern.length = clamp(pattern.length + 1, 0, Pattern.MAX_LENGTH)
    elif self.para_idx == 1:
        pattern.fill = clamp(pattern.fill + 1, 0, pattern.length)
    elif self.para_idx == 2:
        pattern.offset = clamp(pattern.offset + 1, 0, pattern.length)

Notice how much more readable the code is. It's clear what parameter is being changes and what the range of the value should and will be when changed. Also there are no magic numbers (except for self.para_idx, which is acceptable because its usage is clear and isolated to the EuclideanRhythm class).

You could even further improve this by creating helper methods on your Pattern class like this:

class Pattern:
    ...
    def update_fill(self, value):
        self.fill = clamp(value, 0, self.length)


class EuclideanRhythm(EuroPiScript):
    ...
    @b2.handler_falling
    def para_sub():
        ...
        pattern.update_fill(pattern.fill - 1)

If you'd like to continue with this review, I'm happy to keep helping, but I want to set expectations that it's going to be quite a bit of work to get this script into a state I'm comfortable with merging.

[awonak]

Closing due to inactivity.