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

Arc Support #638

Closed
wants to merge 8 commits into from
Closed

Arc Support #638

wants to merge 8 commits into from

Conversation

@okyeron
Copy link
Contributor

@okyeron okyeron commented Nov 8, 2018

Revisiting @scanner-darkly PR for arc support.

This adapts the PR scanner-darkly did back in June to the most recent norns/grid codebase.
https://github.com/monome/norns/pull/424/files

Some comments by @artfwo on the original PR still apply and need to be addressed.

okyeron added 3 commits Nov 8, 2018
copied @scanner_darkly PR for arc support into current norns codebase
okyeron okyeron
okyeron okyeron
@tehn
Copy link
Member

@tehn tehn commented Nov 9, 2018

rad, thanks! i'll take a look.

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 9, 2018

FWIW - There are still some issues it seems.

  • I noticed that from lua the arc.event(n,z) function is returning 1 no matter which encoder is sending. I'm pretty stumped so far about where this would be in the code. EDIT: FIXED!
  • leds may not be being set properly
okyeron added 2 commits Nov 9, 2018
okyeron okyeron
arc.all() is now 2 parameters - first for encoder number, second for value
@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 9, 2018

With last commit, the function for setting all leds is now 2 parameters - arc.all(1,0) so all leds for individual encoders can be set. Will probably need another lua function to set all leds on all encoders.

Not sure how led/map commands are working yet. Seems like not treating each encoder as a quad (like on grid) would be more efficient when sending led/map to an individual encoder group of leds.

@artfwo
Copy link
Member

@artfwo artfwo commented Nov 14, 2018

@okyeron is this code working for you? I'm getting a PANIC: unprotected error in call to Lua API (attempt to index a nil value) error and matron crash when trying to run this locally.

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 14, 2018

It was working for me, but I made more changes that I've not committed yet. I was going to try my new changes as a branch. I'll try to sort that out this afternoon.

Is that error you're seeing just on starting up norns/matron?

@artfwo
Copy link
Member

@artfwo artfwo commented Nov 14, 2018

yeah, but then the process crashes. something wrong with my local build, doesn't seem to happen with a clean checkout from you branch. is that okay to commit changes to your branch directly?

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 14, 2018

Yes - i think so?

@artfwo
Copy link
Member

@artfwo artfwo commented Nov 14, 2018

Okay, then go ahead with testing your changes, I can work with your currently pushed code in the meantime.

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 14, 2018

I created an arc-dev branch that has additional changes:

  • added enc parameter to refresh so each ring can be updated individually instead of all four at once. This seems to fit better with the /ring/map command
  • added support for enc buttons. These exist on only one generation of Arc, but why not support it?
  • found a problem last night that device was not being "released" when changing scripts. Still investigating this. Looks like state.lua wasnt calling update_devices for arc. Fixed.

Here are a couple lua files I started on for testing
https://gist.github.com/okyeron/29d0a8be36c15baaaec3990a00aed05e
https://gist.github.com/okyeron/406ad051b432234d97ded8fb9a1fbde9

@artfwo
Copy link
Member

@artfwo artfwo commented Nov 14, 2018

good job! a couple of comments regarding the later branch:

push encoder support, while good for the owners of the pushable arcs, doesn't provide the api that will be compatible with the rest of devices. scripts for push encoders won't run with the rest of arcs.

refresh in grid api is supposed to refresh the entire device, so let's leave it the same way in arc for consistency. we don't have a method similar to /grid/map or /ring/map, because you're supposed to use the api (set_led) to draw "offscreen", then call refresh() to update the leds on device.

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 14, 2018

FWIW - my want for push encoder support is because I'm making an encoder box which impersonates an Arc.

I'm not sure I understand what you mean by "doesn't provide the api that will be compatible with the rest of devices". Which rest of devices? Norns would just end up sending a serial /prefix/enc/key n d command I assume this would be ignored by any device that doesn't support that command. So why is it a problem to include that?

Seems like this would be more of an easter egg for those button arc folks (and useful to me for future diy things).

On refresh, the particular use case I'm thinking of is refreshing different arc rings on various different timescales - which could get bogged down (or blinky?) by refreshing all 4 every time at high framerates.

@tehn
Copy link
Member

@tehn tehn commented Nov 15, 2018

it's a good idea to support encoder key input. i like the idea of DIY emulated devices.

it shouldn't break the pattern, it just needs a separate callback. (usually this callback won't be used.) so there needs to be another arc event.

for refresh, we should have dirty flags for each ring the same way grids have a dirty flag per quadrant. see here: https://github.com/monome/norns/blob/master/matron/src/device/device_monome.c

the user should absolutely only have one refresh function without args, follow the grid pattern.

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 15, 2018

it just needs a separate callback.

I think I did this (there's both norns.arc.enc and norns.arc.key input handlers), but I don't think i did it correctly (and not sure what correct would be here). Initially I ended up with both enc and key events triggering at the same time, but I added a variable to tell them apart. Maybe that needs to be a table like the midi events use, but I'm not sure how to do that?

FWIW - I'm kinda flying by the seat of my pants here - mostly copying what was there for grid and coding by trial and error. :)

@tehn
Copy link
Member

@tehn tehn commented Nov 15, 2018

i haven't checked your code yet, so i was commenting a little blind based on the discussion.

should have time tomorrow to look though! thank you for diving in!

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 15, 2018

OK - new commit to master that

  • rolls back the led_all to have just one argument (brightness) and updates all leds on device
  • adds button support

I can't seem to find one issue in my lua code where I have to set arc.all(1) instead of arc.all(0) to get the leds to clear entirely. I double checked this in weaver.c and i don't see anywhere else where this is handled. EDIT - oops - this may be an error in my DIY arc code. Maybe someone can test this for me?

test scripts:
https://gist.github.com/okyeron/3b2a3278a1b02b28f1de32d67f44174d
https://gist.github.com/okyeron/e7e2e94393f69937fa16a4f82dffc3f5

@artfwo artfwo mentioned this pull request Nov 15, 2018
@artfwo
Copy link
Member

@artfwo artfwo commented Nov 15, 2018

@okyeron i cannot push more code to this PR (have you enabled 'edits from maintainers'?), so I'm creating a new one with some cleanup and overhaul to your code, see #641

@okyeron
Copy link
Contributor Author

@okyeron okyeron commented Nov 15, 2018

understood.

Not sure why you could not push to this PR, although 'edits from maintainers' is enabled. Sorry... I'm still a bit of a git newb.

@artfwo
Copy link
Member

@artfwo artfwo commented Nov 15, 2018

that's fine, please send me your github email address (via lines), so I can include you in the later PR co-authors.

@artfwo
Copy link
Member

@artfwo artfwo commented Nov 16, 2018

closing this, arc support is merged via PR #641

@artfwo artfwo closed this Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants