Expected action of EventResponder?

[h4yn0nnym0u5e]

This is probably a question for Paul, so I hope he's got some bandwidth!

I've been making regular and successful use of EventResponder to perform what I understand to be its intended action, to trigger non-interrupt-safe code from within an interrupt. I my use cases it's typically audio-related, triggering events from within the audio update to cause stuff to happen as soon as yield() is called.

Despite having looked at the source fairly often, I never spotted until just now that one yield() processes just one triggered Event, no matter how many are pending; at least, that's the way it appears to me. I'd been expecting all pending events to be processed as soon as yield() is called, and it seems to me that logically this should be the case. The comments say "events are often triggered within interrupts or other timing sensitive code. EventResponder can call your function a short time later", but if it takes 10 iterations of loop() to process 10 events, there's a huge risk of the short time becoming very long indeed.

Seeking opinions on whether I'm (a) wrong about what the existing code does, (b) wrong to expect anything else, or (c) right and EventResponder needs updating... If it's important to preserve the existing action for legacy reasons, perhaps either a triggerNextYield() can be added (every event triggered this way is guaranteed response on the next yield() call), or a static flag could be added to switch between the existing single-response and new all-responses behaviour.

Happy to add the feature and submit a PR, if it'll help.

 

[h4yn0nnym0u5e]

No response, so I did it anyway. PR#703.

 

[PaulStoffregen]

I'd been expecting all pending events to be processed as soon as yield() is called, and it seems to me that logically this should be the case. The comments say "events are often triggered within interrupts or other timing sensitive code. EventResponder can call your function a short time later", but if it takes 10 iterations of loop() to process 10 events, there's a huge risk of the short time becoming very long indeed.

The original idea behind calling just 1 event function was to minimize (or balance) the impact to busy loops which call yield().

In an ideal or good case scenario, programs should call yield() frequently and event functions should run as quickly as possible. It a not-as-good case, we could end up with busy loops that normally respond very quickly getting significant delay. Events could have significant latency before they run. Both could happen. It really is the nature of cooperative scheduling. We trade away predictable timing for the safety of not interrupting other than at yield().

So I really don't have an absolute hard answer. Originally I had planned on trying to limit the impact of adding yield() inside libraries which block. Whether that was the best choice, or even a good one, could be debated.

Really what I mean be this non-answer is I'm looking for feedback from anyone who wishes to chime in. Should we favor running all or at least more than 1 pending event on yield() calls?

 

[h4yn0nnym0u5e]

Thanks for that Paul. I can absolutely see your reasoning there, and the aim of my PR is to leave the default action of EventResponder the same, while allowing other options open to the programmer.

My feedback is from the point of view of writing improved versions of audio objects providing playback (and recording) from filesystems. The SD library reacts really badly to mixed access from interrupt and foreground code, so using EventResponder to provide pre-loaded data to the audio updates seemed a good approach. It's easy to use, scales well with the number of playback objects, and the only real complexity is getting the pre-load prediction correct for looped or non-real-time playback. However, the typical use case often requires multiple object instances (e.g. 8-channel streaming for a looper, or for an 8-voice sampled synth), often combined with a display which typically seems to take many milliseconds to update. So if you get a multi-millisecond loop time, and can only update one voice per loop, you end up with insane buffer sizes or stuttering playback. Again.

It would probably be possible to add yet another layer of control onto the updated EventResponder, whereby only a limited number of events get a response on any given yield(). At the moment, it's either all the ones that requested next-yield response plus one of those that didn't, or all requests (if the sketch has used EventResponder:rocessAllEvents(true)). It might also be useful to instrument the EventResponder system so each object records its last and maximum execution times, much as the Audio library does.

 

[defragster]

Tough call - nothing is free ...

>If loop() is fast - the repeats to yield() chip away at the task list in good time

>If loop() is slow, or the list is long, it will persist longer and repeat less often

In the second case things are already toward maxed out and dwelling in yield() to process everything will make the yield() like a delay(?) ...

Having a way for user to express yield() depth [1 ... ALL] would allow choice to have the EventResponder runs as best fits.


@Paul: scanning the PR and saw 4 files and lots of Red/Green in the edits ... was wondering if the new callback( params ) had a place to integrate here?

 

[h4yn0nnym0u5e]

It's effectively only two files, a pair in each of the teensy3 and teensy4 folders, EventResponder.cpp and .h

I'm really trying to figure out the best thing to do for your second case - as you say, if loop() is fast there tends to be no visible issue. But if loop() is slow, as so often happens, then the event response latency for multiple events becomes so slow that there's essentially no point using EventResponder. In the case of display updates, a fairly slow loop seems to be unavoidable - a 30Hz display update rate means over 11 audio updates occur per loop, and a need to average loading ~11kbytes per yield if playing back 8 files from a filesystem. One load per file per loop means every file needs >264ms of buffer…

