Teletype refactoring ideas

Continuing the discussion from Teletype v2 proposals:


I’ve had some ideas about refactoring the teletype codebase, it’s more complex than the other modules and has the potential to be even more complex due to it’s open ended nature.

As it stands the firmware is mainly split between 2 files, main.c which handles the hardware, and teletype.c which handles the algorithm, teletype.c is well insulated from the hardware via the update_* function pointers.

The main thing that I think we should do is reduce the number of global variables in teletype.c and reduce the amount of direct access to those variables. (Ideally, we’d remove direct access entirely.)

I’ve started exploring some ideas on a branch called state on my GitHub account.

Essentially I’ve added a new file state.h that defines a type teletype_t that is designed to encapsulate the entire state of the teletype, so far I’ve only dealt with the stack, e.g.

// state.h
typedef struct {
  int16_t values[STACK_SIZE];
  int16_t top;
} teletype_stack_t;

typedef struct {
  teletype_stack_t stack;
} teletype_t;

as well as some functions to manipulate that state…

// state.h

// I've used a static inline header function instead of a macro (LTO is another option...)
static inline int16_t tt_pop(teletype_t *t) {
  t->stack.top--;
  return t->stack.values[t->stack.top];
}

static inline void tt_push(teletype_t *t, int16_t data) {
  t->stack.values[t->stack.top] = data;
  t->stack.top++;
}

// etc, etc

In teletype.c I’ve created a global variable state and some shim functions to allow existing code to continue working:

// teletype.c
teletype_t state;

int16_t pop() {
  return tt_pop(&state);
}

void push(int16_t data) {
  tt_push(&state, data);
}

I’ve also modified the prototype for ops, from

void (*func)(void)

to

void (*func)(teletype_t *t)

e.g. op_ADD has changed from:

static void op_ADD() {
  push(pop() + pop());
}

to

void op_ADD(teletype_t *t) {
  tt_push(t, tt_pop(t) + tt_pop(t));
}

and I’ve moved it into a new file ops/maths.c (along with some other ops).


Thoughts? It will end up increasing the verbosity of code and there may be a slight performance hit. But I think it will make it a lot easier to rationalise about the behaviour of code. Also if we limit ourselves to accessing teletype_t via public functions it makes it much easier to adjust the internal structure.

As an added bonus, if we were to completely remove all the global variables from teletype.c, and instead just pass around a teletype_t to all the functions you then have the option of unit testing the bits of code that might really challenge your sanity (e.g. converting to and from text for USB saving).

4 Likes

i’m very much in favor of this. still processing how much effort will be needed to transition to this method. another big bonus is the ability to maintain the “simulator” with much more ease-- in addition to repurposing the teletype.c module in other ways (ie max external??)

i’ll take a look at the code and look for complications-- and i’d be curious if you similarly see any roadblocks.

seems like it’d be negligible at first read-- though i’d be curious about your analysis.


one major refactor will be (likely) needing to introduce dynamic memory for the implementation of TL (timeline). unless we choose an arbitrary number of max lines per timeline, and then every scene must store the max number. this would substantially shrink the preset/scene pool as we’d hit the flash ceiling.

1 Like

It’s always really hard to know how much effort to put into things like this. Back when I was working it was always a hard thing to justify to the people that paid the bills (and not without reason either).

In the end, what worked well for me was to have an idea of where I wanted to go, and how I wanted the code to look (almost how I wanted it to feel). Then take small steps towards it every now and again, especially when the change was sympathetic to other more urgent changes.

We could, for example, just do enough to get the ops, variables and arrays not using global variables directly. I’d been looking at providing equivalent PN.* versions of P.* ops, so I’d like to get the pattern ops out into their own file.

Does the idea of starting the V2 work on the current codebase excite you? Or do you feel like you’ll be lost in spaghetti? If you’re happy and feeling positive with the code as it is, the I’d say just get started with the V2 stuff, but keep some of these ideas at the back of your head.


I’ve been thinking a little about the best way to model the state of the teletype too. I think we have 4 ‘scopes’:

  • global
    • all the presets
    • maybe other settings someday… (e.g. calibration data for the DACs)
  • preset
    • a collection of scripts
    • pattern data
    • variables & arrays
    • etc
  • script
    • a collection of commands
    • if/then/else mod status
  • command
    • tele_data_t data[]
    • stack

More practically, there are a few other things that would be good to do…

Coalesce the various update_* functions into a single update_t struct.

Get rid of left, this is mainly used by the vars to differentiate between get and set, instead we could modify:

typedef struct {
	const char *name;
	void (*func)(uint8_t);
	int16_t v;
} tele_var_t;

to be

typedef struct {
	const char *name;
        void (*get)();
	void (*set)();
	int16_t v;
} tele_var_t;

(probably add a teletype_t *t in there too)

If we can fix the hacks for the P and PN ops, left is just a local variable of process only. Part of me feels as though there is some overlap between ops, arrays and variables, and that the solution to P and PN validation lies there. That we some category of commands that behave like variables (with a get and set operation) and some that behave like functions (with 0-n inputs and 0-1 outputs).


First question, are timeline commands going to support MODs?

1 Like

i added the issues (features) to the TT repo and no doubt you’ve seen the list: https://github.com/monome/teletype/issues

many of these would be relatively easy to add with the existing codebase. some less so. i really like the sound of “cleaning up” the teletype codebase-- i appreciate your patience with my sometimes hack-must-ship moments. this is also the most complicated software i’ve written myself (undecipherable mlr max patches withstanding).

yes! simple enough for sure.

agreed. it’s a messy hack.

i’m up for suggestions on how to tackle the P and PN special cases. that’d be amazing.

i don’t see why not, given at this time the timeline would use the same tele_command likely

1 Like

