-
Notifications
You must be signed in to change notification settings - Fork 147
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
[WIP] link: store and access phase, delay transport start #1738
Conversation
...phase getter should probably just lock the mutex and do a full query of micros / phase at that moment.. I suppose. (means moving |
...or: this will sound silly, but could be totally fine to just add a fudge factor to phase. (of, say, twice the sleep period which is 0.1ms, so the reasons i'd consider it are (1) blocking to retreive all the link and session state could be heavy, (2) usually only care about integer component of phase anyway, so priority on correct result after |
Here's a test script I used with your PR: It allows us to start the clock using K2, and stop it using K3. On the first beat the left side of the Grid blinks, on other beats the right side of the Grid blinks. I observe that indeed One weirdness I see is that with this test script I always see Norns starting clock at the 2nd beat of Ableton Live. Is there something wrong in the test script? |
i modified the example from here, like so
only issue i'm seeing is the wonkiness with old phase reporting as noted. i don't have strong opinion about when to fire the transport callback but this seems to work for me. |
@catfact iirc, any sync(x) calls will be delayed until beat 0 anyway on the (local) link timeline. if we want scripts to be link-aware, we can toggle this behavior, so sync(1) will also yield at -3, -2, -1 beats (given that quantum is 4), but then coroutines will need extra checks around sync() calls as below:
right now it's rather working as follows:
does your change solve a different use case? unrelated to the above question, we shouldn't need phase in scripts, as we can always calculate that from current_beat / quantum, but since we have don't have a global "sync rate" and each coroutine inside a script can run at its own (variable) rate, a global phase accessor shouldn't be necessary. for the same reason, we don't need (to know) quantum in most cases too, in norns it only affects the time beat 0 is aligned to (if 2 peers are running with the same quantum it will be identical). |
unless i misunderstand, that's not what i'm seeing. i agree that being able to explicitly query the phase is less important. with or without that, scripts can implement "quantized start" in various ways, but only by specifying different logic when the clock source is Link. the expected user story is that launching a script without link-specific logic (like its absolutely true that i don't entirely know what i'm doing here and have little stake in the outcome, so happy to step out of the discussion. anyways thanks for looking. |
right, that's definitely not by design. perhaps this was broken at some point.
yeah, the solution is to never fire a SYNC_RESUME event at any beat before 0 (consequentially, any active clock.sync(x) for any x should be resumed at beat 0). i would simply add a condition at https://github.com/monome/norns/blob/main/matron/src/clocks/clock_scheduler.c#L96 maybe something like |
if i read correctly, that would mean coro is never run/resumed before beat 0 right? if we want to be really nice and proper, ideally we would still run the coro during the "count-in" part. that's so that if a script does want to be link-aware, it can implement a count-in animation or whatever just as Ableton Live does. hence the thought of effectively decoupling @Dewb shared this useful document from Ableton that sort of covers user-story expectations. |
yes. we can still query beat value and show count-in animations in what i'm suggesting was basically the original intent in this api. it also requires no extra effort for the use-case you describe: if any peer restarts the transport in a link session - awake will restart playback from beat 0 with no script modifications. |
I like the idea to fire early but allow reading that it's count-in and reacting to that per script. Docs would need to be updated to highlight this, and old scripts wouldn't gain this feature without changes, but ultimately rendering the count-in in UI is indeed preferable and makes up for the downsides. |
OK, so I poked at the issue with my example and I found the culprit. Ezra's example doesn't demonstrate the problem I'm describing because it relies on Ableton Live starting the transport. If we add just this to the example script:
then we can start the transport from Norns and observe what I'm talking about.
Note how the first "----" appears at phase "1" and not phase "0". That's because the current documentation and examples all tell us to run Certainly, user scripts could work around this but the count-in idea will be a much better solution. |
it's true that i had really only looked at the case where norns is follower for transport. yes, there is some wonky logic going on (it's WIP, for real), but that description doesn't match the issues i'm seeing and i don't think the initial here is the messy current state of the test script. i'm playing a different pitch on beat 0. when Live is leader, i find the initial when norns is leader, i actually don't see a predictable difference with clock.sync or not (it continues "immediately" i think) but there are some sporadic inconstiencies with when Live picks up the transport, and also with when tempo is applied. omitting the initial sync seems more stable in that case. anyways, i would be perfectly happy if the answer was to do nothing. if @artfwo considers the current behavior (before this change) to be not what was intended, then i would follow their lead for sure. |
ok, so i've updated my norns today and confirmed that transport restart in awake isn't working 100% as it should (without ezra's change). for testing transport syncing i'm using LinkHut example and reference implementation bundled with Ableton Link, see https://github.com/Ableton/link/tree/master/examples/linkhut it doesn't have a lot of dependencies and needs only cmake and some platform-specific headers (portaudio or jack on linux, coreaudio on mac). if using that for testing, make sure that both "link" and "start/stop sync" are enabled, it must display something like this in the status bar:
when stopping the transport from LinkHut, awake stops playback - this is expected. when starting the transport from LinkHut, awake starts playback when the LinkHut metronome begins to tick - this is also expected (provided that both norns and LinkHut quantum is identical). awake won't reset the step position, but i'd say this is an awake issue and should be an easy fix. my immediate thought was to add there's another issue with awake, however - it increments the step position immediately after clock.sync: https://github.com/tehn/awake/blob/main/awake.lua#L147-L154 so, when testing with restructuring the i'd also move the resetting logic into the beginning of that said, it looks like we can leave the transport resetting logic on script's conscience completely here, as (transport) syncing seems to be working correctly to me. adding transport hooks fixes it for awake, if the coroutine is fixed. does anything above make any sense? :) @catfact @ambv |
Ezra, your code reproduces the problem I'm describing. See this video: I'm starting and stopping transport here from Norns. You can see reproducibly that the first audible sound after the transport starts is the second beat, not the first beat. When @artfwo says:
then clearly this isn't currently the case. It pauses until beat 1. In your example, the phases are aligned but that's because you're relying on the new For the record, relevant settings I used in the video are: on Norns, link quantum is 4; on Live you can see in the top bar that Global Launch Quantization is set to "1 Bar". My current preference to how to solve this would be what @artfwo suggested above:
This would allow Norns scripts to implement count-in indication, which would ensure people aren't confused as to why their Norns isn't starting playback right away. And hopefully this would also make it less tricky to actually align sequencers on Norns to start playback at beat 0. On top of this, internal/crow/MIDI clocks to continue working as they are today as they don't have the concept of |
I would prefer if we all focused on the same example. It feels like I'm playing a simultaneous exhibition with all the examples you're both running. Ezra's last example is simple and synthetic, which will allow us to focus on the actual issue.
No, the bug is different in Awake. Look at the original values of So the bug really is that |
good point, i'll have a look at that later today as well.
thanks, good catch. apologies for getting back to the awake example :) so i have updated function stop()
running = false
all_notes_off()
clock.cancel(coro_id)
end
function start()
running = true
reset()
coro_id = clock.run(step)
end
function reset()
one.pos = 0
two.pos = 0
end this seems to work with stock maiden without any modifications when syncing with LinkHut: https://www.youtube.com/watch?v=3OsS8ckx17o does it? am i missing anything here? |
hmm, you're right. in this example, when starting from norns, the initial (with the change, clock.transport.start fires "immediately" when running the routine, but always after the transition to beat zero.) anyways, i don't actually like the delayed transport either. it feels broken as a user expeirence (hit the "start" button, wait with no feedback for an unknown amount of time before actually starting.).(ironically, i suggested from the begining that we not implement this, but was convinced it was wanted.)
yes, it seems to work here too - the other behavior important to link users is norns-as-leader. this seems trickier to me but maybe it's not. small point
as artem points out, it is (or should be) equivalent to calling |
so, is what i'm hearing that we don't in fact need to make any scheduling changes, yeah? maybe we close this PR then (discussion can continue.) and maybe best thing we could do is add some documentation / sample code demonstrating best practice a little better. |
You mean something like: local quantum = params:get('link_quantum')
local pulse_count = math.fmod(math.floor(clock.get_beats()), quantum)
if pulse_count == 0 then
-- new bar
else
-- other beat
end That seems to work but I admit it's somewhat cryptic. So if we're going that way then maybe instead of your PR we want a different PR with just a I can make that PR for you, if you want. |
yeah, something like that. and i am not sure if there are also other places where we would want the system to insert a as a suggestion, and regarding indexing:
use |
that shouldn't be necessary, if coroutines are restarted in the transport start callback. also the first
link is fully peer-to-peer by design, there are no assigned leaders in a session. if start/stop sync is enabled on all peers, it doesn't matter who starts the transport, each peer will have the callbacks triggered and the local timelines realigned. |
You keep repeating this, but this is not what I'm observing. Run this and see for yourself: It's Ezra's example modified to run on the current shipped version of Matron without this PR. Try to start the transport from Norns (press K3) when your other Link device is in phase 2 or 3. You will notice that it immediately starts playing on the next beat, ignoring To work around this, you need to implement a count-in in the coroutine, like: -- Link count in
local clock_source = params:string('clock_source')
if clock_source == "link" then
local quantum = params:get('link_quantum')
while math.fmod(math.floor(clock.get_beats() + 0.4), quantum) ~= 0 do
clock.sync(1)
end
else
clock.sync(1)
end
while true do
-- do work first, and then...
clock.sync(1)
end I still need to fiddle with the details here (the magic number "0.4" for example) but that's the only thing that worked for me. Please confirm that you reproduce this, otherwise we must be talking about some two different setups. |
@ambv could be, here's what i'm observing with your unmodified script under unmodified matron: https://www.youtube.com/watch?v=aM-UO7Hpa_U the behavior looks correct to me, is it not? do you have start/stop sync enabled in both norns and Live as described here? specifically this: |
i am able to make this work as well by modifying the function clock_loop()
local quantum = params:get('link_quantum')
local beat = clock.sync(1)
while true do
local hz
pulse_count = math.floor(beat) % quantum
print(pulse_count)
if pulse_count == 0 then hz = 330
else hz = 220 end
engine.hz(hz)
redraw()
beat = clock.sync(1)
end
end we aren't getting negative beat values here to show remaining beats for count-in, but timing-wise it looks fine. |
ok, so after multiple attempts i was able to get several skips with the K3 case. a few times it began at beat 1 and a few times at beat 3. i think i've forgotten to rewind the local timeline in clock_link, looking into it... |
thanks for looking into it @artfwo again i think it would be wonderful to just have a clearer demonstration of correct practice. and thus maybe identify reasons/ways to encapsulate that. to be clear, my example here is just lightly adapted from the clocks study which is an important reference point and should be fixed if it needs it.
yes thank you for the clarification. what i meant is that i find myself having to do different things depending on whether norns initiates the change to the session transport state or not. closing this PR to make it ultra-clear that it's not the right solution, but comments are still open. |
right, so the above example seems to work with the K3 case after the following change in clock_link.c: diff --git a/matron/src/clocks/clock_link.c b/matron/src/clocks/clock_link.c
index e9ba389a..289cc809 100644
--- a/matron/src/clocks/clock_link.c
+++ b/matron/src/clocks/clock_link.c
@@ -44,7 +44,7 @@ static void *clock_link_run(void *p) {
if (clock_link_shared_data.transport_start) {
- abl_link_set_is_playing(state, true, 0);
+ abl_link_set_is_playing_and_request_beat_at_time(state, true, micros, 0, clock_link_shared_data.quantum);
abl_link_commit_app_session_state(link, state);
clock_link_shared_data.transport_start = false;
} @ambv can you apply this and see if it fixes the problem for you? |
Looking. |
YES! This makes Ezra was skeptical of a solution like this because it potentially waits for close to 4 beats before you can hear playback. I personally think this solution is good because:
|
Ah, forgot to mention, this solution also works better than my count-in attempts with magic numbers to cover for rounding edge cases. |
ah, nice, i think i can prepare a PR for this then. on a slightly offtopic matter, @ambv your profile badge shows you're a python core dev. this summer i've released a similar clock lib for python compatible with asyncio (with only link backend supported due to platform-specific complexities of midi configuration). it's a bit more convenient with python given that you can program coroutine cancellation behavior using exceptions, await for other clock-synced coroutines, use recursion, etc. check it out, if you're using python for any music stuff here: https://github.com/artfwo/aalink |
@artfwo we actually collaborated already: artfwo/pymonome#11 😄 |
just to be clear, i was in fact skeptical of the situation where we delayed transport start callback. that was presented to me as the solution adopted by seamstress. but i think that is inaccurate and seamstress is also using i think the change addresses all our concerns and brings parity between the two environments. if i understand correctly, that's the only place its needed, other places just need to set |
still checking. on the 2nd look it might be that we only need to correct the time in this line: https://github.com/monome/norns/blob/main/matron/src/clocks/clock_link.c#L60 it's right that we only attempt to rewind the local link timeline if start/stop sync is enabled locally (and call transport.start in any case), but it might be that link internals have changed, requiring actual (wall clock) time to be specified when calling abl_link_request_beat_at_start_playing_time, so the actual change will probably be: diff --git a/matron/src/clocks/clock_link.c b/matron/src/clocks/clock_link.c
index e9ba389a..25255fa0 100644
--- a/matron/src/clocks/clock_link.c
+++ b/matron/src/clocks/clock_link.c
@@ -57,7 +57,7 @@ static void *clock_link_run(void *p) {
if (clock_link_shared_data.start_stop_sync) {
if (!clock_link_shared_data.playing && link_playing) {
- abl_link_request_beat_at_start_playing_time(state, 0, clock_link_shared_data.quantum);
+ abl_link_request_beat_at_start_playing_time(state, micros, clock_link_shared_data.quantum);
clock_link_shared_data.playing = true;
// this will also reschedule pending sync events to beat 0
that should be already set by clock.link_start(), the purpose of the structure was to keep data shared between threads in the same basket. |
The last diff is invalid. The API is:
You're trying to put time as a beat. If you want to change 0 into micros, that should happen in diff --git a/matron/src/clocks/clock_link.c b/matron/src/clocks/clock_link.c
index e9ba389a..82b71997 100644
--- a/matron/src/clocks/clock_link.c
+++ b/matron/src/clocks/clock_link.c
@@ -44,7 +44,7 @@ static void *clock_link_run(void *p) {
if (clock_link_shared_data.transport_start) {
- abl_link_set_is_playing(state, true, 0);
+ abl_link_set_is_playing(state, true, micros);
abl_link_commit_app_session_state(link, state);
clock_link_shared_data.transport_start = false; |
right, that should be sorry, been a while since i touched this code 😅 and the change to |
Cool, I'll test the PR when it's up. |
quick attempt to do two things related to ableton link:
EVENT_CLOCK_START
(which triggersclock.transport.start
) until phase 0. this implements the recommended "quantized start" behavior without requiring scripts to track phase (which is a link-specific parameter.)idea is that scripts can still do stuff during "count-in" if they want to specifically be link-aware; if they don't they can still start at the same time as the Live transport.
known issue: there's a problem with this as it stands: the phase is updated in the main link clock routine, which updates every 10ms... and with the way the scheduler works, lua "sees" a 10-ms-old phase value on clock sync. need to find some other time to update the cached link phase.
there could well be other issues, i don't totally grok all the intent in the ableton API even after reading their user-story documentation.