stop the globalocalypse

[catfact]

there are way too many globals and they aren't defined in any particular place. it's a recipe for disaster. not even just in user scripts but when we want to extend or maintain the system.

in the beginning, there was the norns table, and it was global. it had to be, so that the C API glue could see it and call its fields.

then, there were more globals added for convenient usage in scripts and REPL. this was controvesial, but at least they were collected in one place (globals.lua) and commented with ldoc.

now, i don't know what happened to globals.lua but the definition of globals has gone everywhere- many are in startup.lua, which makes sense: https://github.com/monome/norns/blob/master/lua/startup.lua#L9-L31

but many are in script.lua (i think? can't tell! that's the whole problem in a nutshell) https://github.com/monome/norns/blob/master/lua/startup.lua#L9-L31

and probably elsewhere. this is really bonkers because script.lua is otherwise structured as a module!

[pq]

yeah, i see a bunch more.

typing

for k,v in pairs(_G) do print(k..' : '..tostring(v)) end

in the REPL (on a fresh start), yields this list of suspects:

repl out (long)

``` audio_output_level : function: 0x1b104 s_line_rel : function: 0x1b984 grid_rows : function: 0x1bdb0 audio_pitch_on : function: 0x1a514 _G : table: 0x53938 home_dir : /home/we load_engine : function: 0x1b3dc redraw : function: 0x12cf70 poll : table: 0x647b0 s_arc : function: 0x1b74c set_aux_fx_input_level : function: 0x1afb8 s_level : function: 0x1bbdc io : table: 0x55418 audio_monitor_level : function: 0x1b0b4 set_aux_fx_off : function: 0x1a484 gain_in : function: 0x1b534 s_move : function: 0x1bb14 request_poll_value : function: 0x1b1bc bit32 : table: 0x54918 s_line_width : function: 0x1bb90 s_aa : function: 0x1bc28 table : table: 0x54f60 osc_send : function: 0x1aa24 set_insert_fx_on : function: 0x1a474 utf8 : table: 0x574d8 audio_input_level : function: 0x1b154 set_insert_fx_off : function: 0x1a464 grid_all_led : function: 0x1be74 gain_hp : function: 0x1b59c rawget : function: 0x76c0be04 metro_stop : function: 0x1b38c script_dir : /home/we/dust/scripts/ loadfile : function: 0x76c0c674 fileselect : table: 0xd86c0 audio_pitch_off : function: 0x1a504 s_close : function: 0x1bf94 stop_poll : function: 0x1c094 xpcall : function: 0x76c0b8bc s_extents : function: 0x1b5e8 state : table: 0x643d8 debug : table: 0x55c18 startup : function: 0x72de0 audio_monitor_stereo : function: 0x1a544 audio_monitor_on : function: 0x1a534 s_fill : function: 0x1c054 audio_dir : /home/we/dust/audio/ require : function: 0x54558 tape_stop_rec : function: 0x1a4d4 audio_monitor_mono : function: 0x1a554 gridkey : function: 0x12c830 clamped_value : 0.575 error : function: 0x76c0c1d0 wifi : table: 0xd3b78 hid : table: 0x6ae18 restart_audio : function: 0x1a440 _VERSION : Lua 5.3 start_audio : function: 0x1a454 grid_refresh : function: 0x1be14 tonumber : function: 0x76c0bb18 midi : table: 0xa8b90 math : table: 0x568b0 s_rect : function: 0x1b6b0 us : 1528864180 set_poll_time : function: 0x1b20c grid_cols : function: 0x1bd4c os : table: 0x551a8 s_curve : function: 0x1b8c0 s_text : function: 0x1b660 rawlen : function: 0x76c0be44 params : table: 0xe20b0 start_poll : function: 0x1b274 dofile : function: 0x76c0c734 grid_set_led : function: 0x1beec tape_new : function: 0x1b064 mix : table: 0x8fc40 report_engines : function: 0x1a860 grid : table: 0xa16a0 set_aux_fx_return_level : function: 0x1aed4 set_aux_fx_input_pan : function: 0x1af5c controlspec : table: 0x76af0 midi_send : function: 0x1b42c s_circle : function: 0xa3760 set_insert_fx_param : function: 0x1add0 set_aux_fx_param : function: 0x1ae74 set_insert_fx_mix : function: 0x1ae30 send_command : function: 0x1a674 rawequal : function: 0x76c0be9c next : function: 0x76c0c060 tape_pause_rec : function: 0x1a4e4 tab : table: 0x62858 _ : 1 cleanup : function: 0x72ad0 s_text_center : function: 0xa2728 init : function: 0x133568 tostring : function: 0x76c0ba20 s_stroke : function: 0x1c014 g : table: 0xe2550 set_aux_fx_on : function: 0x1a494 paramset : table: 0x746a8 get_time : function: 0x1a564 textentry : table: 0xd6680 engine : table: 0x5b498 osc : table: 0x64ba0 metro : table: 0x99ea8 collectgarbage : function: 0x76c0c250 key : function: 0x72ad0 audio_monitor_off : function: 0x1a524 s_curve_rel : function: 0x1b7fc s_text_right : function: 0xa2710 tape_open : function: 0x1b014 s_line : function: 0x1ba7c tape_start_rec : function: 0x1a4f4 s_move_rel : function: 0x1ba00 osc_send_crone : function: 0x1a870 s_font_face : function: 0x1bcc0 enc : function: 0x132bd8 ipairs : function: 0x76c0c4b0 pairs : function: 0x76c0c490 tape_play : function: 0x1a4c4 screen : table: 0x9f678 s : 986969 util : table: 0x68da0 assert : function: 0x76c0c7bc norns : table: 0x5a740 data_dir : /home/we/dust/data/ setmetatable : function: 0x76c0c334 coroutine : table: 0x54498 usleep : function: 0x1b2cc sound_file_inspect : function: 0x1ad48 load : function: 0x76c0c53c s_update : function: 0x1bfd4 metro_set_time : function: 0x1b324 tape_stop : function: 0x1a4a4 string : table: 0x53a40 select : function: 0x76c0ba48 set_aux_fx_output_level : function: 0x1af18 s_clear : function: 0x1bd0c tape_pause : function: 0x1a4b4 s_font_size : function: 0x1bc74 metro_start : function: 0x1a5a8 print : function: 0x76c0bedc pcall : function: 0x76c0b9a4 type : function: 0x76c0b94c rawset : function: 0x76c0bdb8 package : table: 0x53888 getmetatable : function: 0x76c0c6e4 ```

