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

ii getters that take parameters should return those params #281

Closed
trentgill opened this issue Jan 9, 2020 · 11 comments
Closed

ii getters that take parameters should return those params #281

trentgill opened this issue Jan 9, 2020 · 11 comments
Labels
Milestone

Comments

@trentgill
Copy link
Collaborator

@trentgill trentgill commented Jan 9, 2020

-- get request that requires a parameter
ii.txi.get('param',1) -- here 1 represents the channel being queried

-- effectively calls the below function
ii.txi.event( 'param', 1.934566 )

-- but we need to know what the original arguments were, like
ii.txi.event( 'param', 1, 1.94566 )

This will probably require the user to unwrap themselves like so:

local knobs = {0,0,0,0}
ii.txi.event = function( event, data, device_index )
  if event == 'param' then
    knobs[ data[1] ] = data[2]
  end
end

I'm not super confident of the above approach. The multi-device device_index complicates things as it makes using varargs more difficult, instead probably requiring a table containing the data. For get requests that aren't parameterized, the user shouldn't need to unwrap a table of 1 item...

The data is already available in the ii C driver, but just needs to be plumbed through to the lua environment (might require extending the event datatype).

@ngwese Would love your thoughts on this!

@junklight
Copy link

@junklight junklight commented May 28, 2020

quite honestly - having to unpack the data would be better than anything currently. At least you'd have the value

Can you not just pass the parameter back around so the get parameters also come back to the event?

@trentgill
Copy link
Collaborator Author

@trentgill trentgill commented May 28, 2020

Can you not just pass the parameter back around so the get parameters also come back to the event?

Yes, accessing the data is an easy plumbing job.

having to unpack the data would be better than anything currently

This is what I'm not convinced of without more of a discussion.

I'm hesitant to implement a breaking change like this without some deep consideration & examples of usage. The main thing I'm concerned about is that it complicates the base usecase (ie a standard getter that is not parameterized) in an opaque way. "Why isn't 'data' just the value i asked for?"

In the meantime, the ii communication runs FIFO, so you can manage the state in your script relatively easily:

local chan = 1

function query_txi()
  chan = 1
  for i=1,4 do ii.txi.get('param', i) end
end

local knobs = {0,0,0,0}
ii.txi.event = function( event, data, device_index )
  if event == 'param' then
    knobs[chan] = data
    chan = chan + 1
  end
end

I get that this is not ideal (i hate global state as much as the next person), but I want to make sure we've thought about all the ramifications first. Perhaps the device_index idea needs to be reconsidered, eg: by having a separate 'event' function per address (though this breaks the ability to sequentially index devices by increasing the index. eg. ii.txi.get('param',5) -> returns param 1 of the second device.

@junklight
Copy link

@junklight junklight commented May 28, 2020

ok - that is horrible code wise. slightly tidier than the way I've been doing it but still it relies on your underlying implementation

So my "use case" is largely that I hate magic in code - having the parameter id come though makes it explicit. once it is in then yes it will protect from breaking changes - ie you change it so the ii queue ordering is arbitrary or something

I'd rather not have a separate event function for just the reason you mention.

Currently ""Why isn't 'data' just the value i asked for?"" isn't the thing people are asking :-) they are thinking "how do I get values from different parameters?"

having your snippet above somewhere would be helpful I think at the very least. It would give confidence that it will work (which I certainly wasn't, not being aware that there was a FIFO in there - I'm very untrusting of underlying implementations. People get up to all sorts ;-)

@clayton-grey
Copy link

@clayton-grey clayton-grey commented May 28, 2020

Some random thoughts from a rusty developer:

