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

Crow namespace support #1362

Merged
merged 13 commits into from Jun 29, 2021
Merged

Crow namespace support #1362

merged 13 commits into from Jun 29, 2021

Conversation

trentgill
Copy link
Contributor

@trentgill trentgill commented May 7, 2021

WARNING: needs substantial testing!

This PR removes (almost) all of the manual support for crow on norns. Instead everything is built programmatically so crow feels more like a namespace into a remote device.

It is identical to work with as the prior solution, but doesn't require manual updates whenever the crow API changes.

More discussion TBD.

Requires norns-namespace branch on crow to enable a few missing features.

@trentgill trentgill requested review from tehn and dndrks May 7, 2021 00:42
@trentgill
Copy link
Contributor Author

trentgill commented May 13, 2021

@tehn @dndrks
This PR is ready for testing now. I think it does a really good job of isolating concerns, and separating talking to crow vs talking about crow.

breaking changes:

crow.connected --> norns.crow.connected
crow.add --> norns.crow.add
crow.remove --> norns.crow.remove
crow.receive --> norns.crow.receive
crow.init --> norns.crow.init

crow.send still works, but only for legacy usage. new scripts should use norns.crow.send, or the special special syntax crow(string_of_lua)

new functionality:

  • full crow.public support (example usage forthcoming with new bowering script)
  • norns.crow.loadscript for uploading crow scripts from norns
  • norns.crow.clock_enable() for standard clock lib style clock activation
  • new /core/crow/quote.lua library for stringifying crow datastructures in load()able form
  • norns.crow.public.discovered() event once crow's script has run init() and any public params are discovered
  • norns.crow.public.freezescript() for writing a script to crow's flash, along with the current state of any public params.

concerns:

  • i don't think method calls will work as is (eg. crow.input[1]:reset_events)
  • the asllib helpers need updating (should be easy with a similar quote + metamethod approach)
  • my testing is logically focused, but doesn't pay much regard to real-world usage
  • @tehn suggested we could do a full search through norns-community scripts to find usage of breaking crow.* fns

Also, it's a fun exercise in using metamethods if you want to break your brain a little.

@trentgill
Copy link
Contributor Author

trentgill commented May 13, 2021

Updated crow firmware to support all of the above is here: https://github.com/monome/crow/tree/THREE

Updating crow is not required, but a couple of functions won't work correctly (notably input[1].query()).

Here is bowering which pulls in the newest bowery scripts. Most of those scripts have been updated to work with the public lib to demonstrate how public values are interacted with. You might need to run git submodule init; git submodule update if you install from the command line (unsure what happens via maiden).

The general concept is that bowering is a front-end for the bowery script library which can load any script dynamically (select script with E1, and load with long-hold K1). Once loaded, a dynamic UI will be drawn as defined by the crow script. Select editable param with E2, and change value with E3. Tables can be edited the same way, plus holding K3 and turning E3 will select the table element to be edited. public vars can also be sequins, in which case the currently active step will be background-highlighted (selected edit step is text-highlighted). If you have a set of params you like, you can 'freeze' them onto crow (along with the script), by holding K2 ('FREEZE' will appear) + longhold K1 --- now you can disconnect norns, and crow will boot with that config each time.

https://github.com/whimsicalraps/bowering

@dndrks
Copy link
Member

dndrks commented May 17, 2021

raaaad -- it felt like magic to load a crow script through bowering. excellent work, trent!!!

