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

Chaining Muxlicers doesn't work smoothly #32

Closed
ZwergNaseXXL opened this issue Feb 3, 2022 · 6 comments
Closed

Chaining Muxlicers doesn't work smoothly #32

ZwergNaseXXL opened this issue Feb 3, 2022 · 6 comments
Assignees

Comments

@ZwergNaseXXL
Copy link

ZwergNaseXXL commented Feb 3, 2022

I tried chaining two Muxlicers via EOC -> One-Shot-In connections. Sounds like there's a timing hickup between the two (either way) and also an additional low pitch when going from 1 to 2. When I check the EOC trigger in a scope, it seems to come exactly at the same point as gate 8. Shouldn't it come after gate 8, maybe at the falling edge? Attached screenshot, can't attach saved selection.

grafik

@ZwergNaseXXL ZwergNaseXXL changed the title Chaining Muxslicers doesn't work smoothly Chaining Muxlicers doesn't work smoothly Feb 3, 2022
@hemmer hemmer self-assigned this Feb 3, 2022
@hemmer
Copy link
Collaborator

hemmer commented Feb 3, 2022

Thanks for the report, will take a look.

@hemmer
Copy link
Collaborator

hemmer commented Feb 9, 2022

Confirmed the issue, and have a partial fix. It doesn't yet fix problem when externally clocked though, so I will keep working on it.

From 2c3f723059261536317f413e8afc9c0a2c649d5c Mon Sep 17 00:00:00 2001
From: hemmer <915048+hemmer@users.noreply.github.com>
Date: Mon, 7 Feb 2022 22:00:15 +0000
Subject: [PATCH] WIP fix

---
 src/Muxlicer.cpp | 64 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/src/Muxlicer.cpp b/src/Muxlicer.cpp
index 8f24a0e..aaf6300 100644
--- a/src/Muxlicer.cpp
+++ b/src/Muxlicer.cpp
@@ -173,6 +173,12 @@ struct MultDivClock {
 
 		return multipliedSeconds;
 	}
+
+	void reset() {
+		secondsSinceLastClock = 0.f;
+		dividedProgressSeconds = 0.f;
+		dividerCount = 0;
+	}
 };
 
 static const std::vector<int> clockOptionsQuadratic = {-16, -8, -4, -2, 1, 2, 4, 8, 16};
@@ -254,8 +260,14 @@ struct Muxlicer : Module {
 
 	uint32_t runIndex; 	// which step are we on (0 to 7)
 	uint32_t addressIndex = 0;
-	bool playHeadHasReset = false;
 	bool tapped = false;
+	// Used to track if a reset has been triggered. Can be from the CV input, or the momentary switch. Note
+	// that behaviour depends on if the Muxlicer is clocked or not. If clocked, the playhead resets but waits
+	// for the next clock tick to start. If not clocked, then the sequence will start immediately (i.e. the
+	// internal clock will be synced at the point where `resetIsRequested` is first true.
+	bool resetIsRequested = false;
+	// used to detect when `resetIsRequested` first becomes true (only used when no external clock)
+	dsp::BooleanTrigger detectResetTrigger;
 
 	// used to track the clock (e.g. if external clock is not connected). NOTE: this clock
 	// is defined _prior_ to any clock division/multiplication logic
@@ -430,7 +442,20 @@ struct Muxlicer : Module {
 				internalClockLength = tapTime;
 			}
 			tapTime = 0;
-			internalClockProgress = 0;
+			internalClockProgress = 0.f;
+		}
+
+		// If we have an _internal_ clock, as soon as we get a reset signal (which can come from CV or various
+		// modes of the switch), we will immediately start the sequence and sync up all the various clocks.
+		if (detectResetTrigger.process(resetIsRequested) && !usingExternalClock) {
+			// reset various clocks
+			mainClockMultDiv.reset();
+			outputClockMultDiv.reset();
+			multiClock.reset(mainClockMultDiv.getEffectiveClockLength());
+			// prime the main clock to be ready to receive a clock tick
+			mainClockTrigger.state = false;
+			// set the internal clock to a value that will force a clock tick
+			internalClockProgress = internalClockLength;
 		}
 		tapTime += args.sampleTime;
 		internalClockProgress += args.sampleTime;
