Nested delays have issues

[samdoshi]

See https://llllllll.co/t/teletype-multiple-metronomes/9053

Essentially don't run commands while iterating through the delayed commands array, as a command may mutate the array.

Possible solution: remove the now commands into a temp array before executing them.

[burnsauce]

I tested the following workaround successfully this morning.

diff --git a/src/teletype.c b/src/teletype.c
index 4540507..f4ac949 100644
--- a/src/teletype.c
+++ b/src/teletype.c
@@ -276,6 +276,8 @@ void tele_tick(scene_state_t *ss, uint8_t time) {
         if (ss->delay.time[i]) {
             ss->delay.time[i] -= time;
             if (ss->delay.time[i] <= 0) {
+                // Workaround for issue 80. (0 is the signifier for "empty")
+                ss->delay.time[i] = 1;
                 run_command(ss, &ss->delay.commands[i]);
                 ss->delay.time[i] = 0;
                 ss->delay.count--;

[samdoshi]

Looking at the code scene_delay_t doesn't seem optimal:

typedef struct {
    tele_command_t commands[DELAY_SIZE];
    int16_t time[DELAY_SIZE];
    uint8_t count;
} scene_delay_t;

typedef struct {
    ...
    scene_delay_t delay;
    ...
} scene_state_t

count can be completely removed, I think it's redundant as it can be quickly inferred in those places it's needed. (It's only used to enable or disable the status icon in live mode.)

What do you think of something like this?

typedef struct {
    tele_command_t command;
    int16_t time;
    bool empty;  // or "bool full;" depending...
} scene_delay_t;

typedef struct {
    ...
    scene_delay_t delays[DELAY_SIZE];
    ...
} scene_state_t

I think only files to change would be state.[ch], teletype.c and ops/delay.c. All in all it's quite self contained. What I'd really like is that eventually the delay code is easier to understand (even if it's more verbose).

@burnsauce as you've generously offered to do some coding, do you want to start with this bug? I'm okay with the one line fix if you want. But alternatively going a bit deeper and rewriting the code might give you some insight into how it all glues together. (Feel free to say no!)

[burnsauce]

Cursory calculation indicates that this would increase memory consumption by 12 to 82 bytes, depending on byte alignment and the size of the enum tele_word_t.

As much as I love writing code, I'm inclined to lean towards the one-line, three(?)-instruction fix. I'm personally fine with the use of magic numbers for tracking, and would ultimately like to see delays processed more accurately via timer interrupts if that's a possibility down the road. (I love writing schedulers!)

[samdoshi]

Heh, there's 96kb of RAM, we don't use anywhere near the limit (the heap size is 73kb!). The MCU is also 60MHz so don't stress the instruction count so much either. (As an aside, I'd been thinking about moving some of the lookup tables from ROM to generated at boot and stored in RAM to free up ROM).

Make the PR once you're confident it works, I can offer any git/GitHub advice if you need it. (I can't promise I won't replace it later though... well... if I even remember...). Just make sure it's well documented (in both teletype.c and ops/delay.c), don't forget to make format. And make sure that DEL 1: ... isn't an edge case!

Also, re. interrupts. Were you about for the collective hangover we hand with those? Ah, interrupts and interpreted languages, so much fun. But, yeah the event queue is a single priority FIFO affair. If you've got some insight into adding a priority queue that would be appreciated. We just need to make sure that the keyboard and display code can never get starved of CPU time. There are also issues with locking access to the SPI bus, and maybe even too much locking happening. These issues feel like they're all tied up together. I could probably say a lot more on this. If you ever get to the point where you're comfortable enough with the codebase to attempt something like this, start a new thread on lines....

[burnsauce]

DEL 1 is certainly not an edge case. The only issue is that 0 is the identifier for empty, but also an identifier for "right now". Inside the conditional, it's "right now", where the time value is no longer read or required, excepting a command issuing a new delay, where it's the empty identifier.

I just tested it without issue, making an audio oscillator from the trigger output, stacking all delays up without issue.

I'll review some git docs to get comfortable with the concept of a PR and come back to you if I have questions.

[scanner-darkly]

We just need to make sure that the keyboard and display code can never get starved of CPU time. There are also issues with locking access to the SPI bus, and maybe even too much locking happening. These issues feel like they're all tied up together.

those should all be fixed in my branch, although it requires a lot more testing before we could say that with sufficient confidence. locking can also be made more granular in my version as it deals with interrupt levels individually when pausing irqs. but first we have to decide on what is more important, timer interrupts or trigger interrupts. basically, do we want it to respond better to triggers (esp for audio rates) or do we want timers to be more accurate?

[burnsauce]

There's no reason we can't tune that and leave that up to the user once the queue is priority scheduled. Allow the user to swap between several tuned schedulers.

[burnsauce]

Forgot to mention, interrupt priority can be part of the scheduler profile.