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):

norns/matron/src/weaver.c

Line 186 in d6c4f94

lua_register(lvm, "grid_set_led", &_grid_set_led);

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.