@@ -451,20 +476,23 @@ struct Muxlicer : Module {
 		// so we must use a BooleanTrigger on the divided/mult'd signal in order to detect rising edge / when to advance the sequence
 		const bool dividedMultipliedClockPulseReceived = mainClockTrigger.process(mainClockMultDiv.process(args.sampleTime, clockPulseReceived));
 
-		// see https://vcvrack.com/manual/VoltageStandards#Timing
-		const bool resetGracePeriodActive = resetTimer.process(args.sampleTime);
+		// see https://vcvrack.com/manual/VoltageStandards#Timing (only relevant if using an external clock)
+		const bool resetGracePeriodActive = resetTimer.process(args.sampleTime) && usingExternalClock;
 
 		if (dividedMultipliedClockPulseReceived) {
 			if (isAddressInRunMode && !resetGracePeriodActive && playState != STATE_STOPPED) {
+
 				runIndex++;
+
 				if (runIndex >= SEQUENCE_LENGTH) {
 
+					runIndex = 0;
+
 					// the sequence resets by placing the play head at the final step (so that the next clock pulse
 					// ticks over onto the first step) - so if we are on the final step _because_ we've reset,
-					// then don't fire EOC
-					if (playHeadHasReset) {
-						playHeadHasReset = false;
-						runIndex = 0;
+					// then don't fire EOC, just clear the reset status
+					if (resetIsRequested) {
+						resetIsRequested = false;
 					}
 					// otherwise we've naturally arrived at the last step so do fire EOC etc
 					else {
@@ -474,12 +502,10 @@ struct Muxlicer : Module {
 						if (playState == STATE_PLAY_ONCE) {
 							playState = STATE_STOPPED;
 						}
-						else {
-							runIndex = 0;
-						}
 					}
 				}
 			}
+
 			multiClock.reset(mainClockMultDiv.getEffectiveClockLength());
 
 			if (isAddressInRunMode) {
@@ -569,8 +595,8 @@ struct Muxlicer : Module {
 
 		const bool resetPulseInReceived = resetTrigger.process(rescale(inputs[RESET_INPUT].getVoltage(), 0.1f, 2.f, 0.f, 1.f));
 		if (resetPulseInReceived) {
-			playHeadHasReset = true;
-			runIndex = 8;
+			resetIsRequested = true;
+			runIndex = 7;
 
 			if (playState == STATE_STOPPED) {
 				playState = STATE_PLAY_ONCE;
@@ -583,19 +609,19 @@ struct Muxlicer : Module {
 		const bool switchIsActive = params[PLAY_PARAM].getValue() != STATE_STOPPED;
 		if (playStateTrigger.process(switchIsActive) && switchIsActive) {
 
-			// if we were stopped, check for activation (normal or one-shot)
+			// if we were stopped, check for activation (normal, up or one-shot, down)
 			if (playState == STATE_STOPPED) {
 				if (params[PLAY_PARAM].getValue() == STATE_PLAY) {
 					playState = STATE_PLAY;
 				}
 				else if (params[PLAY_PARAM].getValue() == STATE_PLAY_ONCE) {
 					playState = STATE_PLAY_ONCE;
-					runIndex = 8;
-					playHeadHasReset = true;
+					runIndex = 7;
+					resetIsRequested = true;
 				}
 			}
 			// otherwise we are in play mode (and we've not just held onto the play switch),
-			// so check for stop or reset
+			// so check for stop (switch up) or reset (switch down)
 			else {
 
 				// top switch will stop
@@ -604,8 +630,8 @@ struct Muxlicer : Module {
 				}
 				// bottom will reset
 				else if (params[PLAY_PARAM].getValue() == STATE_PLAY_ONCE) {
-					playHeadHasReset = true;
-					runIndex = 8;
+					resetIsRequested = true;
+					runIndex = 7;
 				}
 			}
 		}
-- 
2.31.0

hemmer added a commit to hemmer/Befaco that referenced this issue Feb 10, 2022
…ooltips/bypasses.

Fixes VCVRack#32. Couple of things going on - one is that the 1ms grace period on reset doesn't really work in this context, and breaks EOC chaining which is a common use case. Two, if the clock is already high when a reset was requested, then the boolean trigger detector doesn't detect a rising edge.
@hemmer
Copy link
Collaborator

hemmer commented Feb 12, 2022

Pushed some further changes to address this.

One thing that might remain, but is more of a design thing - the switch always has output something on COMI/O, so after the sequence resets on one of the Muxlicers it will output the signal of the first step (even though it's not running). If you want to chain COMIO outputs, rather than summing you can redirect them into a third switch (which can be another muxlicer!) which selects which COMIO to use, rather than summing as in your example:

image

hemmer added a commit that referenced this issue Mar 8, 2022
* Noise Plethora
  * Initial release
* Chopping Kinky
  * Upgraded to use improved DC blocker
* Spring Reverb
  * Added bypass
* Kickall
  * Allow trigger input and button to work independently
* EvenVCO
  * Fix to remove pop when number of polyphony engines changes (Fixes #33 )
* Muxlicer
  * Chaining using reset now works correctly (Fixes #32 )
@hemmer hemmer closed this as completed Mar 8, 2022
@ZwergNaseXXL
Copy link
Author

I just checked the new version, unfortunately it's not completely fixed. The extra low pitch is gone, the hickup between Mux 2 and Mux 1 is gone, but the hickup between Mux 1 and Mux 2 is still there. Note that I started the patch by starting Mux 1 in one-shot mode.

The purple gates in the scopes are the combined all-gates outputs, the red and blue trigger are the EOC signals.

grafik

@hemmer
Copy link
Collaborator

hemmer commented Mar 12, 2022

Not sure I understand. They work chained, EOC can be used to switch between gate outs. I'm starting with one shot.

selection
muxs.vcvs.zip

image

@ZwergNaseXXL
Copy link
Author

Sorry for the delay and the confusion. Much time has passed and I completely lost track of this. I just tried it again and got slightly different results than in my 2nd screenshot. Then I realized this was only because of the two internal clocks being out of sync. Works fine when I restart VCV or use an external clock. So I can confirm that this is fixed indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants