LinkClock quantum handling and meter sync #4484

jamshark70 · 2019-07-09T08:03:49Z

Purpose and Motivation

We should not set Link's quantum when setting the SC clock's beatsPerBar. This PR adds quantum and quantum_ so that the user can control it directly (and not change it at an inappropriate time).
- A valid use of quantum_ is: If you know, before performing a piece, that it will be in 3/4 time throughout, you can do clock.quantum = 3 on all SC LinkClocks, and Link will synchronize the barlines.
- But, if you change meter during the performance, you can't change quantum (or beats will be discontinuous), and the discrepancy between quantum and beatsPerBar means that LinkClocks to join later may or may not be barline-synced.
To handle discrepancies, users may now call syncMeter_(true) on a LinkClock and it will do two things:
a. Look for other SC LinkClocks on the network that are also set to sync meter, and align the local beatsPerBar and baseBarBeat to match. Currently playing tasks will keep going as they were (there is no disruption in beats or beat durations), but future scheduling relative to bars (e.g. quant = -1) will be in sync with the other SC peers.
b. Future meter changes will be broadcast to other syncMeter peers.

A use case for syncMeter is: You have a performance that requires a meter change midway through (so, at some point, beatsPerBar and quantum will be different). Suppose then that an SC client fails during the performance and has to rejoin the network. Without syncMeter, it isn't guaranteed to be aligned with the other SC peers -- but, if that machine rejoins with TempoClock.default = LinkClock.new.syncMeter_(true) then it's all taken care of automatically.

Types of changes

Documentation
New feature
Breaking change ("technically" a breaking change in that, previously, setMeterAtBeat would set Link's quantum, and this PR removes that. But it was a mistake before and users shouldn't be relying on it.)

To-do list

Code is tested (PR includes unit tests)
All tests are passing
Updated documentation
This PR is ready for review

jamshark70 · 2019-07-11T06:12:50Z

I had tested previously with multiple sclang instances on one machine. Now tested with two machines, working well.

geoffroymontel · 2019-07-26T10:39:23Z

Dear James. I've just tried this branch to sync Ableton Live with SC and it's working great ! Thanks for this really valuable addition !

jamshark70 · 2019-07-27T08:08:49Z

@geoffroymontel thanks for testing! Glad to hear that it's working.

dataexcess · 2019-10-07T10:48:40Z

Hi,

Just wondering why this awesome feature is not yet merged to the master branch? I am now creating all my projects on this branch, but therefor missing out on all the latest features and bufixes :(...

mossheim · 2019-10-07T14:30:58Z

hi @dataexcess, the answer is pretty simple -- we have a backlog of PRs waiting to be reviewed and merged right now. Someone will get to this one eventually!

In the meanwhile, you can get the other features and bugfixes for yourself by rebasing this branch on the latest develop and rebuilding.

dataexcess · 2019-10-07T16:33:24Z

hi @dataexcess, the answer is pretty simple -- we have a backlog of PRs waiting to be reviewed and merged right now. Someone will get to this one eventually!

In the meanwhile, you can get the other features and bugfixes for yourself by rebasing this branch on the latest develop and rebuilding.

Ahh of course! thnx for the tip.

jamshark70 · 2019-10-08T01:05:02Z

I am now creating all my projects on this branch...

Out of curiosity, which feature(s) are you using? Asking because this PR is relevant only when changing beatsPerBar on the fly.

If you're always in 4/4, then Link should sync the barlines without an extra syncMeter.

If you're always in 3/4, I believe in the current release, you can set beatsPerBar to 3 in all peers before starting the performance -- currently this sets the quantum as well, which may break some beat durations but that's ok if you haven't started playing yet.

If you are changing beatsPerBar midstream, then you need this branch. I'm pretty sure, if you're not doing that, you don't really need the new stuff here.

dataexcess · 2019-10-08T07:25:23Z

I am now creating all my projects on this branch...

Out of curiosity, which feature(s) are you using? Asking because this PR is relevant only when changing beatsPerBar on the fly.

If you're always in 4/4, then Link should sync the barlines without an extra syncMeter.

If you're always in 3/4, I believe in the current release, you can set beatsPerBar to 3 in all peers before starting the performance -- currently this sets the quantum as well, which may break some beat durations but that's ok if you haven't started playing yet.

If you are changing beatsPerBar midstream, then you need this branch. I'm pretty sure, if you're not doing that, you don't really need the new stuff here.

My bad! LinkClock is included in the bleeding edge build, and I was running the '3.10.3 - apple notarization signed build'. 🤦‍♂️ I wasn't really worried about these features.. I thought this was the main LinkClock branch. Nonetheless, changing beatsPerBar midstream would be cool to be able to do :)

mossheim

haven't looked at the test code yet

SCClassLibrary/External/Ableton/LinkClock.sc

mossheim · 2019-12-24T02:26:58Z

SCClassLibrary/External/Ableton/LinkClock.sc

@@ -1,6 +1,8 @@
 // clock with Ableton Link synchronization

 LinkClock : TempoClock {
+	var <syncMeter = false, <id,
+	meterChangeResp, meterQueryResp;


typically in class files if instance variables are spread across multiple lines, each line starts with a var.

also, why is id publicly accessible?

Finally working through these.

also, why is id publicly accessible?

Now it's a deliberate decision -- if id is to be user-configurable in the new class, then it should also be accessible. (Your instinct to ask about it was right, though -- I had exposed it mainly for debugging during development, so there wasn't a good reason. Now there is.)

SCClassLibrary/External/Ableton/LinkClock.sc

mossheim · 2019-12-24T02:33:08Z

SCClassLibrary/External/Ableton/LinkClock.sc

@@ -11,6 +13,169 @@ LinkClock : TempoClock {
 		).prInitFromTempoClock(clock)
 	}

+	init { |tempo, beats, seconds, queueSize|
+		super.init(tempo, beats, seconds, queueSize);
+		id = 0x7FFFFFFF.rand;  // to ignore 'self' queries


should be configurable. what if you have multiple clients running with the same randSeed?

That's a good point (though I'd suggest, in a typical use case, that users should initialize clocks across the network before pulling random numbers for musical purposes).

I can also imagine an ensemble may just assign each member an id that they use every time, so, making it configurable is a good idea.

Between configuring this and network ports (which aren't a part of Link's protocol, nor of normal clock operation), I'm thinking it would be better to split meter sync into a separate class (to avoid god-object design). So instead of LinkClock.new.syncMeter_(true), you might do:

l = LinkClock.new etc; m = SCLinkClockMeterWatcher(l);

... and address commands to the separate sync object. I think it makes more sense than cramming tangentially related options into LinkClock itself.

I'm thinking it would be better to split meter sync into a separate class (to avoid god-object design)

yes, that's a great idea! i hadn't even considered that

Ok, I'll try to do that next week, and incorporate the other recommendations at that time. (I generally agree with the other review comments, won't reply to them all individually but I'm noting them to include in v2.)

sounds good! fyi, when there are a lot of comments on a PR i find using the 👍 reaction and/or "resolve conversation" buttons helpful to keep track of what has and hasn't been dealt with yet.

