Allen-Synthesis / EuroPi

EuroPi: A reprogrammable Eurorack module based on the Raspberry Pi Pico
Creative Commons Zero v1.0 Universal
431 stars 84 forks source link

Updates to Pam's #292

Closed chrisib closed 1 year ago

chrisib commented 1 year ago

Some improvements & bug-fixes to Pam's. Still in progress, but changes of note so far:

NOTE: because of the changes to the clock mod & wave shape menus, you may need to delete your existing saved_state_PamsWorkout.txt file from the module to avoid errors.

Interesting trivia with the new swing implementation: setting the swing to 0% or 100% effectively squashes one of every two notes out of existence, meaning you can get some interesting rhythmic interactions with odd-length Euclidean patterns. Not a design goal, but an interesting side-effect of the implementation.

awonak commented 1 year ago

Would you mind keeping this in Draft mode or holding of on submitting a PR until it's ready for us? This will help us know when it's ready to review and merge and limit the number of emails we get.

chrisib commented 1 year ago

Sorry, I thought I was done and then someone asked about transposition on discord, so I added that.

I think it's ready to go now though.

roryjamesallen commented 1 year ago

Update: I just did a replace all with k2 for k1 and it seems to work although I haven't done extensive testing. More importantly though, it does seem wayyy more intuitive and easy to use quickly

chrisib commented 1 year ago

I've taken a stab at the changes you proposed, including the K1 changes.

With regards to the original Y and N inputs, I did that just because those are what "real Pam's" does for the reset action. Seemed like a good place to start, but OK/Cancel is probably clearer.

roryjamesallen commented 1 year ago

I've taken a stab at the changes you proposed, including the K1 changes.

With regards to the original Y and N inputs, I did that just because those are what "real Pam's" does for the reset action. Seemed like a good place to start, but OK/Cancel is probably clearer.

Perfect fix for reset, I didn't mean it for mute too though, I think that is still clearer as either True/False or Y/N or Yes/No, something that relates to the fact it's a boolean. I think the mute and reset are different if I understand it as reset is a button that does an action, whereas mute is a boolean setting that remains after editing

roryjamesallen commented 1 year ago

I've just had a go and I can't seem to set an output to have knob as a wave type. It works perfectly as a modifier to other parameters like skip% but having it work as a single voltage output is really useful in my opinion, as it can be used in combination with other features like the AIN can be - a knob being quantized and used as a pitch output or used as a continuous voltage to control a filter etc

roryjamesallen commented 1 year ago

I've just had a go and I can't seem to set an output to have knob as a wave type.

I got this to work in my version by adding

Underneath line 147 add: WAVE_KNOB = 5

To the list WAVE_SHAPE_LABELS add: "KNOB"

Underneath line 843 copy the function for AIN but change the name:

elif self.wave_shape.get_value() == WAVE_KNOB:
    if rising_edge and not self.skip_this_step:
        wave_sample = CV_INS["KNOB"].get_value() / MAX_OUTPUT_VOLTAGE
    else:
        wave_sample = self.previous_wave_sample
roryjamesallen commented 1 year ago

And I've already made the bytearray for a knob for it! bytearray(b'\x06\x00\x19\x80 @@ @ \x80\x10\x82\x10A @\xa0 @\x19\x80\x06\x00')

chrisib commented 1 year ago

And I've already made the bytearray for a knob for it!

Could you please send me the PNG (on Discord is fine) so I can add that to the docs folder too?

chrisib commented 1 year ago

I've also just made another branch that's exactly the same as 4f7513a, except that it doesn't use the KnobBank class anymore; it's just always reading the value directly from K2 for menu navigation: https://github.com/chrisib/EuroPi/tree/unlocked-knob

Can you please try that out and see if it makes the menu navigation easier? With everything on one knob now I think this may actually be easier than going back and forth all the time to unlock the knob as you change menu levels.

Assuming the consensus is that no KnobBank is easier, I'll merge that branch into the MR.

roryjamesallen commented 1 year ago

I've also just made another branch that's exactly the same as 4f7513a, except that it doesn't use the KnobBank class anymore; it's just always reading the value directly from K2 for menu navigation: https://github.com/chrisib/EuroPi/tree/unlocked-knob

Can you please try that out and see if it makes the menu navigation easier? With everything on one knob now I think this may actually be easier than going back and forth all the time to unlock the knob as you change menu levels.

Assuming the consensus is that no KnobBank is easier, I'll merge that branch into the MR.

I do personally find the non knobbank version easier to navigate, but I'm happy to accept it if other people disagree. Maybe there's an inbetween where the distance the knob needs to turn to be 'unlocked' could be smaller if that's an option when using the bank?

chrisib commented 1 year ago

After using it some more I've come to the decision that I don't really like how the KnobBank was working, so I've removed it.

chrisib commented 1 year ago

I'm waffling. I've re-added the KnobBank, but in a slightly modified form. Now it locks on all menu levels, instead of just 2. That makes it a little more consistent-feeling (to me at least). But it does mean you'll sometime need to sweep the knob to unlock it.

For the untrained, the KnobBank class basically implements multiple knob interfaces. Every time you choose the next item from the bank, the previous one is basically locked at its current location. Until the knob is swept back to that location the value will not change.

The advantage here is that when you apply a setting the screen stays on the current display, allowing you to more easily confirm that you set it correctly. Sometimes items with lots of options will jitter because of noise on the ADC, and you may be off-by-one. It's nice to be able to immediately see & correct that, without needing to scroll all the way back to whatever setting you were just modifying.

The down-side is, as always, that you'll often need to move the knob back and forth to unlock it when changing levels. I'm honestly torn which I find more annoying. Last night it was the scrolling, but this evening it was definitely the off-by-one and not being able to see that I actually set whatever it was the way I wanted.

I think that since Pam's was released initially with the KnobBank implementation that it's better to leave it there, if only for consistency. We're already breaking the existing user interface by moving to a single-knob system, so leaving the rest alone feels like a reasonable step. Maybe later we can revisit the KnobBank implementation and find a way to make it less intrusive (e.g. just moving the knob a certain amount will unlock it, regardless of the previous position).

chrisib commented 1 year ago

For anyone reading the MR notes & isn't on Discord (IDK who that might be, but just in case) I think the ADSR mode is the final new feature for this update. I wasn't going to include it now, but the MR hadn't been merged yet, so I figured there was time to squeeze in one last useful feature.

I think at this point it's just down to testing & fixing any bugs that come up.

Please let me know if there are any further changes you'd like me to make. I'm hoping to record a video sometime later this week that's a deeper dive into the features of this script. Just waiting for another couple of modules to come in first.

roryjamesallen commented 1 year ago

ADSR works, but at 100 for attack or decay a divide by zero error occurs and the module freezes. You can just unset it from 100 and it'll continue but definitely does need fixing!

roryjamesallen commented 1 year ago

The dynamic settings for waveforms is great, I think this'll be really nice in future if anything gets added/modified. Does phase impact the ADSR type wave?

chrisib commented 1 year ago

at 100 for attack or decay a divide by zero error occurs and the module freezes

Fixed that. Instead of removing 100% as an option I've fixed the math so that if attack is 100% the module basically outputs a ramp wave. If decay is set to 100% you get the expected AD envelope, where the decay will go down to the sustain level, but there's no actual sustain or release phases anymore.

If you set decay to 100% and the sustain level to 0% you should be able to create an asymmetrical triangle wave.

Does phase impact the ADSR type wave?

It was intended to, but I forgot to actually apply it. That's been fixed now too.

edit Formatting.

roryjamesallen commented 1 year ago

It all seems perfect to me, happy to wait for others to test but as far as I can tell this is ready to merge!

chrisib commented 1 year ago

Emergency bug-fix added to address the out-of-phase issue that was reported on Discord earlier. 2/3 users report it fixes the issue, the third hasn't tried yet. But I'm confident.