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 #424

Closed
wants to merge 2 commits into from
Closed

arc support #424

wants to merge 2 commits into from

Conversation

@scanner-darkly
Copy link
Member

@scanner-darkly scanner-darkly commented Jun 13, 2018

arc support. i mostly copied the grid implementation, trying to re-use monome naming/functions were possible. i did have to introduce arc specific methods for LED rendering as it's done differently for arcs.

there is a couple of issues with this implementation:

  • it will return 0 for number of encoders - from what i can tell libmonome doesn't currently support it, i'll add it once i have a chance to add it to libmonome
  • it breaks (error messages in REPL suggest some memory corruption, and then at some point it hangs) if both grid and arc are connected and LEDs are set on both (or if refresh is called). they seem to work separately fine, or if you only update one or the other, so i think it should still be okay to merge this, and i'll fix it in a separate PR.

didn't want to create a global variable a so used arc instead.

@artfwo
Copy link
Member

@artfwo artfwo commented Jun 13, 2018

the global arc variable could be entirely dropped in this case (implying that arc add/remove is handled by the script).

@@ -137,6 +174,11 @@ int dev_monome_grid_cols(struct dev_monome *md) {
return monome_get_cols(md->m);
}

int dev_monome_arc_encs(struct dev_monome *md) {
// FIXME returning rows, libmonome currently doesn't support returning encoders
return monome_get_rows(md->m);

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

if this is broken, consider returning a constant or removing the method completely.

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

as a hack, scripts can get the number of encoders from the device name (which can be done in lua).

This comment has been minimized.

@scanner-darkly

scanner-darkly Jun 13, 2018
Author Member

that would be easier - is it considered a reliable enough way? not sure if it's different for DIY'd arcs. (the reason i didn't use a constant was because it would give me a compiler error about unused md variable, since warnings are treated as errors)

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

right, just use (void) md; in the beginning of the function to avoid that warning. that's what we currently do to override that.

This comment has been minimized.

@scanner-darkly

scanner-darkly Jun 13, 2018
Author Member

cool, good to know!
going to remove this and move the implementation to lua as you suggested.

// set all arc LEDs to value
void dev_arc_all_led(struct dev_monome *md, uint8_t val) {
dev_monome_all_led(md, val);
md->arc = true;

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

this flag shouldn't be set here or in the above method. i'd suggest removing it completely and using rows/cols for checking that we're dealing with an arc in dev_monome_refresh (arcs have 0x0 size in libmonome)

This comment has been minimized.

@scanner-darkly

scanner-darkly Jun 13, 2018
Author Member

i'm considering implementing separate arc methods instead of re-using dev_monome_refresh/dev_monome_init etc. it wouldn't create much code duplication and it would be cleaner (and i have a feeling this would help with the issue of grid and arc not working together).

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

this will also make things a bit messier. i believe the issue with grid/arc not working together could be solved with the current implementation. could you run gdb to see what leads to the memory errors?

This comment has been minimized.

@scanner-darkly

scanner-darkly Jun 13, 2018
Author Member

wouldn't it be cleaner with separate methods?

void dev_arc_all_led(...)
void dev_grid_all_led(...)
vs

void dev_monome_all_led(...)
...
if (is_arc)
    // arc implementation
else
  // grid implementation

they are pretty light weight wrappers, so there will be hardly any code duplication (and if there is it could be extracted into generic monome functions which both arc and grid functions would call).

regardless yeah, i'll investigate the problem some more, i have a feeling we might run into this with multiple grids as well. thanks for the suggestion, will give it a try!


norns.monome.remove = function(id)
-- call both since we don't know if it's a grid or an arc
-- grid/arc functions will check for existence

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

makes sense to check for monome device type here and call the appropriate remove handler (and remove the existence check from the handler then).

This comment has been minimized.

@scanner-darkly

scanner-darkly Jun 13, 2018
Author Member

same as above, i think i'll add norns.arc.add / norns.arc.remove instead - any objections?

This comment has been minimized.

@artfwo

artfwo Jun 13, 2018
Member

i'd say that sticking to universal monome handler makes sense in C, since it's all just libmonome wrappers. in lua grid and arc should be different objects for sure.

@scanner-darkly
Copy link
Member Author

@scanner-darkly scanner-darkly commented Jun 13, 2018

"the global arc variable could be entirely dropped in this case (implying that arc add/remove is handled by the script)." - could you elaborate on this a bit?

@artfwo
Copy link
Member

@artfwo artfwo commented Jun 13, 2018

take a look at the midi module, which hasn't got a global "m" var, like "g" for grid. the reason for midi not having a "default" variable is that there are use-cases for several devices and device-specific mappings in scripts. midi scripts (such as earthsea) then define handlers for midi.add and midi.remove to setup the controller.

since arc is much more rare and specific than the grid (and often used as secondary controller), it could also have the requirement to explicitly define add/remove handlers in a script, eliminating the need for global default "arc" variable.

@scanner-darkly
Copy link
Member Author

@scanner-darkly scanner-darkly commented Jun 13, 2018

makes sense.
one thing to consider for having a global variable is script.lua turns off all LEDs when doing a script cleanup, which is a good thing. is there a way to achieve the same without a global var?

@artfwo
Copy link
Member

@artfwo artfwo commented Jun 13, 2018

yeah, send a led_all(0) on arc.add that will be called by reconnect in turn. same could be done for the grid.

@scanner-darkly
Copy link
Member Author

@scanner-darkly scanner-darkly commented Jun 13, 2018

sounds good, will remove the global var.

@scanner-darkly
Copy link
Member Author

@scanner-darkly scanner-darkly commented Jun 14, 2018

ran into several issues with this update. somehow arc:refresh() would cause issues even though it worked before (i had a script responding to encoders and displaying current values with LEDs). while investigating i managed to brick norns with an unclosed print statement, had to reflash the latest update and after that arc:refresh works, but somehow arcenc function is not getting called (which worked before). pretty baffled at this point and feel like i'm hitting the limits of my [very limited] knowledge of lua.

i'm going to treat this as a universe way of saying i should finish teletype first - going to close this PR and come back to this when i have the time to properly investigate and refactor.

@scanner-darkly
Copy link
Member Author

@scanner-darkly scanner-darkly commented Jun 14, 2018

also should mention - it's almost as if failures somehow accumulated between stop.sh/start.sh cycles - is there a "cache" of some sort that doesn't get flushed by stop/start cycle?

@catfact
Copy link
Collaborator

@catfact catfact commented Jun 14, 2018

@artfwo artfwo mentioned this pull request Nov 15, 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