Combine #1650 and #1309

tlubke commented 1 year ago

Not sure if this will the take place of the mentioned PRs, but I figured this is the easiest way to link to both. Did a quick post-merge test recompiling and playing with awake, everything looked and sounded good.

catfact commented 1 year ago

https://github.com/monome/norns/discussions/1615#discussioncomment-4180802

catfact commented 1 year ago

awesome, thank you for taking the time to do that.

i'd like to try things out, is this comment still accurate regarding necessary changes in /boot/ and additional dependencies? https://github.com/monome/norns/discussions/1615#discussioncomment-4180802

tlubke commented 1 year ago

awesome, thank you for taking the time to do that.

No problem, just hope I didn't miss anything in the merge.

i'd like to try things out, is this comment still accurate regarding necessary changes in /boot/ and additional dependencies?

Yup, those instructions should still be accurate.

Is there anything else I can help with to move things along?

tehn commented 1 year ago

this is looking great, thank you! i'll run it through some testing in the next couple days and evaluate how the update will look.

catfact commented 1 year ago

if we merge all these things at once (which seems somehow correct to me, though maybe not for any good reason), then there are still questions to answer about the breaking changes for screen events.

initial comment on this runs down the APIs that are broken or added: https://github.com/monome/norns/pull/1309

but the main one that is actually used a lot being text_extents.

i think deciding how to deal with that is the primary actual blocker for merging the screen events stuff.

i can think of ways to easily work around or even improve on usage of the old APIs, and i think actually the new callback version of text_extents gives more useful information, but i can't really think of a way to transparently support the old API. so that's the big question i have to kick back to @tehn and @dndrks .

next topic. if these changes are merged then i think it also might make sense to also implement something that has been discussed on lines, a C-driven refresh timer. the same thread could drive this timer, perform cairo rendering, make SPI transfers, and initiate callbacks to lua so that scripts can synchronize their rendering requests with the actual refresh rate. that's something that is totally available and straightforward to take on if @tlubke has appetite for it.

whaddayall think?

catfact commented 1 year ago

oh hey i forgot the #1309 thread has plenty of ideas already about how to address common text_extents usages. but i think nothing that doesn't require migration. https://github.com/monome/norns/pull/1309#issuecomment-766940833

tlubke commented 1 year ago

if these changes are merged then i think it also might make sense to also implement something that has been discussed on lines, a C-driven refresh timer. the same thread could drive this timer, perform cairo rendering, make SPI transfers, and initiate callbacks to lua so that scripts can synchronize their rendering requests with the actual refresh rate. that's something that is totally available and straightforward to take on if @tlubke has appetite for it.

Just pushed https://github.com/monome/norns/pull/1717/commits/267012b7213f79547698527f059d84c72b7878cc which has a waf config option (--enable-display-timer-thread) for enabling the current iteration of the timer thread present in #1650.

The thread as-is drives the timer and makes the SPI transfers. It could also initiate Lua callbacks, but I'm wondering if another entry in the screen_event_id_t enum would make more sense. That way it could be more portable to alternate displays, rewrites, and desktop norns (the driver is dependent on screen_events.h, rather than the Lua system being dependent on the driver).

If the thread in ssd1322.c is disabled in the build, then the thread in screen_events.c takes on the burden of SPI transfers when screen_update() is called. The refresh should be close to instant (in the context of screen events at least), limited to the hardware refresh rate which I believe is currently set near 120hz.

To me, the separation makes sense, but I'm open to hearing some arguments against it. Could the exchange from ssd1322 thread -> screen event -> lua callback be slow or inconsistent enough to undermine its usefulness?

tehn commented 1 year ago

revisiting the text_extents issue.

current scripts using it explicitly:

tehn@obsidian:~/local/allnorns$ rg -l text_extents
euclidigons/euclidigons.lua
phonotype/phonotype.lua
arcologies/lib/graphics.lua
flora/lib/modify.lua
flora/lib/plant.lua
flora/lib/flora_pages.lua
moln/hack/lib/norns_engine_tester.scd
moln/hack/moln_new.scd
oblique/lib/mod.lua
midi-review/midi-review.lua
dreamsequence/lib/functions.lua
dreamsequence/dreamsequence.lua
cheat_codes_2/lib/transport.lua
zxcvbn/lib/vterm.lua
zxcvbn/zxcvbn.lua
ack/hack/lib/norns_engine_tester.scd
loom/loom.lua
foundry/foundry.lua
wobblewobble/wobblewobble.lua
pitfalls/lib/display.lua
pitfalls/lib/functions.lua
rpmate/lib/lua/screen_main.lua
r/hack/screen_test_3.scd
r/hack/screen_test_2.scd
r/hack/test_script_1.scd
r/hack/lib/norns_engine_tester.scd
n.kria/lib/screen_graphics.lua
graintopia/graintopia.lua
fibonacci/fibonacci.lua
loudnumbers_norns/loudnumbers_norns.lua
spirals/spirals.lua
nice-tapes/lib/mod.lua
qfwfq/qfwfq.lua
amenbreak/amenbreak.lua
u/REF.lua
nc03-ds/gimbal.lua
thirtythree/lib/graphics.lua
yggdrasil/lib/Field.lua
yggdrasil/lib/Slot.lua
yggdrasil/lib/tracker.lua
yggdrasil/lib/functions.lua
yggdrasil/lib/graphics.lua
pirate-radio/lib/ui/Marquee.lua
amen/amen.lua
splnkr/lib/sequencer_screen.lua
beacon/beacon.lua
gridstep/gridstep.lua
fall/lib/ui.menu.lua
step/hack/step_plus.scd

and those using the lib util func trim_string_to_width

tehn@obsidian:~/local/allnorns$ rg -l trim_string_to_width
sempra/sempra.lua
phonotype/phonotype.lua
scholastic/scholastic.lua
cheat_codes_2/cheat_codes_2.lua
dreamsequence/dreamsequence.lua
cheat_codes_2/lib/transport.lua
cheat_codes_2/lib/main_menu.lua
cheat_codes_2/lib/midicheat.lua
arp_index/arp_index.lua
timber/lib/timber_engine.lua
repl-looper/lib/repllooper_engine.lua
rpmate/lib/timbereq_engine.lua
faeng/lib/timber_guts.lua
takt/lib/browser.lua
raindrops/raindrops.lua
less_concepts/less-concepts.lua
nc03-ds/lib/sc_helpers.lua
foulplay/foulplay.lua
drum_room/drum_room.lua
gridstep/gridstep.lua
gridstep/lib/timber_gridstep.lua
fall/fall.lua
fall/lib/ui.menu.lua
fall/lib/page.lfo.lua

so, it's not insubstantial, but also many of these scripts come from the same author and if there is fix advice, we could likely get everything migrated quickly.

of course, if someone downloads a "fixed" script but hasn't updated their core norns software, it'll then break.

catfact commented 1 year ago

i had a thought about that. it's a little ugly, but we could keep text_extents as a blocking function with some convolutions:

the lua API call remains the same.
in the lua->weaver function, we just block (on main thread) until we get SCREEN_EVENT_TEXT_EXTENTS in the main event queue
then return to lua with the event data. (actually partial data matching the old API results.)

that will be a little tiny bit tricky because it means adding thread synchronization stuff to weaver (not ideal) or moving this little async component to a new code module. i could give it a go.

of course we can also add one or more of the other possibilities mentioned, and encourage migration to those (particularly for graphics- and timing-heavy scripts; text_extents usage will become a new bottleneck though hopefully not a dire one. (no worse than currently.))

catfact commented 1 year ago

@tlubke yes! using a screen event to trigger lua updates would be perfect.

Could the exchange from ssd1322 thread -> screen event -> lua callback be slow or inconsistent enough to undermine its usefulness?

nah, i don't think so. if script is doing a ton of heavy work, main thread could be slow to pick up the event. but one would hope, not slow compared to the refresh rate.

fortunately we don't have to guess and can measure it.

tehn commented 1 year ago

i agree that it's annoying to code something extra to accommodate/perpetuate a suboptimal pattern, given the effort to refine the entire screen subsystem.

it may, however, be the most painless approach. if we were insistent, we could deprecate this function at a future version, to provide warning.

tlubke commented 1 year ago

yes! using a screen event to trigger lua updates would be perfect.

Cool, I'll see if I can get that done in the next couple days. Should this callback be a mod hook, or something more like enc() and key() functions? Thoughts on names? I'm guessing function refresh(...) might be fairly common in many scripts already.

FWIW I think accommodating the existing text_extents pattern is the right move, but we should probably mark it deprecated (lua REPL print statement?) right away.

catfact commented 1 year ago

hm, i guess i would lean towards putting the new callback in core rather than making it a mod hook. and yeah, following the pattern for key/enc/etc. i don't have an opinion on naming.

i'm looking at making text_extents and peek blocking again on the main thread. a little uglier than i anticipated but ok.

catfact commented 1 year ago