So some potential tensions are...

  • How can I request and recieve a simple response? (like crow's input voltage)
  • How can I request and recieve an explicit response? (like a TXi param)
  • How can I request a subset of the the data from the device? (eg: all TXi params, but not ins)
  • How can I request all of the data at once? (the entire TXi state)

The rub being you've already rolled everything out with a fixed event handler structure that expects to return 3 values, where in it becomes unintuitive to pass other optional parameters after the device address. And as you cited, it's quite possible to pack many values into an object, but introduces a disparity between a simple request that shouldn't require unpacking, and a complex request that requires more arguments.

Maybe a poor suggestion, but can device_index be an arbitrary final parameter rather than the fixed 3rd parameter? Such that it's just an argument that is assumed in the final position. At which point you could recieve any number of response values in logical repeating pattern where in the final value is always a device address?

In any case, I'll keep thinking about this. I appreciate the code snippet, and also think it might be useful to post elsewhere while this issue is in discussion. It might also be worth noting this limitation in the txi.lua file, as I spent a while this morning and yesterday trying to figure out what I was missing, as was evident by my lines inquiry.

@junklight
Copy link

@junklight junklight commented May 28, 2020

it also occurs to me that you could just wrap the Txi stuff in a bit of a module. People (myself included) would just use it

a = txiparams:new(1) -- 1 being the device id

a:params[1].value or even a:params(1) for value of knob 1

I'd likely not bother thinking about it again :-)

@clayton-grey
Copy link

@clayton-grey clayton-grey commented May 28, 2020

One other point that comes to me as I think about these things, is that unique to crow vs TT, is how frequently the value should be polled to be available on demand? If I am writing a TT script, the polling happens inline in real time, with crow that is not the case. This generally means I'll want to update TXi in a metro, from my early foray here, and it would be nice to have some formal guidance in this regard.

@trentgill
Copy link
Collaborator Author

@trentgill trentgill commented May 28, 2020

What about just tacking them on the end with varargs?

ii.txi.event = function( event, data, device_index, ... )
  -- switch on 'event' for different i2c messages
    -- if that event took no params, just use 'data' as your value
    -- if that event took params, they are accessible:
    local params = {...}
    params[1] -- first param
    params[2] -- second param etc.
end

It seems kinda ugly to me, especially because device_index is an uncommonly used argument. The obvious benefit is that it doesn't break any existing code, though does require some experience working with varargs (ie. need to know to wrap them in a local table).

As it's Lua, arguments in an event handler are all optional, so you only need to type out as many as you need.

//

If one of you writes a module we could consider adding it to the firmware, so users could include it with dofile( txi_module.lua ) or similar. I wonder if it's worth making it generalized across other modules that have numerous continuous controllers (eg. 16n faderbank)?

@clayton-grey
Copy link

@clayton-grey clayton-grey commented May 29, 2020

The varargs thing does feel ugly. A module seems like an aesthetically pleasing approach that also suggests a metaphor for their literal externality? I dunno. Need to write more code!

@trentgill
Copy link
Collaborator Author

@trentgill trentgill commented May 29, 2020

Realizing that while varargs allows support of any number of params, you can still write them in explicitly if you know the number of params you have. In the case of txi it would look like:

local knobs = {0,0,0,0}
ii.txi.event = function( event, value, device_index, channel )
  if event == 'param' then
    knobs[ channel ] = value
  end
end

That doesn't feel too bad to me when you don't have to unwrap the varargs yourself. The ordering of arguments is certainly arbitrary this point and it could be nicer to use a table constructor for the whole function. Something like:

ii.txi.param( 1 ) -- this arg list is saved ie: { 1 }

ii.txi.event = function( e )
  if e.event == 'param' then
    knobs[ e.arg[1] ] = e.value
  end
end

-- ie. the event handler is called with a table:
e = { event = 'param'
    , value = 0..4095
    , device = 1
    , args = { 1 } -- the table of args
    }

The benefit to this approach is you don't have to worry about the order of arguments when writing a script, however you do need to remember the names of those arguments.

Even if you write a module, you're still going to need access to this data in your module, so we need to settle on a solution. Can anyone see any other options (paging @ngwese)? At present it seems we're just choosing between different event handler arguments like so:

  1. ( event, value, device, ... ) where params are tacked on the end.
  2. ( e ) where 'e' is a table with keys: event, value, device, args={}
  3. ( event, data, device ) where data is a table of the return value followed by the args

Personally I'm leaning toward (1), but I could be convinced too go with (2) if there was a compelling reason.

//

Re-reading this all though, one thing that sticks out is the way we match on event, which is the name of the query function that was called. Perhaps the arguments to that query are connected to the event name.

I'm thinking what if we went for:

ii.txi.event( event, value ) -- event is a table!
  if event.name == 'param' then
    knobs[ event.args[1] ] = value
  end
end

-- where
event = { name = 'param'
        , device = 1
        , args = { 1 }
        }

-- note how the above table is just a representation of the initial query:
ii.txi.param( 1 )

-- or explicitly with the device index
ii.txi[1].param( 1 )
       |  |      ^ args
       |  ^ name
       ^ device

'event' is probably the wrong word for the table-arg. Perhaps 'query' to reinforce that it is just telling you which request you made is being responded to?

@junklight
Copy link

@junklight junklight commented May 30, 2020

I like the event thing. More importantly - lots of other things support that model - for example Juce - when you get a mouse event you get a mouse event object with all the context (just using that an example because I'm writing something in it at the moment)

It also fits the principle of least surprise - people will expect to be able to get hold of the context and finding it an event object will meet that.

+1 from me

@clayton-grey
Copy link

@clayton-grey clayton-grey commented May 30, 2020

I agree with that sentiment as well. Getting context from the event seems pretty common with the event models I've used.

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

Successfully merging a pull request may close this issue.

3 participants