Teletype refactoring ideas

this makes a ton of sense. are you basically thinking individual functions for each mode? so handler_ScreenRefresh etc are just calling functions based on mode?

1 Like

Yeah basically.

The way I’d rig it would be to have each mode M_LIVE, M_EDIT, etc, have it’s own file with a function to handle the keypress and the the screen refresh. Then have main.c call the appropriate functions based on the current mode.

I’m also thinking that the module code (e.g. main.c) needs to be split out into it’s own directory, like we have for the tests and the simulator.

Another progress update… the ops have now been moved into their own files

https://github.com/samdoshi/teletype/tree/dev/src/ops

They’re still quite messy. At some point soon I’ll work my way through each of the files and format them nicely.

3 Likes

wonderful! i’m buried in arc business but i’m excited to start in on tt v2 very soon. your edits are going to make the experience much smoother!

1 Like

@tehn is the mutes variable in teletype.c redundant? (there is another one in main.c)

I’ve deleted it.


Any idea when you’re going to want to start? I’ll try and make sure things are merged in first.

Pretty decent Saturday of code (thanks to this), I’ve got the pattern data merged into the new scene_state_t struct.

I will say that before starting on anything to do with V2, main.c needs sorting out! Some of the bits of code in there are so deeply nested that you can’t do anything before hitting the 80 char limit. :open_mouth:


Current todo list

  • update the mods to use the state types
  • move the stack to command_state_t
  • break the ops into separate files
  • sort out pattern access (no more direct array access?)
  • use ragel to to make the parsing much much faster
  • move the scripts into teletype.c and implement a higher level script runner
  • get rid of the update_* function pointers, I’ll probably replace them with a compile time solution
  • move itoa from libavr32 to the teletype repo
  • fix those annoying rmdir errors on OSX
  • move all the documentation for ops into comments in the src code, and generate a manual from that

Next up, it’s either Ragel, or move the script data into scene_state_t.

4 Likes

Looks like this is OPEN SOURCE and IT IS HAPPENING. It’s exciting if tehn’s workload can be lightened and we can still move forward. Maybe it’s scary, too, but if tehn gives sam the go ahead, I guess I second that emotion…

@sam

Fantastic work on the refactoring! Really thoughtfully done. Sincerely appreciated.

Quick question: I’m wondering if you’ve checked in the completed task of “break the ops into separate files”? Or, maybe I don’t quite understand the task as I still see all the ops sitting in teletype.c.

Thanks!!

b

Just a quick reply incase your still awake… it’s on a different branch.

https://github.com/samdoshi/teletype/tree/dev/src/ops

I’ll try and post some more stuff later today so that you can keep your work easily merge-able. (I saw your questions in the V2 thread.)

1 Like

Looking at my diary, I’m going to be a bit too busy to get much more coding done till late June now.

Almost everything I’ve done has been pushed up to the dev branch on my fork.

I’m currently in the process of wrapping access to the script array, but I haven’t finished it yet, and might not get a chance for the next few weeks. Once I’ve done that, I’m at the point of being able to completely separate the algorithm part from the hardware part. Ideally that’s when I’d have put the next PR in. But I’m wondering if it might be better to put a PR in now?

Here is the compare view from GitHub


@bpcmusic regarding i2c, I know that @tehn has been doing some work on enhancing the i2c communication, look at the commits for libavr32. I think it was partly due to another manufacturer’s module adding support for the II bus (and I think I know which one too).

Anyway, here is the ultra quick guide to adding your own ops, assuming that you’re working of my dev branch.

  1. (in case you’re not a git expert) Fork the repo and don’t work on the master branch!
  2. Create a new .c and .h file in src/ops/, maybe expander.c?
  3. Add the .c file to the Makefiles in tests and simulator, and to config.mk in src/
  4. Add the include to src/ops/op.c
  5. Create the ops (perhaps copy the CV, etc ones and adapt)
  6. Add the ops to the table in src/ops/op.c, don’t forget to update the number of ops

If you get stuck with defining an op properly give me a shout (or anything else you need help with). Unfortunately it’s still in a state of flux, so the comments or structuring aren’t the best.

3 Likes

Thanks so much for this, @sam. Was just about to dig in deeper with your branch. (I did some experimentation with the current TT branch and was able to add my i2c stuff as an experiment - but realized that I quickly needed to create my own op to make it manageable.)

No worries or hurry for me; I’m going to be traveling for the next several weeks and will be toying around with the code but not at the bench to load and test anything. :slight_smile:

Was planning to fork today, push a build to my dev TT and see if all is good before I roll up my sleeves and get implementing.

Cheers!

b

2 Likes

this is wonderful progress, and despite being not fully to your goal, i think a merge would be good-- i’m going to start back in on TT code in earnest very soon and if i understand the structure you’re going for with abstraction, i can likely help finish up the task as well!

5 Likes

Cool, I’ll try to get a PR in by Thursday or Friday. I’d like to have a go at finishing the code that’s uncommitted on my computer… if don’t get time I’ll put the PR in anyway.

3 Likes