---

obviously some come from lua core (print, etc.) but yeah, here's to rationalizing the rest. 👍

[catfact]

well a lot of them are c functions defined in matron. I don't actually know if there's a way to collect these in a table from c. If not maybe they should at least have a special naming convention

[artfwo]

can't we just use lint to detect unsanctioned globals in the scripts?

[tehn]

perhaps c functions can use a preceding underscore?

most of the rest of the globals can be cleaned up and put into the norns table. midi and grids still have some possible things to work out re: convenience functions

[pq]

perhaps c functions can use a preceding underscore?

obfuscated names would help for sure. better still would be not to pollute the namespace at all. i'm not up on how the c/lua integration works but my hunch is there's got to be a way to specify an env that's not _G.

finally, assuming we can't make the globals go away we could consider tailoring the env used in scripts to hide or protect access to globals. (poc started in #426.)

[pq]

i'm not up on how the c/lua integration works

a forward pointer for anyone following along (who like me doesn't know their way around quite yet):

https://github.com/monome/norns/blob/d6c4f94d510d42bbd201414fa9ecba3bb8ec984e/matron/src/weaver.c#L186

looking at the lua manual, i see there's a bunch of suggestively named stuff in the C API (lua_newtable, lua_setmetatable, etc.). anyway, maybe the pieces are all there?

(caveat: just browsing around and haven't actually tried anything yet.)

[catfact]

ok, i think the solution would use lua_pushcfunction and lua_setfield

( to register C functions as fields in a single table)

[catfact]

btw i am checking out this book, which is a little tedious but seems to have many useful nuggets https://www.safaribooksonline.com/library/view/creating-solid-apis/9781491986301/

[pq]

user trickyflemming was just hit by this on lines:

I was finally bitten by the globals last night. I was working on a script and I created an array called metro. Hoo boy, norns did not like that. The error text was not useful so it took a bit of hunting to figure out what happened. I had to replace the offending variable, resave the script, and then reboot norns to fix it.

the lvm reset fixes that are in flight would make this a little better (i think metro would get reset on the next script run, or?) but it may be worth talking through something like the shadow ENV idea in #436.

[tehn]

i have LVM reset working now! it's great. just trying to prune unneeded code now

[catfact]

that's cool - but a word of caution - resetting the LVM isn't actually a solution for the main failure case we've seen so far - which is users accidentally overwriting system globals. e.g. this problem: https://llllllll.co/t/norns-development/14073/214?u=zebra

resetting the LVM seems useful for two reasons:

1. it certainly clears out the global namespace between script launches, with a heavy hammer indeed. i have vague reservations about nuking everything between scripts, but we can see how it goes, with special eye on timer and peripheral device management.

2. it could be a useful kind of panic button connected directly to maiden (like, a special websocket command in matron, not thtrough lua), or even to a hardware button press combo. so that you can get out of a broken state without power cycling. (this was what i thought the request was for.)

but for the case where someone makes a global script variable called metro, nuking the LVM state doesn't help at all. for this we still need a sandboxing environment layer for scripts. PR #436 looks pretty good to me for that. it covers the situation where i do a global metro = foo assignment in a script. it unfortunately doesn't cover the situation where i assign to a field of metro without declaring it first: metro[0] = foo. that seems like a hard class of error to guard against given that scripts do need to be able to access fields in the module - it would require some metatable stuff in the metro.lua module itself, to make certain things "read-only." the effort/reward seems not worth it, at this time.

the idea from the forum of adding norns sysgtem globals to maiden syntax highlighting, also seems like a smart thing to do and not too difficult.

[tehn]

i am confused how resetting the LVM wouldn't fix a mistaken renaming of metro.

of course it's not going to fix the broken script, but this to me seems a different issue entirely. either we explicitly require every single script to require everything they need, or have some default globals that users need to know not to overwrite.

that said, #436 seems very elegant, and doesn't have the issue of my current LVM reset PR (re: device detection). is there any downside to #436? if not we should just go with that and abandon this LVM reset business. i should still make an attempt to refine the script loading (which i did in the LVM reset PR)

[catfact]

to be sure i understand correctly, i'm looking at PR #448 and running a script now seems to go like this:

• check that the request file exists
• use Script.savestate() to write the requested filename and system state to system.lua on the filesystem.
• reset LVM. this should run startup.lua, read system.lua, and load the requested script file on startup.

let's say the startup script assigns metro = foo. startup.lua executes metro = require 'metro', then the script says metro = foo, and metros are broken. they would be broken in the same way if we rebooted. this is why system global clobbering is so bad - user script can put the norns in a basically unbootable state with a very simple error.

the only way out of this is not letting user scripts overwrite system globals.

[catfact]

anyway yes, i think PR #436 seems more apropos to the problem at hand, i don't see any downside, and i have other concerns about the lua reset. (see new issue.)

re: dev detect, yes would simply need a new facility in weaver to explicitly request a report of all connected devices. device_scan only reports new devices.