i dug into euclidean this afternoon and my initial experiences seem like good leads into asking a few q’s on both sides of the bowering + namespace system. so many great possibilities here, but hoping some some naive perspective helps:

  • loading the euclidean script, i wasn’t sure what was required to make things go, until i looked at the leading comments for the file — maybe a mechanism could be added to surface the helper text on the norns screen, same as when loading a script via SELECT?
  • as i played with euclidean, i immediately found myself wanting to midi map the values -- since norns can dynamically hide and show a script's midi-mappable parameters (using params:hide(id), params:show(id) and _menu.rebuild_params()), could public parameters be added to this layer of the system? as a user (both playing with scripts and scripting for crow), this feels like it'd be a huge value-add.
  • euclidean features this public param: public{fills = {4,5,9,12}}. when these values go negative, it totally crashes crow + freezes norns until crow's USB connection is physically removed. meanwhile, crow is sending tons of error messages to matron like:
     crow:	pt:59: in field 'change'
     crow:	?: in function 'change_handler'
    
    so, i tried to sort out how i'd clamp these values, but range seems to work only on single variables? i tried public{fills = {4,5,9,12}}:range(1,16) but that still allowed me to go negative. all to say, if there's a way to clamp a table to a range, that'd be awesome!
  • after the crash i had with euclidean, if i reconnected my crow without power-cycling, maiden reported:
    >>>>>> norns.crow.remove 2
    >> ttyACM found, but not a crow
    dev_list_add: error allocating device data
    
    norns.crow.connected() returned that crow was indeed not connected, so i power-cycled my case. but if i were running the script without a laptop handy, i wouldn't have known that crow was totally crashed -- i'm wondering if there's any benefit / opportunity to have bowering use norns.crow.connected() to confirm to the user that a crow is actually connected? maybe just a lil' crow sprite in the bottom-right corner?
  • also, curious if there's a graceful way for crow to crash without flooding norns, so that at least the user doesn't think norns also requires a white button/pull power panic?

lots of thoughts about a very tiny sliver, but hopefully helpful q's + reports?

@tehn
Copy link
Member

tehn commented May 22, 2021 via email

@ngwese
Copy link
Member

ngwese commented May 22, 2021

@trentgill - maiden will fetch submodules when installing from a git repo. it wasn't an explicitly implemented feature, just library behavior i discovered and continue to use

@tehn
Copy link
Member

tehn commented Jun 23, 2021

breakage list:

crow.connected

  • buoys
  • nmMelodyMagic

crow.add

  • caliper

crow.remove

crow.receive

crow.init

  • yggdrasil
  • patchwork
  • cheat_codes_2
  • awake-passerby
  • arcologies

lots of crow.send use (typically for ii commands)

accomplished via this weird script to clone everything in maiden catalog: https://gist.github.com/catfact/5521e15a66bc39474782f8751319460f

then using ag for each search

@tehn
Copy link
Member

tehn commented Jun 23, 2021

@dndrks any additional testing on this would be great

@trentgill last bit to figure out is asllib ?

@dndrks
Copy link
Member

dndrks commented Jun 23, 2021

results same between using crow v3.0.0b0 + the latest commit to THREE (v3.0.0b0-2-g4d22604)

test 1: crow norns clock + input change (fails)

seeing errors in matron as I attempt to clock norns (on 1362)

crow:	bad argument #2 to 'tell' (string expected, got boolean)
crow:	stack traceback:
crow:	[C]: in function 'tell'
crow:	?: in field 'tell'
crow:	repl:1: in field 'change'
crow:	?: in function 'change_handler'

maybe obvious, but confirming that tempo doesn't change either :)
ran into the same with testing input change, as might be expected

test 2: input stream (works!)

function process_stream(v)
  print("input stream: "..v)
end

crow.input[1].stream = process_stream
crow.input[1].mode("stream", 0.25)

prints input stream: 4.310925, etc to matron

test 3: generic i2c to JF (works!)

crow.ii.jf.mode(1)
crow.ii.jf.play_note(3)
crow.ii.jf.play_voice(5,3/12,10)
crow.ii.jf.trigger(0,1)
etc

test 4: specific i2c JF mode getter (fails)

script:

jf_mode = nil

crow.ii.jf.event = function( event, value )
  if event.name == 'mode' then
    jf_mode = value
  end
end
  
