clean up metro stage API

[okyeron]

I was trying to sort out how to use stage in a metro today and noticed this in the metro.lua code - stage here sets self.init_stage, but then the C function uses self.props.init_stage

function Metro:start(time, count, stage)
    local vargs = {}
    if time then self.props.time = time end
    if count then self.props.count = count end
    if stage then self.init_stage = stage end
    self.is_running = true
    metro_start(self.props.id, self.props.time, self.props.count, self.props.init_stage) -- C function
end

Is this correct or should it be if stage then self.props.init_stage = stage end?

[tehn]

yes that's a bug and your fix should work!

[okyeron]

FWIW - it seems to work the same either way.

[tehn]

I'll check it out tomorrow!

[okyeron]

Following up on this stage business...

To set stage outside of the start() function, you have to use init_stage. In the example below using k.stage = 10 (instead of k.init_stage) does not work.

local k
function init() 
   k = metro.alloc()  
   k.callback = tst
   k.time = 1/2
   k.count = 15
   k.init_stage = 10
   k:start()
end
function tst(stage)
   print("Hello " .. stage)
end

Should there be something to set up stage = init_stage?

EDIT - which could be accomplished by adding this at line 107: elseif idx == 'stage' then self.props.init_stage = val

[catfact]

@okyeron seems like 2 questions

1) these are different: init_stage - where metro will go on :start() stage - current metro stage, updated on callback, read-only. so in other words, setting stage outside of start isn't intended to work. the C interface doesn't even support it. this metro abstraction is supposed to be dirt-simple; the counter is available as a sanity check but i think more complex sequencing (loop points &c) should be a lua abstraction.

2) i may be confused about the issue since i see the init_stage setter metamethod. https://github.com/monome/norns/blob/master/lua/metro.lua#L106

the :start code is weird-looking but correct. external code should definitely not set props directly but it is ok to do it here inside the module - in metro:start(), it is to avoid multiple calles to the metro_start C function. since init_stage is just a lua data variable, it doesn't matter if the setter is used or not, and it is used by default.

[catfact]

so, i'm kinda not seeing the problem here (except readability) and will now close this pending further problems.

maybe the real request is to allow setting stage number of running metro; but my answer to that would be that i don't like baking this into the low-level layer since there is an inherent race condition there.

[okyeron]

I think my request about init_stage is this:

• if you set the time and count parameters separate from the metro:start() command, you can't set stage the same way. You'd have to use init_stage - which is undocumented. Or you have to set all the variables in the metro:start() command.
• I suppose this makes it difficult if you want to stop and re-start the metro with a different stage.
• This would be when creating the metro, not necessarily on a running one.
• FWIW - i think it'd be more consistent if metro:alloc() included stage

[catfact]

i agree that the API is quite arbitrary and probably unclear.

the intention is kinda that you should usually just restart the metro if you want to change its parameters. for example if you change the time parameter, it will not take effect until the metro's next "sleep cycle." if you do this in a metro callback, the metro has already gone to sleep and the next callback will occur after the old period has elapsed. (i really don't think there's any way around this given that we want maximally precise timings on the callback periods themselves.) if you want to trade precision for "modulability" the best thing to do is to restart the metro in its callback.

if you set the time and count parameters separate from the metro:start() command, you can't set stage the same way.

but, again, metro:stage is a different field with a different meaning, and should not be set! that is exactly why it has no setter. init_stage is the initial stage and is only applied on restart. you can absolutely set it any time if you really want to - TBH i didn't imagine these things to be very useful except for the simplest of applications. (building internal timings into UI widgets, &c.)

i think it would be super confusing to have a setter named after one field (metro.stage =), that sets a different field (metro.init_stage), and for the actual setting to have no effect until restart, while the thing you think you set (metro.stage) continues to change value autonomously.

i think the real issue here is the poor quality of the documentation combined with the admittedly odd structure of the module.

[catfact]

anyways - sorry this is so confused - you want metro:stage to have a setter, right?

that is of course really trivial to do. but here's the problem, and the reason it's not done already: because of the threading model and the chosen tradeoff of accuracy and efficiency, if i say metro:stage = 2 there is no way to know whether the next callback will get :

• stage value 3 (you set the stage=2, the metro woke up, incremented, posted event)

• or whatever it would have been (the next callback is already in the event FIFO for lua to process.)

  i don't want to have to explain this "bug" to people so my answer is, no, there is no setter. if you want an arbitrary sequence, keep track of its state in lua. that's the easy answer. (should this be added to the module? i dunno.)

the "hard" answer would be giving more sequencing information to the timer thread. honestly i don't really see the point though.

i guess a third answer is that maybe the race condition just doesn't bother people, but there are situations where this would lead to bugs in a script (if you set stage number, and at the same time change callback logic assuming that the next callback will increment the stage number you just set.)

[okyeron]

I'm getting pretty confused as this seems a bit deeper than I thought.

I don't really have a use case for metro:stage to have a setter, it just seemed inconsistent based on my limited understanding.

Not super worried about this either way.