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

coroutine execution scheduler #712

Merged
merged 37 commits into from Apr 26, 2019
Merged

coroutine execution scheduler #712

merged 37 commits into from Apr 26, 2019

Conversation

@artfwo
Copy link
Member

@artfwo artfwo commented Feb 13, 2019

This PR addresses #129 by adding a module for syncing to an external clock using Lua coroutines. It does not replace the current metro API.

The main rationale for using coroutines is that coroutines are a super-flexible tool for describing musical processes in code in a functional way: they are concurrent by nature, have multiple re-entry points, can execute other coroutines.

The clock module provides 2 main functions:

  • clock.run(function() ... end) - will create a coroutine from the given function and immediately run it. returns coroutine ID that can be used to stop it later.
  • clock.sync(beats) - will pause execution until the given fraction of a beat is reached in time. must be called from within a coroutine launched with clock.run.

Helper functions:

  • clock.sleep(s) - pause coroutine execution for s seconds.
  • clock.stop(coro_id) - stop execution of a coroutine started using clock.run
  • clock.set_source(source) - select the sync source, currently clock.TEMPO and clock.MIDI

Synchronisation: clock.sync schedules coroutine wakeup events using clock_reference_t structure in clock.c - the structure stores the count of beats (as double), tempo, and time of last update of the two former numbers. The scheduling logic is implemented in clock_schedule_resume_sync: it basically counts backwards to "beat zero", then calculates the time until the next given fraction of a beat.

Clock sources: this PR only provides tempo and midi (needs more testing). Sources can update the reference at any suitable rate via clock_update_reference_from - it will only accept updates from source selected by clock_set_source.

Example script:
clock_test.lua - with generators
clock_test.lua - without generators

@tehn
Copy link
Member

@tehn tehn commented Feb 13, 2019

wow, this looks amazing.

could you provide a few lua usage examples?

@artfwo artfwo force-pushed the clock-coroutines branch 2 times, most recently from 6cb5ddf to e9886ce Mar 6, 2019
@artfwo artfwo force-pushed the clock-coroutines branch 2 times, most recently from 649e5c7 to e1fafae Mar 26, 2019
@artfwo artfwo force-pushed the clock-coroutines branch from 418cd43 to 83c079a Apr 4, 2019
@artfwo artfwo changed the base branch from dev to master Apr 7, 2019
@artfwo artfwo force-pushed the clock-coroutines branch from 7199f82 to 9524eef Apr 7, 2019
@artfwo artfwo removed the dev label Apr 7, 2019
@okyeron
Copy link
Contributor

@okyeron okyeron commented Apr 27, 2019

This looks awesome. I was trying to figure out how to get bpm for incoming midi clock earlier today and then saw this had been merged.

I'm playing with the sample script and can't quite get midi clock synced up with my TR-09

adding a 3rd tick box and then this:

params:add_option("clock", "clock", {"midi", "internal"}, 1 and 2)
params:set_action("clock", function(x) start_midi_clock(x, 3) end )
function start_midi_clock(x, y)
  clock.set_source(x)  
  tasks[y] = clock.run(tick_generator(y, 220))
end

It tracks tempo from the TR-09 roughly, but is not exactly in time and seems kinda drifty.

Also - How do I set the INTERNAL tempo?

@artfwo
Copy link
Member Author

@artfwo artfwo commented Apr 27, 2019

you don't have any code to stop the tick tasks when you change the parameter value, are you sure that the drifting tracker comes from midi clock and not internal?

Also - How do I set the INTERNAL tempo?

for now you can use a global _clock_internal_set_tempo. it needs a proper namespace and wrapper in clock.lua.

@okyeron
Copy link
Contributor

@okyeron okyeron commented Apr 27, 2019

Not really sure if I'm even approaching this the right way :) Any suggestions would be appreciated.

I just tried mapping a clock.stop(task[3]) to a midi stop - which works. but it seems like a clock.start() is also needed to use with midi start/continue. Or is that clock.resume() ? That doesn't quite work as once I hit stop, clock.threads[coro_id] gets set to nil and wont resume.

A funny thing about midi clock is that it's always running/sending. So I don't understand how clock.run and clock.stop work in that context.

FWIW - I just tired mapping _clock_internal_set_tempo to enc1, but while the value changed, the internal tempo did not change.