pushed branch block-on-screen-queries to my fork https://github.com/catfact/norns/tree/block-on-screen-queries

added screen_results module which just wraps a semaphore and a single-element "queue" of results (which are still event_data unions.)

each weaver function now pushes screen event, waits on the semaphore, reads results event data, and returns directly to lua. (also frees the event data, since this whole sequence is bypassing the event queue which normally does that.)

screen event handler now pushes results event data and signals the semaphore.

so, not complicated but maybe a little confusing.

i made the changes for text_extents and peek but not yet for current_position.

haven't yet tested this really except to ensure that it compiles on rpi4. will try and get to it later today. i guess i'll PR back to this branch if it looks ok.

catfact commented 1 year ago

so i did test a bit and found many things to fix. fixes are in and PR is up.

i tested basically by running the euclidogons script, which uses text_extents in the midst of smooth/fast animation. i didn't really test peek and current_point except to determine that they don't completely fall down.

what's nice is that with the combined changes there is an absolutely night and day difference in the smoothness of the graphics (and in musical jitter, though i didn't measure it yet.) it is a much improved experience and feels like a higher quality device. so i think the effort is worth it.

catfact commented 1 year ago

here is that PR btw if anyone wants to have a look https://github.com/tlubke/norns/pull/2

it's a little weird that screen result events are still defined alongside other events but aren't actually handled in the main event queue.

in fact now that i think about it i should check that the results event payloads aren't bigger than other event payloads (thus increasing the size of everything in the event queue, i think.) that would be a non-cosmetic reason to define them separately.

in fact heck i'll move them anyway. later.

tlubke commented 1 year ago

Reviewed https://github.com/tlubke/norns/pull/2, will merge at the end of the night if there aren't any concerns from others.

tehn commented 1 year ago

perhaps we should merge to a testing branch? ie screen? i'd like to roll a test beta (with the update process in place for boot.txt which might require a little per-device sorting iirc for shields vs standard)

very enthusiastic and appreciative of all that's going on here

tlubke commented 1 year ago

https://github.com/tlubke/norns/commit/75a15939e85cf8e61a8e71f4e577e67d3336b9e0

Here is some initial testing on a refresh callback. I'm using this little norns script:

function redraw()
  screen.clear()
  screen.move(64, 32)
  screen.text_center("i="..i)
  screen.update()
end

function refresh() 
  i = i+1
  redraw()
end

perhaps we should merge to a testing branch? ie screen? i'd like to roll a test beta (with the update process in place for boot.txt which might require a little per-device sorting iirc for shields vs standard)

@tehn, once https://github.com/tlubke/norns/pull/2 is merged, I think everything will be ready for a test branch! Regarding boot.txt, what's the cause for the difference between shields and standards? Pin layout?

tehn commented 1 year ago

one issue i foresee (i'm attempting to roll a beta update that will work on all norns, shields, etc) is that shields have the screen rotated by 180 degrees, so in config.txt there is dtparam=rotate=180

it'd likely be trivial to do the rotation within matron, after some device detection

tlubke commented 1 year ago

Just a reminder to compile with --enable-display-timer-thread for the beta! At this point I think I'll either remove the option, or reverse the logic to --disable-display-timer-thread, but don't want to push that out until I can test it.

tehn commented 1 year ago

thanks for the reminder. recompiling and retesting everything

catfact commented 1 year ago

heads up small PR https://github.com/tlubke/norns/pull/3

mostly minor cleanup but i do keep messing up the lua file somehow

dndrks commented 1 year ago

hihi! hope all's well :) these changes look fantastic and, echoing @catfact , make the system navigation/usability feel super pro. thank you all for the work on this, it's so exciting + staggering! sorta losing my mind over how good oooooo looks!!! also sorta losing my mind over how good a simple quick-scroll through params looks!!! dang, mind completely lost.

here are some quick lowkey testing results:

rendering differences

test code from @tehn

```lua -- screen redraw function redraw = function() -- clear screen screen.clear() -- set pixel brightness (0-15) screen.level(15) -- enable anti-alasing screen.aa(1) -- set line width screen.line_width(1.0) -- move position screen.move(0,0) -- draw line screen.line(10,20) -- stroke line screen.stroke() -- draw arc: x-center, y-center, radius, angle1, angle2 screen.arc(20,0,10,0,math.pi*0.8) screen.stroke() -- draw rect: x,y,w,h screen.rect(30,10,15,20) screen.level(3) screen.stroke() -- draw curve screen.move(50,0) screen.curve(50,20,60,0,70,10) screen.level(15) screen.stroke() -- draw poly and fill screen.move(60,20) screen.line(80,10) screen.line(70,40) screen.close() screen.level(10) screen.fill() -- draw circle screen.circle(100,20,10) screen.stroke() screen.level(15) screen.move(0,63) -- set text face screen.font_face(9) -- set text size screen.font_size(20) -- draw text screen.text("new!") -- draw centered text screen.move(63,50) screen.font_face(0) screen.font_size(8) screen.text_center("center") -- draw right aliged text screen.move(127,63) screen.text_right("1992") screen.update() end ```

left: new / right: current

(note the level difference between the rectangles, and aliasing on curve, circle, and new! text)

`text_extents` errors

assuming text_extents usage shouldn't error out, i'm seeing some wonk with flora.

steps to repro:

boot flora
CW E1 to water amp 2.0 screen
CW E2 until the water # note freqs 1 displays, then next CW turn will start to error out as text_extents is called at https://github.com/jaseknighter/flora/blob/9500494c815c2593e746c6a359fee79039f4eb42/lib/flora_pages.lua#L265:

screen_results_wait
screen_results_post
text_extents, got screen results
lua: /home/we/dust/code/flora/lib/flora_pages.lua:265: attempt to perform arithmetic on a nil value
stack traceback:
    /home/we/dust/code/flora/lib/flora_pages.lua:265: in upvalue 'draw_top_nav'
    /home/we/dust/code/flora/lib/flora_pages.lua:336: in field 'draw_pages'
    /home/we/dust/code/flora/flora.lua:262: in field 'event'
    /home/we/norns/lua/core/metro.lua:164: in function </home/we/norns/lua/core/metro.lua:160>

flickering

when explicitly running big scripts at 60fps (eg. changing this line to SCREEN_FRAMERATE = 1/60 in flora), the screen noticeably flickers as it gets cleared and redrawn.

it might just be worth noting that heavier scripts won't just flip the switch to 60fps without some lite refactoring, but maybe this is useful touchpoint for y'all?

catfact commented 1 year ago

@dndrks thanks much for the feedback 👁️

the error is probably because i keep messing up screen.lua in stupid ways

fix is in the PR above. you can pull branch cleanup-screen-results from my fork, or just edit screen.lua so that text_extents actually returns a value: https://github.com/tlubke/norns/pull/3/files#diff-0b1308291708b3e72b732e90ec9c2eb1c384d5869da45d7b983a64c0e5f94922R209-R211

catfact commented 1 year ago

re: flickering i think it would be a good idea to try using the new refresh callback to drive the redraw function. just to see. (i also haven't really looked at how that is set up, but will review it more closely some time today.)

regardless of what tricks we pull, 60fps is probably pretty ambitious given our "rendering pipeline" unless the actual rendering is pretty minimal.

that said, i have noticed what i think are some new artifacts like: when launching a script, the selection menu shows for maybe 1 frame between the script info page and the script ui. some kinda race condition with swapping the redraw functions, idk. (this particular thing didn't bother me much when weighed against animation smoothness.)

catfact commented 1 year ago

Also yeah let's move this to a staging/test branch under monomes repo, ASAP, so that tlubke doesn't have to reveiw every change?

tlubke commented 1 year ago

@dndrks Thanks so much for testing! I'll merge https://github.com/tlubke/norns/pull/3 quick so the text_extents issue is fixed.

Regarding flickering, this comment in ssd1322.c might be relevant -- I was seeing a huge amount of flickering when I put event_post() at the top of the while-loop:

    while( spidev_buffer ){
        if( display_dirty ){
            ssd1322_refresh();
            display_dirty = false;
        }

        // If this event happens right before ssd1322_refresh(),
        // there is quite a bit of flashing. Possibly from being
        // at a weird sync point with the hardware refresh.
        event_post(event_data_new(EVENT_SCREEN_REFRESH));

    clock_nanosleep(CLOCK_MONOTONIC, 0, &ts, NULL);
    }

echoing @catfact, try using the function refresh() ... end callback and see if that helps. If there's still flickering, try recompiling norns without the thread in ssd1322.c enabled. The callback won't work after you disable the thread, but I have a feeling it'll probably look cleaner without the timer.

tlubke commented 1 year ago

Also yeah let's move this to a staging/test branch under monomes repo, ASAP, so that tlubke doesn't have to reveiw every change?

Let me know when a branch is made, I'll edit the PR target branch.

tehn commented 1 year ago

just made screen branch, let's use that

monome / norns