Watching this with great interest.

This, in particular, excites me. Going further, I wonder if, in general, there isn’t a natural way to (re)factor the module sources so that they could easily be wrapped in max externals. Having max patch counterparts to firmwares would be a boon for exploration and smoke-testing. (All the better to get them more or less for free!)

1 Like

agreed, this would be great. i was throwing out the idea-- but i must note that my todo list is very long.

2 Likes

I think I’m going to park the big refactoring for a few weeks… instead I’m going to try and do some lower level things first. In particular I’m going to see if I can fix the P and PN hack. I’ve got a few ideas…

I’ll have a bit of a think about timeline memory management too. But surely each preset needs to be the same size? Otherwise saving a loading to flash gets really tricky, doesn’t it? If you want to get dynamic, you could have a pool of commands shared between the scripts and the timeline, but that only has a benefit if you want to start allowing more than 6 lines in a script. (And I’m not sure how I feel about that.)

:thumbsup:

In the short-term just being mindful and maintaining (the already good) isolation of teletype.c from avr bits will keep the way clear down the road. Cheers!

Okay, so I think this is my plan for refactoring ops, vars, arrays and keys…

  1. Extend ops so that they have a ‘get’ and an optional ‘set’ operation, the arity of of the set is 1 more than the get. A set returns nothing. Update validate and process to deal with this. This should fix the P and PN hacks.

  2. Convert keys to ops, use a macro to keep the code duplication down.

  3. Convert variables and arrays to ops. The values (v) will need to be stored elsewhere, probably just global state for now.

  4. Simplify the parsing, validation and processing code as we only have numbers, ops and mods.

Can anyone think of any gotchas with this approach?

Longer term, once the parsing code is as simple as possible, we can think about replacing it with a finite state machine, which will bring the parsing of a single symbol down to O(length of longest symbol), i.e. the same as a single strcmp, instead of the multiple strcmp we do now.

1 Like

I think I’ve got a handle on the parse and validate functions. Would you like me to add some unit tests to test the existing code before I make my changes? That way we can try to make sure that no bugs are introduced with my changes.

Personally I think unit tests work really well for fiddly bits of code like this, but I’d only want to do it if there is some commitment to maintain the tests, and not just delete them when/if they stop working.

1 Like

Yes please!

Unit tests would be great and I completely agree that they’re ideal for fiddly bits like this. I say go for it. Happy to pitch in too.

2 Likes

this seems very sensible!

also very reasonable.

let me know when i can jump in and help cut-paste code around-- i don’t want to overlap or mess up your progress.

1 Like

And me too. I’d be happy to help throw together some tests as well.

Don’t suppose you happen to know much about C unit testing frameworks? I’ve done a bit of googling, the criteria are:

  • simplicity, I really don’t want this to be a burden for others to maintain (or even to use), so it needs to fit well with the existing build proces e.g. no autotools!
  • isolation, we’ve got a lot of global state (maybe we won’t in the future, but we do now), so I guess some sort of fork based runner?

@ppqq, do you own a teletype module?

Hilarious. I was punting and hoping you had this in the bag!

Honestly, I’m just now coming (back) to C and am a bit shocked at how wild west things are.

To your criteria, I’d add Continuous Integration support. Maybe not an issue but I’d love to see things running on travis (I know this has been discussed elsewhere).

Any thoughts about unity?

I do. LOVE it. Unfortunately, I have very limited time in the studio these days so I rarely even glimpse it. That said, I realized that the teletype story could be translated very well to working on the computer (hence my interest in the simulator and an external wrapper around teletype.c.)

Drat. This is the first C coding I’ve ever really done! (Although I have coded in almost every other programming language under the sun, including a bit of C++ a few years back)

https://travis-ci.org/samdoshi/teletype
https://travis-ci.org/samdoshi/libavr32


I’ll have a look a unity. Seems like if we go for a fork based runner, we’d have to give up any hope of running it on the device (as there is no fork on the embedded platform). It shouldn’t be too hard to remove the global state from the parse and validate functions. process will require a lot of refactoring though.

1 Like

oy. shall we delegate some tasks?

ie. i can throw in some hours finishing a cut-copy job of making (nearly everything) into OPs on another branch?

If you like. Only problem is that vars and arrays can’t get turned into ops, until ops gain the ability to run in the set postion (a.k.a left).

We need to change

typedef struct {
    const char *name;
    void (*func)(void);
    char params;
    int8_t returns;
    const char *doc;
} tele_op_t;

to

typedef struct {
    const char *name;
    void (*get)(void);
    void (*set)(int16_t);
    char params;
    int8_t returns;
    const char *doc;
} tele_op_t;

and then update validate to allow ops to work with either get (a.k.a left) or set (right).

We need a way to indicate that an op doesn’t support the set operation. Either NULL or a special notsupported function pointer that we can check for equality. validate and process needs to take this into account.

If you’ve got time to burn at the moment and you want to get on with things I can try and go into even greater detail about how I’d do it? Otherwise I’ll be able to get stuff sorted in the next week or so.

Right, C unit testing, I’ve given up on fork based. We’re just going to have to be careful with our testing and global state.

So far I’ve got:

Header only

Minimal C

I quite like the look of the silentbicycle pair. Particularly theft as we might be able to use it to stress test the parser and validator. Greatest also mentions that it doesn’t allocate which could be useful for running on hardware (if we ever do that).

I will try and get a PR in tomorrow with a working test runner and some helper functions / macros for testing the parse and validate functions, then we can start creating some test cases.

2 Likes

:thumbsup:

It’s funny. On my way to work I was thinking, screw it, we could just write a simple framework in maybe a file. Happy to see someone’s already done it!

Needless to say, stoked to see this happen. Let me know if I can help.

1 Like