bpm = 90
function enc(n, z)
  if n == 1 then
    _clock_internal_set_tempo = bpm + z
    bpm = _clock_internal_set_tempo
  end
end
@artfwo
Copy link
Member Author

@artfwo artfwo commented Apr 27, 2019

clock.run and clock.stop are unrelated to midi, they're only used to manage coroutines, to start and stop tasks that will be synced to "the clock".

not sure why tempo changing doesn't work, i will be able to look into the issue next week.

@okyeron
Copy link
Contributor

@okyeron okyeron commented Apr 27, 2019

I'm happy to help out with testing on this (esp midi). I've just got to wrap my head around how it all works.

Query: Would a clock.get_source function be helpful? I had a moment where I wasn't sure which way it was set and thought that might be good to have.

@artfwo
Copy link
Member Author

@artfwo artfwo commented Apr 27, 2019

yeah, it would be nice to have a global (system-level) parameter for source, so we can get its value at any time.

i won't be able to help you much in the coming few days as i will not have good access to the internet, but will be able to look into any upcoming issues towards the end of next week.

@okyeron
Copy link
Contributor

@okyeron okyeron commented Apr 27, 2019

bookmarking this for later: Internal Clock seems to always be 120 due to this (?):

clock_internal_set_tempo(120);

@okyeron
Copy link
Contributor

@okyeron okyeron commented May 29, 2019

I have some time later this week and want to revisit testing the clock co-routines.

Has the above clock_internal_set_tempo(120); been looked at?

@artfwo
Copy link
Member Author

@artfwo artfwo commented May 31, 2019

@okyeron this is the default tempo that's set during initialization, it's supposed to be like this. in your code above you assign the bpm value to the function: _clock_internal_set_tempo = bpm + z instead of _clock_internal_set_tempo(bpm + z) and that's probably why you aren't getting the tempo changed.

@okyeron
Copy link
Contributor

@okyeron okyeron commented May 31, 2019

Aha! Fantastic. Thanks for that clarification.

@okyeron
Copy link
Contributor

@okyeron okyeron commented May 31, 2019

@artfwo I'm still having some issues changing the tempo. Trying the following code - the tempo appears to stay at 120/default.

Does the _clock_internal_set_tempo need to be "applied" in some fashion in clock.run or with clock.sync?

function init()
  bpm = 60 
  _clock_internal_set_tempo(bpm)
  clock.run(function()
    while true do
      redraw()
      clock.sleep(1/60)
    end
  end)
end
function enc(n, z)
  if n == 1 then
    newbpm = bpm + z
    _clock_internal_set_tempo(newbpm)
  end
end
@artfwo
Copy link
Member Author

@artfwo artfwo commented May 31, 2019

Does the _clock_internal_set_tempo need to be "applied" in some fashion in clock.run or with clock.sync?

no, i think the problem here is that you're not updating the global(?) bpm variable in the encoder handler.

@okyeron
Copy link
Contributor

@okyeron okyeron commented May 31, 2019

Apologies - I was simplifying and left that out of the code

bpm = newbpm is updated after _clock_internal_set_tempo(newbpm) in my test script.

@okyeron
Copy link
Contributor

@okyeron okyeron commented May 31, 2019

Started on clock_get_source function and I notice that in weaver.c _clock_set_source is 0-based while pretty much everything else is 1-based. It seems like changing this to 1-based would work better with things like params:add_option (?)

work in progress here: https://github.com/okyeron/norns/tree/clock-get-source

also added a wrapper for internal tempo (although I still can't make _clock_internal_set_tempo work).

clock.internal_set_tempo = function(tempo)
  _clock_internal_set_tempo(tempo)
end
@artfwo
Copy link
Member Author

@artfwo artfwo commented May 31, 2019

there are matching constants in the clock module, that's why the source id is 0-based. i'll have a look at the issue with set_tempo, could be a problem with the resume scheduler.

@artfwo
Copy link
Member Author

@artfwo artfwo commented May 31, 2019

Started on clock_get_source function

i don't think it is really necessary, as the clock source can be a script or a global param, which you normally only have to set every once in a while. for display getting the param value should be enough as well.

@okyeron
Copy link
Contributor

@okyeron okyeron commented May 31, 2019

Gotcha. I'll abandon that for now I guess.

@artfwo
Copy link
Member Author

@artfwo artfwo commented Jun 1, 2019

@okyeron i cannot reproduce the issue with tempo not being changed, do you have any clock.set_source calls in your script? can you share the complete code for the script somewhere?

@okyeron
Copy link
Contributor

@okyeron okyeron commented Jun 1, 2019

Yes I do have a clock.set_source for starting midi. Does that cause a problem?

Gist of the test script I'm using (which is extended from your example script): https://gist.github.com/okyeron/a67b2405c64823cfe4528fa4bde5b32f

@artfwo
Copy link
Member Author

@artfwo artfwo commented Jun 2, 2019

Yeah, your script uses midi clock as sync source. Remove that line to sync to internal clock.

@okyeron
Copy link
Contributor

@okyeron okyeron commented Jun 2, 2019

OK - I'm a bit confused as I thought I would be able to switch between midi clock and internal clock and that my start_midi_clock function (and thus clock.set_source()) would not do anything until you turned midi on in the params. Am I doing that wrong?

If I want to be able to switch between midi and internal - what would you suggest?

@artfwo
Copy link
Member Author

@artfwo artfwo commented Jun 2, 2019

well, your clock source param will be 1 or 2 so it never reaches the value 0, try the following code instead (i hope this mess will clear up when we have these params as matron-wide):

  params:add_option("clock_source", "clock source", {"internal", "midi"}, 1)
  params:set_action("clock_source", function(x)
    if x == 1 then
      print('switching clock to internal')
      clock.set_source(clock.INTERNAL)
    else
      print('switching clock to midi')
      clock.set_source(clock.MIDI)
    end
  end)

without incoming midi clock events, the clock will stick to last known tempo (default in this case), and setting internal tempo will do nothing, i'm sure that's the problem that you're having.

@okyeron
Copy link
Contributor

@okyeron okyeron commented Jun 2, 2019

Thanks for that. However, I must still be doing something else wrong as I can't make the internal tempo change on the running script using _clock_internal_set_tempo(newbpm).

I added some debug to clock_internal_set_tempo and noticed this - which seems a bit odd to me - interval_nseconds is not getting any precision from interval_seconds (since that's coerced to (uint64_t) first)?.

bpm 23.000000
interval_seconds 10.434783
interval_nseconds 10000000000
bpm 22.000000
interval_seconds 10.909091
interval_nseconds 10000000000
bpm 23.000000
interval_seconds 10.434783
interval_nseconds 10000000000

Also - apologies if I'm being a bit thick. My lack of knowledge here is showing.
Why I'm excited about this - I've made a little host-host adapter and I'm keen to see if I can sync two norns together.

@artfwo
Copy link
Member Author

@artfwo artfwo commented Jun 3, 2019

good catch, the fix is coming.

I've made a little host-host adapter and I'm keen to see if I can sync two norns together.

i don't know how a host-host adapter helps that, but we also have an ableton link clock source in the works for timesyncing norns+norns or norns+PC.

@okyeron
Copy link
Contributor

@okyeron okyeron commented Jun 3, 2019

Looking forward to Link support for sure.

Basically with 2 hosts you can't just send USBMIDI between them with a simple USB cable. I built a little adapter with a couple of Teensy LC's and tested this out with 2 RasPi's tonight (one running orca the other norns). Did not try clock sync between them yet, but it should work.

There's a commercial host-host adapter from a company called SevillaSoft that does this, but I wanted to try DIYing one.

@artfwo
Copy link
Member Author

@artfwo artfwo commented Jun 3, 2019

midi clock source has a few problems now - needs a smoothing buffer for averaging interval measurements and doesn't react to "continue" event.

@Dewb
Copy link
Contributor

@Dewb Dewb commented Oct 12, 2019

Finally getting the chance to dig deeper into this. Looks awesome and will be a huge help on my poly arp project! Using it to trigger crow right now. 🐦

It looks like lua errors inside coroutines are silently lost. Is there an easy fix for that?

Also, like @okyeron I'm also not observing any change after calling _clock_internal_set_tempo(). Tried the suggested param definition, and changing it in script init.

@artfwo
Copy link
Member Author

@artfwo artfwo commented Oct 12, 2019

It looks like lua errors inside coroutines are silently lost. Is there an easy fix for that?

Good catch! Yes, coroutine.resume will return false in case of an error, so we can intercept this as follows: #900

Can look into tempo setting problems later as well.

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

Successfully merging this pull request may close these issues.

None yet

4 participants