I've just instrumented one of my test cases (just 2 files being played) which has a fast loop time, and the vast majority of yields take <10us (presumably, no event response needed), the ones which do need a response typically take <200us, and never take >1000us. If I put in an artificial stall lasting 40ms, and extra yield() calls to simulate my proposed EventResponder change, then 90% of loops get <1000us added to them, and <0.3% get 2-5ms added. I suspect that wouldn't be noticed, though it could be that some users would spot it.

It could be worth looking at some sort of user-controlled limit on how many event responses are run per yield(), but only if the basic principle of multiple responses per yield is acceptable in the first place. My PR is backward-compatible with the existing behaviour (well, nearly so. It won't crash if you trigger an event without attaching a response function...). And even as it stands, people can always misuse it!

 

[h4yn0nnym0u5e]

Really what I mean be this non-answer is I'm looking for feedback from anyone who wishes to chime in. Should we favor running all or at least more than 1 pending event on yield() calls?

... tumbleweed ...

Can I put in a plug for this at least to go into the next Teensyduino beta? As far as I can see:

• no-one's objecting
• no-one's even using EventResponder in Teensyduino libraries
• it's backward-compatible with existing code

The proposed behaviour is pretty much vital to the event-response-driven filesystem audio playback. Yes, it could be achieved by telling users to add [blocks of] yield() calls at "suitable" intervals in their code, or disguising it by mandating frequent calls to a function AudioPlayWAVbuffered::updateBuffers() which just wrap yield(), but that just swaps one potential support nightmare for another.

As noted previously, I would happily consider adding other safeguards, but would prefer only to do so in the face of a demonstrable issue that needed fixing. We might get such, or even decide it's too dangerous and remove it for now, if we got some evidence from a beta.

 

[defragster]

It's effectively only two files, a pair in each of the teensy3 and teensy4 folders, EventResponder.cpp and .h
...

Failed to post reply 2 weeks back ...

Wasn't saying it was too much code - just that it wasn't all 'parsed' - assuming it works as claimed to address what seems a valid issue.

And wondering if @PaulStoffregen had EventResponder 'plans/goals/future' that would conflict or justify an edit including this for beta.

 

[KurtE]

I honestly don't know what the right answer is here. When I have used the event reponder in the past, with things like DMA SPI transfers, I wanted the code to run then and there, so I had it run on the interrupt and not wait for other mechanism like yield()...

As for finding stuff in beta, probably would not hurt to try... Not sure when the next one might be. I think Paul still has his hands full with other stuff, like finding parts.

 

[h4yn0nnym0u5e]

I’d like to think it Works As Intended, but I’m just as good as the next person at including bugs in my code Hence suggesting it goes in a beta for people to try, or ignore, as they wish.

The big advantage of using ER to trigger yield code is it plays nicely with SdFat, which gets deeply grumpy if called from both foreground and interrupt. But as you say, it’s flexible enough to trigger immediately if that’s what you need.

 

[jmarsh]

I can see this causing mystery problems for people in the future, of the variety "Why am I getting periodic dips in performance every X ms" when a bunch of yield functions get procssed together instead of separately.
I think if you have a collection of callbacks that need to be processed as a single group, it should be up to you to organise them like that rather than changing the framework.

 

[h4yn0nnym0u5e]

In my use of ER I’ve actually gone to some effort to avoid my callbacks being processed as a group. There’s no advantage to doing so, and as you say it could make loop() timings significantly variable. Not that anyone should depend on that anyway….

The issue I’ve encountered is that sometimes, or even often, multiple events can “pile up” while the CPU is busy. A prime example is display updates, which tend to involve shifting large amounts of data over a slow and blocking connection. Say that takes 50ms, and you have 8x 44kHz mono audio files streaming at the same time. That means 8*50ms=400ms of audio data might be pending loading. Spread over 8 files you don’t usually get the full 20MBps SD card speed, it might be more like 8MBps, so the 34kB would add a “lump” of about 4.4ms to the loop time. Call it 10%, and note that if the display is updated every loop then the lump occurs every loop too, so it’s no longer causing a dip in performance, it’s just part of the equation.

Processing one event per loop will still require loading 34kB, but it’ll have to be for just one audio file, so we suddenly need 8x the buffer memory. Assuming a conservative strategy of reloading when half empty, the total buffer size goes from 70kB to over 550kB.

Obviously I can’t legislate for how for how others might “misuse” this. We’ve all seen posts where people’s code does a serial write or a busy loop inside an interrupt service routine…

Yes, I could effectively clone ER within my code, but it doesn’t seem terribly sensible given the right tool is so very nearly available, and would be available with a fairly minor change. And doing that clone would potentially result in the exact same performance dips anyway.