function init()
  crow.ii.jf.get('mode')
  print(jf_mode)
end

matron:

crow:	bad argument #2 to 'tell' (string expected, got table)
crow:	stack traceback:
crow:	[C]: in function 'tell'
crow:	?: in field 'tell'
crow:	repl:1: in field 'event'
crow:	?: in function 'ii_LeadRx_handler'

test 5: crow.send + norns.crow.send (works)

norns.crow.send("ii.wsyn.play_voice(1,3/12,5)")
norns.crow.send("ii.wsyn.play_voice(1,3/12,0)")
^ works same as
crow.send("ii.wsyn.play_voice(1,3/12,5)")
crow.send("ii.wsyn.play_voice(1,3/12,0)")

@trentgill
Copy link
Contributor Author

@dndrks @tehn
fixed most of these issues. requires new crow THREE branch, and pull this crow-namespace-support branch, and bowering too (which requires submodule update).

loading the euclidean script, i wasn’t sure what was required to make things go, until i looked at the leading comments for the file — maybe a mechanism could be added to surface the helper text on the norns screen, same as when loading a script via SELECT?

i've just disabled the script preview for now. it wasn't helpful in it's terse form, and often was too long, overwriting the script name. a deeper version that previewed like the norns script loader would be nice, but too much work before the 3.0 release.

euclidean features this public param: public{fills = {4,5,9,12}}. when these values go negative, it totally crashes crow + freezes norns

this is fixed now. :range method clamps table values to the range limits on norns itself.

breakage list:

thanks for compiling this! i can just open PRs with those scripts as the changes are super fast.

not worried about crow.send persisting, but i want to push the docs toward norns.crow.send or just the crow shortcut so future scripts are clearer.

test 1: crow norns clock + input change (fails)
test 4: specific i2c JF mode getter (fails)

these are fixed with the updates. i haven't tested the clock handling specifically though, and i'm guessing it expects the value to be a number, but now change sends a boolean. if this is an issue i can fix it.

last bit to figure out is asllib ?

i think so! i will try and get it done tomorrow.

@tehn tehn merged commit 4e96902 into main Jun 29, 2021
@tehn tehn deleted the crow-namespace-support branch June 29, 2021 13:18
@@ -198,8 +198,7 @@ function clock.add_params()
function(x)
clock.set_source(x)
if x==4 then
crow.input[1].change = function() end
crow.input[1].mode("change",2,0.1,"rising")
norns.crow.clock_enable()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to also disable clock on crow when the source changes from crow to something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm looking at the code rn and i'm at a loss how it actually works?! the norns.crow.clock_enable was just a tiny abstraction to put that crow code into the crow file. i'm worried the routing of the crow event into the clock system was based on some magic behind the scenes that has been broken here.

@artfwo do you remember where the crow 'change' event hooks into the clock system?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artfwo do you remember where the crow 'change' event hooks into the clock system?

i think the clock handle callback is triggered here:
https://github.com/monome/norns/blob/main/matron/src/device/device_crow.c#L84

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure how this worked previously, but regarding this code:

https://github.com/monome/norns/blob/main/matron/src/clocks/clock_crow.c#L67

does this call only update the global clock if source is set to crow clock reference?

trying to figure out if the crow needs to conditionally call the clock sync based on which clock is enabled--- which is better i think than telling crow to stop sending ^^change which is a generic function and could be connected to other uses

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my reading of the clock code is that it's fine as-is... that all clock sources are independent (?)

@dndrks
Copy link
Member

dndrks commented Jun 29, 2021

@trentgill , i'm still running into trouble with test 1 + test 4 -- same results as previously. my shield is up to date on main and my crow is running the latest build on THREE (v3.0.0b0-7-gf2fded0).

lmk if i can help with any additional testing!

@tehn
Copy link
Member

tehn commented Jun 29, 2021 via email

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

Successfully merging this pull request may close these issues.

None yet

5 participants