(sorry if you already do that, i've seen some people not do it and with PRs this big it can generate quite the sprawl)

mossheim · 2019-12-24T03:04:44Z

SCClassLibrary/External/Ableton/LinkClock.sc

+				// So we have to do a two-step check.
+				if(bpbs.size == 1) {
+					// 'choose' = easy way to get one item from an unordered collection
+					newBeatsPerBar = bpbs.choose;


pop is better, i think. choose randomly probes the backing array until a valid item is found, while pop will linearly scan. since items are inserted linearly into the backing array, pop will return after checking the first index while choose may not

Good catch, will do.

mossheim · 2019-12-24T03:06:12Z

SCClassLibrary/External/Ableton/LinkClock.sc

+						// myBeats = replies.choose[\queriedAtBeat].round(round);
+						// theirPhase = bibs.choose;  // should be only one
+						newBase = baseBeats.choose;
+						if(verbose) { "syncing meter to %, base = %\n".postf(bpbs.choose, newBase) };


this could be newBeatsPerBar

i'm generally skeptical of printing informative text by default. can you talk a little bit about this one?

this could be newBeatsPerBar

Good point -- and now that it's .pop (which modifies the set), it has to be.

i'm generally skeptical of printing informative text by default.

There is one concrete reason to post. Suppose you already have one SC LinkClock with a meter sync object with beatsPerBar = 3, and on another sclang instance, you start another LinkClock, with meter sync, and default settings (beatsPerBar = 4). Meter sync demands that the new clock join the 3/4 meter (and, if the network is functioning, it will), but that isn't what the code explicitly requested. So the user may have thought she was asking for normal 4/4 time, but ended up with 3/4 (because meter sync means "your requested meter, or others' meter if available"). Maybe it's better to do that silently, but I think it's better to advise the user.

I could change this to if(verbose and: { newBeatsPerBar != clock.beatsPerBar }) perhaps.

mossheim · 2019-12-24T03:09:20Z

SCClassLibrary/External/Ableton/LinkClock.sc

+						// calculate baseBarBeat such that my local beatInBar will match theirs
+						// myBeats = replies.choose[\queriedAtBeat].round(round);
+						// theirPhase = bibs.choose;  // should be only one
+						newBase = baseBeats.choose;


again pop. also, the comment above doesn't really make sense to me

mossheim · 2019-12-24T03:09:54Z

SCClassLibrary/External/Ableton/LinkClock.sc

+						this.prSetMeterAtBeat(bpbs.choose, newBase);  // local only
+					} {
+						// this should not happen
+						Error("Discrepancy among 'syncMeter' Link peers; cannot sync barlines").throw;


perhaps the error message should be different to distinguish it in case and severity from the one below?

mossheim · 2019-12-24T03:11:21Z

lang/LangPrimSource/SC_LinkClock.cpp

+    if (!clock) {
+        error("clock is not running.\n");
+        return errFailed;
+    }


this matches surrounding code but would prefer to see some of this factored out if you have time, the same 5 lines are repeated in every primitive here

I don't mind to change it, but how should it be refactored? If there's a getClockInstance() function, then you could pass in either g->sp or g->sp - 1, easy enough, but then... if you return this, then the calling function still needs to have an if block to post the error and return. Or, leave the stack access as is and the factored-out function could handle the null pointer case -- but the calling function would still need an if block to return the error code to its caller.

Sure, code duplication is usually a red flag, but at my admittedly low C++ skill level, I don't see how factoring it out will make it more concise (and if it doesn't make it more concise, then what's the point?).

one simple idea -

LinkClock* tryGetLinkClock(PyrSlot* clockSlot) { auto* clock = static_cast<LinkClock*>(slotRawPtr(slotRawObject(clockSlot)->slots + 1)); if (!clock) { error("clock is not running.\n"); } return clock;

int prLinkClock_GetQuantum(...) { if (auto* clock = tryGetLinkClock(g->sp)) { SetFloat(g->sp, clock->GetQuantum()); return errNone; } else { return errFailed; } }

another idea, kind of like a continuation, doesn't contain an extra branch like the idea above. if we had std::optional with a monadic interface we could use map()

// Expected signature for F: `int(LinkClock*)`, returning an error code template <typename F> void getLinkClockAndThen(PyrSlot* clockSlot, F&& continuation) { auto* clock = static_cast<LinkClock*>(slotRawPtr(slotRawObject(clockSlot)->slots + 1)); if (clock) { return continuation(clock); } else { error("clock is not running.\n"); return errFailed; } }

int prLinkClock_GetQuantum(...) { getLinkClockAndThen(g->sp, [](const LinkClock* clock) { SetFloat(g->sp, clock->GetQuantum()); return errNone; }); }

int prLinkClock_SetQuantum(...) { getLinkClockAndThen(g->sp - 1, [g](LinkClock* clock) { double quantum; if (slotDoubleVal(g->sp, &quantum)) { return errWrongType; } else { clock->SetQuantum(quantum); return errNone; } }); }

you could even get it down to something like

int prLinkClock_GetQuantum(...) { return tryGetLinkClockField(g->sp, &LinkClock::GetQuantum); } int prLinkClock_SetQuantum(...) { return trySetLinkClockField(g->sp, &LinkClock::SetQuantum); } int prLinkClock_GetLatency(...) { return tryGetLinkClockField(g->sp, &LinkClock::GetLatency); } int prLinkClock_SetLatency(...) { return trySetLinkClockField(g->sp, &LinkClock::SetLatency); } // at which point you may as well refactor it so you can write this: definePrimitive(..., "_LinkClock_GetQuantum", &tryGetLinkClockField<LinkClock::GetQuantum>);

but that would depend on type assumptions that are a bit fragile.

happy to explain any of this further if you'd like me to. also feel free to not do any of it in this PR.

I ended up taking the first, simplest option. I agree that it reads better.

I'm not interested enough in C++ to wrap my head around that last one 🤣 The continuation does make sense to me (since I'm accustomed to first class functions in sclang) but I lean towards a simpler C++ idiom. (One reason, for instance, why I haven't touched supernova code is that I just don't understand it, and don't have time or interest to get there.)

I'm not interested enough in C++ to wrap my head around that last one 🤣

tbh, i find it a little frustrating that you asked me to help you understand how to refactor this, and are now saying you're not interested in understanding it after i made an attempt to explain it.

I lean towards a simpler C++ idiom.

so do i, which is why i prefer the solution that uses standard library features to eliminate a large amount of duplicated code.

Oh, now I see the "reply" field... it wasn't there before.

I apologize for causing offense -- I had read the last couple of suggestions like "well, you could do this but haha of course we won't," and my intent was to respond in a similarly lighthearted way, but I see how my wording could be read differently. (I might also suggest that PR review comments could ideally stick closer to what is required to resolve the PR, without an expectation that anything extra would be taken with the same weight. The only parts that I said I wasn't interested in were rather exotic suggestions that we would never have used.)

I incorporated the first of those suggestions -- agreed that it reads better this way.

Quantum is not the same as SC's 'beatsPerBar'. Ableton/link#66 (comment)

Per Ableton's advice, we should not change quantum while playing. Quantum and beatsPerBar are not the same concept.

The problem reproduces sporadically but the test seems to hit it fairly reliably under multiple trials

Automatic transmission of meter changes from one machine to all 'syncMeter = true' peers is not finished yet

Without this change, queryMeter might call the action without waiting for a Condition to hang, breaking any thread that's waiting on it. Should be consistent: *always* defer the reply.

A new LinkClock's timing is not stable immediately. When we allow time for it to stabilize, this scheduling must be on a different, stable clock. This solves an issue with improper barline sync.

…ctor

Tested by simulating a 75% failure rate of message delivery. Passed.

…etwork

Old way: Flag passed as an argument New way: Private method for local changes; main method for broadcast

mossheim

@jamshark70 this is not a review of the entire PR -- if you'd like to wait, i will probably review the rest tomorrow

mossheim · 2020-01-04T23:28:28Z

SCClassLibrary/External/Ableton/SCClockMeterSync.sc

+// but this may in theory apply to any clock that answers to
+// baseBarBeat_ (e.g. MIDISyncClocks, or BeaconClock)
+
+// TODO OSC message prefix with SC_


this appears to be done now

Ah, forgot to remove that. Will do in the next round.

mossheim · 2020-01-04T23:29:55Z

SCClassLibrary/External/Ableton/SCClockMeterSync.sc

+// TODO OSC message prefix with SC_
+
+SCClockMeterSync {
+	var <clock, <>id, <ports;


i don't think id should be settable once this object is constructed -- otherwise you leave open the possibility for it to be changed while messages are in flight. same with ports. it's probably simpler (i.e., less error prone) in those cases to just require users to construct a new object.

mossheim · 2020-01-04T23:32:27Z

SCClassLibrary/External/Ableton/SCClockMeterSync.sc

+	repeats_ { |value|
+		if(value.isKindOf(SimpleNumber).not or: { value < 1 }) {
+			Error("Invalid number of repeats '%'".format(value)).throw;
+		};


the value check i understand, but not the class validation.. isn't it more smalltalk-y to allow any object here that works like a number?

Ideally yes, but:

( c = Complex(1, 0); c.isNumber.debug("Complex is a number"); fork { c.wait }; ) Complex is a number: true -> a Routine ERROR: Message 'wait' not understood. RECEIVER: Instance of Complex

We're missing a polymorphic interface for isWaitDuration. I'm not quite sure how else to handle it in the context of this PR.

Incidentally SimpleNumber is the only number-like class that defines wait.

In the backend, threads are rescheduled if they yield something where slotDoubleVal() succeeds. As far as I can see, that eventually gets around to:

template <typename numeric_type> inline int slotVal(PyrSlot* slot, numeric_type* value) { if (IsFloat(slot)) { *value = static_cast<numeric_type>(slot->u.f); return errNone; } else if (IsInt(slot)) { *value = static_cast<numeric_type>(slot->u.i); return errNone; } return errWrongType; } ... and... inline bool IsFloat(const PyrSlot* slot) { return slot->tag == tagFloat; }

So we should allow only slots that are tagged float or int (i.e. SimpleNumber). Anything else will cause the thread to hang indefinitely. And, anythingElse.wait will throw a DoesNotUnderstand error, also causing the thread to hang indefinitely. So, this is ugly, but it might be the safest way to ensure the thread proceeds.

that's fair. i guess my concern is, do we want to start auditing every bit of code in the class library for this? when i pointed out that the thread could end up erroring and hanging indefinitely, i wasn't thinking it meant it had to be avoided at all costs. i was noting it to see if you had any thoughts or opinions on it.

if you think this needs to be addressed this way, then i think we need to mitigate technical debt:

open a new issue describing the problem

agree on a workaround (what you have here seems fine)

comment everywhere the workaround is used so that when the issue opened in (1) is resolved, the workarounds can be easily spotted and replaced with the solution

in some order - compile a list of places where this safeguard is needed, and solve the underlying issue, and then safeguard unsafe waits relying on user-given values

I guess, if that's what is required, then we might as well just let it error out. That's more in keeping with SC's general attitude of allowing users to shoot themselves in the feet. (I did misunderstand your comment -- I thought you were asking for prevention.)

It makes me a little curious about garbage collection for hanging threads. In this case, if the wait time is valid, the queryMeter thread is reachable via the clock's queue array. The thread is running an open function whose containing scope has received a callback function from the resyncMeter thread, and that function refers to a Condition which contains the resyncMeter thread. I suppose, if the queryMeter thread timeout is not scheduled (by error or by inappropriate yield value), then the whole construct is unreachable and GC-able. If that's the case, then I'm fine with letting it die and then it's up to GC. But if we will have these stray actions hanging about forever, then we should more actively prevent it.

I guess, if that's what is required, then we might as well just let it error out. That's more in keeping with SC's general attitude of allowing users to shoot themselves in the feet. (I did misunderstand your comment -- I thought you were asking for prevention.)

up to you. i think that 'attitude' is less an explicit choice and more often lack of time to make things as robust as we'd like. which is understandable.

But if we will have these stray actions hanging about forever, then we should more actively prevent it.

they will be reachable since they are not freed, so they will not be garbage collected. you could protect {} the wait and free in the cleanup function.

they will be reachable since they are not freed, so they will not be garbage collected.

Oh gosh, good catch, yes, that should be protect-ed.

My question is really about the threads, though, for which there is no destructor. I would assume that a thread is either executing (can't be GC'ed) or scheduled on a clock (referenced from a queue, can't be GC'ed) or yielded a value that prevents rescheduling, in which case... there might be no references? (Unless the user is keeping one.) I suppose it must be like that; otherwise we'd be leaking memory like crazy.

mossheim · 2020-01-04T23:34:09Z

SCClassLibrary/External/Ableton/SCClockMeterSync.sc

+
+// TODO OSC message prefix with SC_
+
+SCClockMeterSync {


naive question -- why is there is SC prefix on this class name?

Link peers may include SC LinkClocks and clocks or timelines from other applications. This class doesn't (can't) try to sync any non-SC peers. The "SC" prefix is meant to clarify that it's only for us -- SC clocks, not "clocks" in general.

I didn't call it LinkMeterSync because I believe that it might work with the Utopia quark's BeaconClock as well. (BeaconClock promises to sync beats, but not barlines.)

Actually, I did test that just now -- BeaconClock needs to add numPeers { ^addrBook.peers.size } but after that, SCClockMeterSync works correctly in all respects. 😮

So I guess I should remove references to Link from the messaging (because it is actually a more general solution).

btw. why didn't you call it ClockMeterSync?

The "SC" prefix is meant to clarify that it's only for us -- SC clocks, not "clocks" in general.

that makes sense to me. thank you for taking the time to make this suitably generic! that's not fun work

btw. why didn't you call it ClockMeterSync?

that's.. the exact question i asked.

Btw I'd welcome suggestions for other names for this class. For the 17 years I've been using SC, I've been consistently terrible at naming classes and methods, with no sign of any improvement. If you can think of something that reads better, I'd be happy to hear it. (Sincere request... SCClockMeterSync is rather clunky and I wouldn't mind changing the name.)

I think that the difficulty in naming is an objective difficulty because programming exceeds the capacity of language. The choice is either to be as explicit as possible (and look horrible) or delegate to the background knowledge of the reader (and in lucky rare cases, find an elegant expression). I would go for the former in rarely/internally used names, the latter for everyday code.

We don't usually make it explicit that only SC is concerned, only if something is specifically for something else, so I find the SC a bit redundant. But I don't mind too much. Apart from this, the name seems fine. Alternatives might be SharedMeter / SharedMetre / SharedMetrum, NetMetre if you look for something shorter.

On further consideration, MeterSync is probably fine and leave it to documentation for the specifics. The one thing I might add to the documentation is to be clear about the fact that MeterSync assumes it's the clock's responsibility to sync beats and tempo (no TempoClock support, because a/ different machines, if beats aren't synced, you can't sync barlines at all and b/ same sclang instance, there's no reason to have two TempoClocks running identically -- just use the one).

mossheim · 2020-01-04T23:39:47Z

lang/LangPrimSource/SC_LinkClock.cpp

+    if (!clock) {
+        error("clock is not running.\n");
+        return errFailed;
+    }


I'm not interested enough in C++ to wrap my head around that last one 🤣

tbh, i find it a little frustrating that you asked me to help you understand how to refactor this, and are now saying you're not interested in understanding it after i made an attempt to explain it.

I lean towards a simpler C++ idiom.

so do i, which is why i prefer the solution that uses standard library features to eliminate a large amount of duplicated code.

The meter sync object's setMeterAtBeat should be private.

The user may pass in bad values for timeout factors. In case of error, use protect to clean up the OSCFunc.

(This will already catch most bad values, and it isn't good OOP style to check specific classes.)

mossheim

couple general comments:

i think MeterSync.sc now needs to be moved to a folder other than External/Ableton. if it's meant to be a generic solution, it should be installed even if someone builds without Ableton support.
can you please revisit these tests and see if you can make them run any faster? we need to be as economical as possible with the time it takes to run the test suite. these are (eventually) going to run on every CI build.

HelpSource/Classes/LinkClock.schelp

testsuite/classlibrary/TestLinkClock.sc

mossheim · 2020-01-12T07:03:29Z

testsuite/classlibrary/TestLinkClock.sc

+				};
+				cond.signal;
+			}.fork(clock2);
+			20.do {


since you use it in 4 places, probably good to give 20 a meaningful name

testsuite/classlibrary/TestLinkClock.sc

mossheim · 2020-01-12T07:07:17Z

testsuite/classlibrary/TestLinkClock.sc

+				};
+				this.assertEquals(
+					list.count { |bool| bool },
+					list.size,


it would be simpler to keep an integer variable that you increment for every successful trial, and then assert that its value is equal to numTrials.

mossheim · 2020-01-12T07:07:40Z

testsuite/classlibrary/TestLinkClock.sc

+			outerCond.unhang;
+		});
+		outerCond.hang;
+		1.0.wait;


not sure i understand why you wait here

mossheim · 2020-01-12T07:08:13Z

testsuite/classlibrary/TestLinkClock.sc

+
+	test_LinkClock_sync_meter_propagates_meter_changes {
+		var clock1 = LinkClock.new.enableMeterSync,
+		clock2, resp, cond = Condition.new, result;


you never use result

mossheim · 2020-01-12T07:08:39Z

testsuite/classlibrary/TestLinkClock.sc

+			this.assert(false, "Make sure no other LinkClocks are running while testing");
+		};
+		1.0.wait;
+		clock1.stop; clock2.stop;


please put separate statements on separate lines. also, same question about the wait here.

mossheim · 2020-01-12T07:11:08Z

forgot to mention in my review notes -- everything else in the C++ and sclang code looks good to me. thanks for your patience and hard work on this, it's a huge feature! @telephon maybe you want to give it a look too? i'm not an expert when it comes to clock-related things.

jamshark70 · 2020-01-13T01:29:08Z

i think MeterSync.sc now needs to be moved to a folder other than External/Ableton. if it's meant to be a generic solution, it should be installed even if someone builds without Ableton support.

OK, into SCClassLibrary/Core (alongside Clock.sc)? But it doesn't apply to any classes in Core... Or Control? DefaultLibrary? Or maybe delete it from here and make it a quark.

Not sure what is best.

Previously, we waited a second to see if peers would join. Now, we use a \numPeers notification to init immediately when peers are available. This should speed up unit tests.

jamshark70 · 2020-01-13T02:31:59Z

Except for moving MeterSync.sc, I think I got them all...?

mossheim · 2020-01-13T02:57:24Z

OK, into SCClassLibrary/Core (alongside Clock.sc)? But it doesn't apply to any classes in Core... Or Control? DefaultLibrary? Or maybe delete it from here and make it a quark.
Not sure what is best.

i don't have a strong opinion, but SCClassLibrary/Common/Control seems like a decent place for it. Core seems to be for more, well, core functionality, and DefaultLibrary doesn't seem to have much in it at all. Control has classes like node allocators and the server watcher, which seem like similar classes to me in that they mediate between sclang and some other process.

jamshark70 · 2020-01-14T03:39:45Z

OK, I moved the file into Common/Control.

Also, after editing the unit tests yesterday, I tried them again today: a/ after deliberately breaking MeterSync (so that it doesn't change the clock's state), unit tests fail; b/ after restoring the proper code, unit tests pass.

To speed up unit tests, I reduced the timeout to look for Link peers to 0.25 instead of 1.0. The first LinkClock to set up MeterSync must wait this long. I haven't tested how long it takes Link to find peers on a second machine. I expect it will be faster than 250 ms, but I might be pushing the lower limit here. Should I just raise it to 0.5? That would add 0.25 sec to the two unit tests related to meter sync but would be a little safer for users.

With that said... I'm afraid I just discovered a race condition.

Machine A does LinkClock.new.enableMeterSync and sets a different meter or barline.
Machines B and C do LinkClock.new.enableMeterSync within 0.25 seconds of each other.
a. Machine B reads A and C, with different meters/barlines --> failed.
b. Machine C reads A and B, with different meters/barlines --> failed.

To be correct, once one machine does enableMeterSync, other machines should block until that's finished, and each machine's request should listen only to the ones that have already finished meter sync. That is, A sets itself up. Then B syncs to A (while C waits), then C syncs to them both.

Before I try to hack something up, let me ask the computer-science people in the room if there are any known approaches for taking a lock across the network? Or do we just document this as something to be careful about?

Or change the logic so that the first clock to enable meter sync becomes the winning answer?

mossheim

thanks!

mossheim · 2020-01-14T04:38:00Z

w/r/t to the other two issues you mentioned, let's tackle them in different tickets/PRs. this one is in a good state already.

edit -- that is to say, i'd really like to not do another round of review in this thread.

joshpar · 2020-01-14T04:59:30Z

Across the network, @jamshark70 - I can't think of anything like that which would be standard or within the realm of our work / codebase. I think a warning is fine. I agree with @brianlheim - let's get this in for now and consider these cases more in another PR if it looks like it is needed. Seems like an issue of looking for trouble before we would know there IS trouble? IS there a race condition that you find? If so - I imagine a more useful model would be master / slave relationships (like in MIDI)... I can't think of anything that would create that kind of checking in networking code.

jamshark70 · 2020-01-14T05:43:04Z

that is to say, i'd really like to not do another round of review in this thread.

Heartily agreed 😆

Seems like an issue of looking for trouble before we would know there IS trouble? IS there a race condition that you find?

I can reproduce it like this:

// A. sclang running in one terminal window
l = LinkClock.new.latency_(s.latency).setMeterAtBeat(3, 1).enableMeterSync;

// B. In SCIDE (launch IDE first so that this one has port 57120)
(
o = OSCFunc({
	{
		l = LinkClock.new.latency_(s.latency).enableMeterSync;
	}.defer(0.15);
}, '/sc_startedLink').oneShot;
)

// C. A second command line sclang (other terminal window)
l = LinkClock.new.latency_(s.latency).enableMeterSync; NetAddr("127.0.0.1", 57120).sendMsg('/sc_startedLink');

B and C both throw "LinkClock peers disagree on beatsPerBar; cannot sync barlines."

If so - I imagine a more useful model would be master / slave relationships (like in MIDI)... I can't think of anything that would create that kind of checking in networking code.

Link is deliberately not designed after this model. One benefit of a "network of peers" is that any machine can crash, restart, and rejoin the network without disrupting the others -- whereas, in a master/slave model, if the master crashes, then you're toast.

One thing I can imagine is to keep a registry of the order in which MeterSync object IDs became active, and then sync to the oldest one (instead of the current logic, which looks for consensus).

joshpar · 2020-01-14T05:51:40Z

Link is deliberately not designed after this model. One benefit of a "network of peers" is that any machine can crash, restart, and rejoin the network without disrupting the others -- whereas, in a master/slave model, if the master crashes, then you're toast.

One thing I can imagine is to keep a registry of the order in which MeterSync object IDs became active, and then sync to the oldest one (instead of the current logic, which looks for consensus)

Makes sense - but does it have an API for you to plug in to that manages that? If it doesn't, I'm not sure that is SC's job - seems like a feature request for the Link devs (in my opinion) that, once it is architected, SC could create an implementation for.

jamshark70 · 2020-01-14T06:36:13Z

Makes sense - but does it have an API for you to plug in to that manages that? If it doesn't, I'm not sure that is SC's job - seems like a feature request for the Link devs (in my opinion) that, once it is architected, SC could create an implementation for.

I discussed it with them. I'm pretty sure they won't do that.

Ableton/link#66 (comment)

Unfortunately Link can not provide continuous beat times when changing the quantum...

Here is what Link does: The quantum is used to align the downbeat to other peers that use the same quantum. As I said in the other thread, the quantum is only used when calculating the local state. It is never shared on the network and quantum changes are not orchestrated with other peers. It is used to determine a grid that tells us how to align the timelines of the individual peers, but there is no "master" timeline for all peers. And the point the individual timelines are aligned to, is opaque from the API.

So in the examples above the new timeline for the new quantum is gap-less in case the change coincidentally happens at a beat time, that is a downbeat for both, the old and new quantum. But there is no way to assert that as a user of the API.

Here is how we deal with a related scenario in Live: We set the quantum when starting playback. In case the time signature in the arrangement timeline changes, we still query the beat time using the initial quantum and just iterate beat time from there - accepting that the phase of the new time signature might not match phase with peers using the same time signature.

... which is what this PR does: when SC changes beatsPerBar, we (now) do not set the quantum. Then it becomes our responsibility to manage baseBarBeat (which MeterSync does).

I'm not sure I completely agree with their reasoning (they could define the Link timeline as beat, phase, quantum and baseBarBeat, instead of only the first three), but from their answer, I don't read anything like "we are thinking about how to improve this." Rather, it looks like "this is how it is, and you have to work with it."

jamshark70 added enhancement comp: class library SC class library labels Jul 9, 2019

nhthn added this to General in Future Release Aug 4, 2019

mossheim suggested changes Dec 24, 2019

View reviewed changes

jamshark70 added 18 commits January 1, 2020 13:12

lang/classlib: Add separate methods to get/set quantum

a4a35c5

Quantum is not the same as SC's 'beatsPerBar'. Ableton/link#66 (comment)

classlib: LinkClock: Remove quantum_ from setMeterAtBeat

5eba96c

Per Ableton's advice, we should not change quantum while playing. Quantum and beatsPerBar are not the same concept.

Help: Document new get/set quantum methods

eba1407

TestSuite: TestLinkClock: Check for beat continuity when changing meter

d32f741

The problem reproduces sporadically but the test seems to hit it fairly reliably under multiple trials

TestSuite: Rewrite TestLinkClock beat duration check

cf8ad63

classlib: LinkClock: Add queryMeter method and supporting code

055ca15

classlib: LinkClock: Add resyncMeter method and support

5420483

Automatic transmission of meter changes from one machine to all 'syncMeter = true' peers is not finished yet

classlib: LinkClock: Respond to meter changes without a full resync

da462be

classlib: LinkClock: queryMeter always threads its response

e0da686

Without this change, queryMeter might call the action without waiting for a Condition to hang, breaking any thread that's waiting on it. Should be consistent: *always* defer the reply.

classlib: LinkClock: Auto-syncMeter schedules on SystemClock

1cc64c5

A new LinkClock's timing is not stable immediately. When we allow time for it to stabilize, this scheduling must be on a different, stable clock. This solves an issue with improper barline sync.

classlib: LinkClock: Remove debugging posts and clean up verbosity

08d53a3

classlib: LinkClock: resyncMeter automatically calculates rounding fa…

93b98a5

…ctor

classlib: LinkClock: Delete outdated comment

46d578d

classlib: LinkClock: Reduce wait time for auto-syncing meter

3c61c4c

TestSuite: TestLinkClock: syncMeter_(true) test

7319238

classlib: LinkClock: Improve resyncMeter's fault tolerance

a8cb721

Tested by simulating a 75% failure rate of message delivery. Passed.

TestSuite: TestLinkClock: Add test for meter changes syncing across n…

40d7b61

…etwork

classlib: LinkClock: Refactor setMeterAtBeat local vs broadcast

9d9ccea

Old way: Flag passed as an argument New way: Private method for local changes; main method for broadcast

lang: LinkClock.cpp: Remove trailing whitespace

370e382

mossheim suggested changes Jan 4, 2020

View reviewed changes

jamshark70 added 7 commits January 5, 2020 10:06

Classlib: SCClockMeterSync: setMeterAtBeat -> prSetMeterAtBeat

343d430

The meter sync object's setMeterAtBeat should be private.

Classlib: SCClockMeterSync: Remove id_ and ports_ setters

3acf9b6

Classlib: SCClockMeterSync: Free OSCFunc even if an error occurs

dd94bba

The user may pass in bad values for timeout factors. In case of error, use protect to clean up the OSCFunc.

Help: Rename SCClockMeterSync file -> MeterSync.schelp

f6c660d

Classlib: Rename SCClockMeterSync -> MeterSync

d49ba40

Classlib: Rename SCClockMeterSync.sc -> MeterSync.sc

9cbd144

Classlib: MeterSync: Check repeats/delta values, but not class

b9a828c

(This will already catch most bad values, and it isn't good OOP style to check specific classes.)

mossheim suggested changes Jan 12, 2020

View reviewed changes

jamshark70 added 4 commits January 13, 2020 09:31

Classlib: Rename LinkClock:syncMeter to isSyncingMeter

844c938

Classlib: MeterSync uses \numPeers to initialize faster

4efa5bb

Previously, we waited a second to see if peers would join. Now, we use a \numPeers notification to init immediately when peers are available. This should speed up unit tests.

Classlib/testsuite: Speed up unit tests

c6aa6ed

Testsuite: TestLinkClock cleanups

aff2460

Classlib: Move MeterSync.sc into Control

9ab1170

mossheim approved these changes Jan 14, 2020

View reviewed changes

joshpar approved these changes Jan 14, 2020

View reviewed changes

joshpar merged commit 5099f50 into supercollider:develop Jan 14, 2020

mossheim mentioned this pull request Jan 15, 2020

[link] aLinkClock.beatsPerBar_() breaks beat continuity #4401

Closed

patrickdupuis moved this from General to Done in Future Release Jan 26, 2020

LinkClock quantum handling and meter sync #4484

LinkClock quantum handling and meter sync #4484

Conversation

jamshark70 commented Jul 9, 2019

Purpose and Motivation

Types of changes

To-do list

jamshark70 commented Jul 11, 2019

geoffroymontel commented Jul 26, 2019

jamshark70 commented Jul 27, 2019

dataexcess commented Oct 7, 2019

mossheim commented Oct 7, 2019

dataexcess commented Oct 7, 2019

jamshark70 commented Oct 8, 2019

dataexcess commented Oct 8, 2019

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamshark70 Jan 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon Jan 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim commented Jan 12, 2020

jamshark70 commented Jan 13, 2020

jamshark70 commented Jan 13, 2020

mossheim commented Jan 13, 2020

jamshark70 commented Jan 14, 2020 • edited

mossheim left a comment

Choose a reason for hiding this comment

mossheim commented Jan 14, 2020 • edited

joshpar commented Jan 14, 2020

jamshark70 commented Jan 14, 2020

joshpar commented Jan 14, 2020

jamshark70 commented Jan 14, 2020

jamshark70 Jan 5, 2020 •

edited

telephon Jan 6, 2020 •

edited

jamshark70 commented Jan 14, 2020 •

edited

mossheim commented Jan 14, 2020 •

edited