#81

@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

#82

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

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

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

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

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

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

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

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

#90

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

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

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

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…

#94

the 200e has a lot of i2c. we’ve been discussing it.

i’m wading through the PR and haven’t come up with questions yet, though i have much more to cover.

1 Like
#95

started looking into https://github.com/monome/teletype/issues/16 [key behavior: repeat on hold] somewhat selfishly wanting to specifically add support for holding backspace while editing, and right now it’s implemented separately for each supported key in handler_KeyTimer.

it’d be trivial to add it for backspace only, but thinking it might be good to refactor it to support any key hold by extracting key processing into a separate function and then calling it from both handler_KeyTimer and handler_HidTimer, so in essence the latter will imitate a key press when appropriate. any concerns / objections for such change?

#96

made the change as described: https://github.com/monome/teletype/compare/master...scanner-darkly:dev
did a quick test and everything seems to be working okay. will do a pull request after i test it a bit more.

3 Likes
#97

thank you for breaking out process_keypress as i was going to do that myself!

#98

going to refactor it a bit, remove global variables for the modification keys and add a parameter for specifying when it’s a held key as in some cases repeated key should be ignored (like saving a scene, for instance).

realized makefile for clang-format is Mac specific, is there a way to write in a way that would autodetect the OS and run a different command on Windows?

#99

https://github.com/monome/teletype/pull/44

finished key repeat implementation. reformatted earlier code a bit (got rid of global key variables in process_keypress) and also key repeat will now be ignored for writing and reading from flash. key repeat will still work for triggering scripts - thought it might be useful to have it work that way…

also found a couple of minor bugs from @sam’s refactoring, pattern length wasn’t getting properly updated in some cases, and [ and ] didn’t update values when in TRACKER mode, that’s fixed now. also made a change to ENTER behavior when in TRACKER - it extends pattern length when pressed on the last step as before, but now it will also go to the next step regardless of where it’s pressed, not just when on the last step - this seems like a good thing to have.

and discovered a few key combinations i didn’t know about! CTRL and CTRL+SHIFT in TRACKER mode specifically is rad, very handy to be able to set pattern values with the PARAM knob…

5 Likes
#100

thank you for this! and yes good suggestions with the key behavior.

1 Like