was hoping to get some opinions/thoughts on further enabling variable length i2c messages in the teletype codebase. (these comments refer to the refactored ops branch that @sam has been working on.)

while it is true that @tehn added support for a variable-length package to be sent over i2c, the eventing framework that these messages are passed through in the teletype is done using the event_t struct which limits the data size to s32.

for the output expander’s purposes, we could get away with adding just a wee bit more data room for the expanders as they exist today - that is, the messages for the output expander will most likely look like:

[ u8 - expander address ] [ u8 - output # ] [ u8 - command ] [ s16 - (optional) data payload ]

but, one could imagine that in the future wanting to send more complex information. (for example, i believe someone mentioned sending a block of pattern data at one point.) on the other hand, most of the handlers don’t even use the data parameter - so overhead here would seem like overkill.

there are a number of ways to do this, but no individual one is standing out as ideal. any opinions?

1 Like

there are certainly a lot of ways to do this. @zebra and i worked on a working specification for a broader i2c communication scheme that i’d like to implement into the TT ecosystem, and i’ll be working on this in the coming weeks.

we’ll start publishing information about it once we do some preliminary testing.

at the moment i’d suggest simply making your hardware and basic code work. the “old style” i2c method of static addressing will work fine as a test platform.

1 Like

sure. output expander is easy to test this way (i’d already hacked it in).

eager to see the specification and explore a custom operator to speak with multiple expanders. also, i’m very curious about your preferred implementation for pulling data in so I can build out the input expander functionality.

thanks!

b

Here we go then… let me try to summarises what changes I’ve made, and what’s left to do.

  • The ‘pure’ algorithm code now resides in the src directory, while all code related to the module is in module. Hopefully this will allow main.c to be broken up into smaller files and for it to be obvious what those files concern. There are definitely things in main.c which really belong in src, things like inserting and deleting lines from a script, which currently make quite intrusive modifications of the arrays, rather than calling a higher level function like e.g. tele_script_insert.

  • The algorithm state is now represented by 3 structs, scene_state_t, exec_state_t and command_state_t, see state.h for details. The only static variable left in the algorithm code is a static scene_state in teletype.c, at some point soon that will be removed and all the functions will expect a scene_state_t as the first argument (the ops already do).

  • Ops have now been broken out into separate topical files. This is to keep like minded code together and make it easier to see ways to reduce code duplication. A good future example of this will be when I (or someone else) creates PN.* versions of all the P.* ops.

  • Scripts are now executed using run_script and run_command which take care of creating a valid exec_state_t (process is still there doing the actual business). This means that the behaviour of IF, ELSE, and ELIF have changed subtly. But on the plus side it should be trivial to add a STOP op to halt the current script.

The big things left to do are…

  1. Get a grip on main.c
  2. Get rid of the static scene_state
  3. Rewriting the parser to not use strcmp (probably via ragel)
  4. Build a better user documentation system

To the best of my knowledge everything still works properly, but I haven’t been able to test it a lot. Given how much has changed I find it hard to imagine that some bugs haven’t slipped in.

The code itself is still a WIP, but I’m probably not going to have much time to work on it for the next month. So it seems like a good idea to get it merged in now. Any questions please ask either here or on the issue.


Just a quick thought on the i2c stuff, I’ve never really used it before and I know very little about embedded programming outside the teletype codebase, but…

I’d say it’s important to over engineer the protocol, given how many different modules (and alt firmwares) will be using it, it’s going to be very difficult if we need to continually update them every time it changes. Even more important, if we do need to change in a breaking way, there needs to be a way for those messages to be rejected by devices running older codebases. It would be terrible if someone was to buy a new meadowphysics, connect the II bus and then have their teletype constantly crash due to protocol incompatibilities.

3 Likes

doing a big review today of this! looks amazing. thank you so much. it’ll take me a bit to fully wrap my brain around what you’ve done here.

re: i2c, @zebra and i are definitely accounting for very broad use cases. we’ll deal with firmware updates to keep other things up to date and deal with message breakage.

1 Like

thank you for this huge effort! will be reviewing the PR / commenting sporadically this week.

re: i2c - agreed on over engineering it and accounting for potential future use cases. would be great to have the ability to pass different number of parameters for different commands, and to be able to pass an array of values as well [such as pattern data].

i think back compatibility shouldn’t be a huge issue? since all the existing firmwares could be updated prior to that, and then when folks update their teletype it’s easy to update all the modules at the same time? but perhaps the updated protocol could start with a byte that is not currently reserved for anything so any modules running the old firmware could just ignore the new II commands [assuming they can deal with more data being sent].

1 Like

I’m about for the next fews days if anyone does have any Qs. After that response times might be a bit slow (till late June).

The sketchiest bit of the code is the most recent, the script array getters and setters, basically it’s unfinished, eventually it will be replaced with more semantic functions (e.g. tele_script_insert), but replacing the direct array access with functions seemed like a useful stepping stone when I was refactoring.


On the i2c front, I’m guessing that @zebra has had some experience with this via the e bit of the Buchla 200e system? That’s i2c based too isn’t it? Practical experience trumps theoretical knowledge…