Move ops into their own files, introduce new state structures #43

samdoshi · 2016-06-07T13:11:35Z

See http://llllllll.co/t/teletype-refactoring-ideas/2836/90?u=sam

also introduce copy_command and copy_sub_command

split the algorithm code from the module code update libavr32 use a single .clang-format and .editorconfig move the format target to the root Makefile

scanner-darkly · 2016-06-10T03:07:25Z

module/main.c

@@ -131,13 +127,13 @@ int16_t output, output_new;
 #define FIRSTRUN_KEY 0x22

 typedef const struct {
-    tele_script_t script[10];
-    tele_pattern_t patterns[4];
+    scene_script_t script[10];


curious about the name change - scene_script_t is still a single script, right? 'scene' here seems to be more appropriate for the variable name (an array of scripts for a scene) than the type itself.

also thinking might be good to rename script to scripts to keep it consistent with patterns. this is a minor thing though, as is the above.

See state.h, that's where the actual scene types are defined. This is just for saving data to and from nvram. I haven't paid much attention to it, but it probably should be renamed to scripts.

Putting scene at the front is an attempt at namespacing. I've renamed tele_pattern_t to scene_pattern_t too.

yeah as namespacing i think it's a good thing. the only concern would be if for whatever reason scripts and patters as entities could live outside of scenes, but perhaps that would be a different type altogether.

scanner-darkly · 2016-06-10T03:18:42Z

module/main.c


-        for (i = 2; i < 8; i++) {
+        for (size_t i = 2; i < 8; i++) {


was going to ask why size_t is a better choice here (and in other loops) but then realized that it makes perfect sense! 👍

scanner-darkly · 2016-06-10T03:27:54Z

module/main.c

+                            if (v < 32766) {
+                                tele_set_pattern_val(edit_pattern,
+                                                     edit_index + offset_index,
+                                                     v++);


something like tele_inc_pattern_val could be a good function to have as it'll make it more readable and will keep the magic number in the place where all pattern related things are. i can add it once this is merged.

Good idea. One of the things I've been (slowly) trying to do is keep like minded code together. It definitely helps spot patterns and code reductions.

scanner-darkly · 2016-06-10T03:31:32Z

module/main.c

+                            if (v > -32767) {
+                                tele_set_pattern_val(edit_pattern,
+                                                     edit_index + offset_index,
+                                                     v--);


similar to above, would be good to have tele_dec_pattern_val

scanner-darkly · 2016-06-10T03:44:27Z

module/main.c

+                                                tele_set_script_l(
+                                                    edit,
+                                                    tele_get_script_l(edit) +
+                                                        1);


same as above, could possibly be tele_inc_script_l

... have to stop here, will continue tomorrow.

Yes, I almost did this myself.

But... it might be worth doing things like tele_script_insert first. We might find that once they're in place there is no need for a tele_inc_script_l. On the other hand doing tele_inc_script_l first might make it easier to understand whats going on...

i could make this and similar changes once this is merged, assuming @tehn is okay with the change - i think it would be nice to encapsulate all script related functionality in one file eventually (same for patterns)

tehn · 2016-06-11T13:40:26Z

thank you for your patience-- that was a lot to review!

i am incredibly grateful for this confounding level of improvement to the codebase. i learned a few tricks, and got the chance to repeatedly meditate on my development process-- how i was adding features and hacking them in without standing back to assess the overall system.

this is a fantastic refactoring. thank you again. i look forward to extending new functionality with this as a model.

scanner-darkly · 2016-06-11T19:34:49Z

module/main.c

-                                    tele_patterns[edit_pattern].l < 64) {
-                                    tele_patterns[edit_pattern].l++;
+                                uint16_t l = tele_get_pattern_l(edit_pattern);
+                                if (edit_index + offset_index == l && l < 64) {


should this be l < 63?

(actually, this PR is probably not a good place, will move this to main repo instead and only post PR specific questions here)

scanner-darkly · 2016-06-15T03:35:47Z

i have even more appreciation for your monumental refactoring now - tried to diligently review all the changes but it's just not possible [i did review all of the changes in main.c at least, everything looks good], best to just dive in and work on some of the issues, which should be easier now because of this. thanks again for tackling this!

samdoshi added 30 commits Apr 29, 2016

simplify mods by copying the sub-command in process

0a9a923

also introduce copy_command and copy_sub_command

rename pop & push to cs_pop & cs_push, add command_state_t arg to both

842d2ca

move the stack to command_state_t, remove some global variables

07e73c3

move update_* to teletype_io.h

eaee4ba

switch update_* function pointers to concrete tele_* functions

c81a120

replace tele_input_state with tele_get_input_state, remove global var

a1efe92

add '// clang-format off' to auto-generated data.c and data.h

dfa0588

use find to run clang-format in Makefile

d672ee9

run 'make format' (only help.h changed)

1c4f0ed

start moving ops out of teletype.c

d902a87

move ops out of teletype.c

2f0210a

remove condition, replace with if_else_condition

4e6dbe6

update MAKEMOD (and rename to MAKE_MOD)

6974b4e

move delay variables to scene_state_t, introduce command.[hc]

e95c7e1

move stack op data structures to scene_state_t

2d61cff

move mods out of teletype.c

75a17b3

replace teletype.h with command.h in ops/op.h

ed74fb5

rename to_v to to_voltage and move to helpers.c

5d4ee63

merge process_delays with tele_tick

a22a06e

tidy up teletype.c

4ca3faa

modify print_command to copy string to buffer, rather than a global var

28f98aa

move op and mod arrays to op.c

d566533

add const qualifier to src for copy_command and copy_sub_command

fc9030a

move print_command to command.c

3b1eb0d

change parse and validate to not modify the input command

37e11af

return error message directly from parse and validate

05eb321

use strcat in print_command

7b4cbf0

increment 'time' before running delayed commands

d7e8d2f

remove unused mutes from teletype.c

a5ecfc3

remove duplicate definition of clear_delays

ba6d42c

samdoshi added 15 commits May 14, 2016

move tr_pulse to scene_state_t as tr_pulse_timer

21a2ba2

pattern getters and setters

8487fc9

switch tele_patterns.start to use accessor

429aae1

switch tele_patterns.end to use accessor

0082700

switch tele_patterns.wrap to use accessor

d6ee4f9

switch tele_patterns.l to use accessor

ddd1b1f

switch tele_patterns.v to use accessor

1c5564d

switch tele_patterns.i to use accessor

7a91b50

remove tele_patterns as public variable, add helpers to get ptr and size

c611d78

rename tele_scene_t to nvram_scene_t

708a960

move tele_patterns to scene_state_t and rename

bace2c5

move script from main.c to teletype.c, add getters and setters

ea22a1f

move script to scene_state_t

e85bddd

add run_script and run_command, parse now requires an exec_state_t

dbc784b

rearrange code, update libavr32

Loading status checks…

2b3b054

split the algorithm code from the module code update libavr32 use a single .clang-format and .editorconfig move the format target to the root Makefile

scanner-darkly reviewed Jun 10, 2016
View changes

module/main.c

for (i = 2; i < 8; i++) {

for (size_t i = 2; i < 8; i++) {

This comment has been minimized.

Sign in to view

scanner-darkly Jun 10, 2016
Member

was going to ask why size_t is a better choice here (and in other loops) but then realized that it makes perfect sense! 👍

scanner-darkly reviewed Jun 10, 2016
View changes

module/main.c

if (v > -32767) {

tele_set_pattern_val(edit_pattern,

edit_index + offset_index,

v--);

This comment has been minimized.

Sign in to view

scanner-darkly Jun 10, 2016
Member

similar to above, would be good to have tele_dec_pattern_val

scanner-darkly reviewed Jun 10, 2016
View changes

tehn merged commit d7d21ef into monome:master Jun 11, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

scanner-darkly reviewed Jun 11, 2016
View changes

samdoshi deleted the samdoshi:dev branch Nov 16, 2016

monome / teletype

Move ops into their own files, introduce new state structures #43

Move ops into their own files, introduce new state structures #43

samdoshi commented Jun 7, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tehn commented Jun 11, 2016

This comment has been minimized.

This comment has been minimized.

scanner-darkly commented Jun 15, 2016

monome / teletype

Move ops into their own files, introduce new state structures #43

Move ops into their own files, introduce new state structures #43

Conversation

samdoshi commented Jun 7, 2016 • edited

This comment has been minimized.

scanner-darkly Jun 10, 2016 Member

This comment has been minimized.

samdoshi Jun 10, 2016 Author Collaborator

This comment has been minimized.

scanner-darkly Jun 10, 2016 Member

This comment has been minimized.

scanner-darkly Jun 10, 2016 Member

This comment has been minimized.

scanner-darkly Jun 10, 2016 • edited Member

This comment has been minimized.

samdoshi Jun 10, 2016 Author Collaborator

This comment has been minimized.

scanner-darkly Jun 10, 2016 Member

This comment has been minimized.

scanner-darkly Jun 10, 2016 • edited Member

This comment has been minimized.

samdoshi Jun 10, 2016 Author Collaborator

This comment has been minimized.

scanner-darkly Jun 10, 2016 Member

tehn commented Jun 11, 2016

This comment has been minimized.

scanner-darkly Jun 11, 2016 Member

This comment has been minimized.

scanner-darkly Jun 11, 2016 Member

scanner-darkly commented Jun 15, 2016

samdoshi commented Jun 7, 2016 •

edited

scanner-darkly Jun 10, 2016
Member

samdoshi Jun 10, 2016
Author Collaborator

scanner-darkly Jun 10, 2016
Member

scanner-darkly Jun 10, 2016
Member

scanner-darkly Jun 10, 2016 •

edited

Member

samdoshi Jun 10, 2016
Author Collaborator

scanner-darkly Jun 10, 2016
Member

scanner-darkly Jun 10, 2016 •

edited

Member

samdoshi Jun 10, 2016
Author Collaborator

scanner-darkly Jun 10, 2016
Member

scanner-darkly Jun 11, 2016
Member

scanner-darkly Jun 11, 2016
Member