Use of yield() in Teensy Cores and Libraries

[shawn]

@h4yn0nnym0u5e would you then advocate for having a user call SomeLibrary.loop() inside their main program loop, plus anywhere they’d wait, instead of hooking into yield (whatever the mechanism)? (Eg. QNEthernet)

 

[h4yn0nnym0u5e]

I would prefer that they didn't have to do that, because to my mind using an EventResponder instance to hide SomeLibrary's requirement for regular polling is very elegant. Any extra thing the user should do, but can forget, inevitably means that some will! (The number of audio applications where the user forgets to use AudioMemory() is a case in point).

However, in the likely absence of a wholescale overhaul of the yield/EventResponder infrastructure as suggested in post #50, I think at least having an option to call SomeLibrary::poll() (I've quietly renamed it and made it a static method there...) instead of linking into the EventResponder yield() list is probably the way to go. I'll try it out in the audio library branches I'm tinkering with to see how it goes. But I would 100% be open to other approaches, especially if they make the user's life simpler.

Having said that, without EventResponder/yield "just working", it's hard to see anything being much simpler than having to add SomeLibrary::usePolling() in setup(), and SomeLibrary::poll() in loop()...

 

[joepasquariello]

Clearly if you're creating/using a co-operative OS you know to keep to the "one task, one piece of hardware" rule, so you're OK. I guess you could get into issues if you use QNEthernet with SPI-based Ethernet hardware plus, say, a SPI display, because then you'd have to know SPI uses yield() (which the naïve user doesn't because it's not documented). And then your task has to run both the display and Ethernet, which is messy.

The hardware resource that is shared and requires protection is the SPI peripheral, not the external devices.

If you're a library writer wanting to take advantage of facilities built into cores, then limitations need to be documented much better than they currently are. EventResponder is nearly 6 years old, yet still documented as "an experimental API". The documentation for attach() states "Calls from yield() allow use of Arduino libraries, String, Serial, etc." - as I've discovered, this is simply not true in the general case: you can't safely use serial, Wire, SPI, SD, ADC, USB ...

Calls to yield() are found in all Arduino cores and many libraries, and that fact has to temper what you do if you override it with something else, such as EventResponder.

I'm still not sure what would be a minimal, useful, non-breaking set of changes to vanilla Teensyduino (putting slightly aside super-users like @joepasquariello, while still trying to make sure their existing codebase is unlikely to break - I would never advocate that!).

This would be a breaking change, but I think it's the right thing to do. Modify EventResponder to not override yield(), and instead just have a function that must be called from loop(). Users could still attach to it, but because they'd need to explicitly call it from their sketch, they'd know where it was called and perhaps be better able to avoid problems or find and fix them when they occur. I think this has to be okay because EventResponder can be used effectively with sketches that contain only one call to yield(), i.e. the one in main().

I'll post my cooperative OS so you can see how simple it is, and I hope you'll try it. Arduino is mostly incompatible with preemptive OS. I'm guessing the originators made the choice not to have a preemptive OS to avoid requiring every developer to understand that their code could be switched out at any moment. I'm glad they provided a hook for cooperative OS, which provides a lot of the benefit and can be used safely.

 

[shawn]

For the record, the QNEthernet library doesn’t have to use EventResponder or hook into yield(). In fact, that’s how it started out. The only thing that you need is to call Ethernet.loop() regularly. (And comment out the attachLoopToYield() call.) The reason I added the hook was to make it easier on developers.

The only thing missing is a way to configure this feature to be present or not. I’m in the process of adding that and will commit the change when I get a chance. It’ll likely be a configuration macro in qnethernet_opts.h.

 

[jmarsh]

I don't think Teensyduino requires any changes at all because as it is, it works as designed.
The problem in this case seems to lie with hooking extra things into yield via EventResponder without appropriate safety checks. If you're going to do that you have to be prepared for the consequences, similar to accessing a non-threadsafe resource from two separate threads - hence why EventResponder can be extended to add any extra functionality required.

 

[shawn]

I don't think Teensyduino requires any changes at all because as it is, it works as designed.
The problem in this case seems to lie with hooking extra things into yield via EventResponder without appropriate safety checks. If you're going to do that you have to be prepared for the consequences, similar to accessing a non-threadsafe resource from two separate threads - hence why EventResponder can be extended to add any extra functionality required.

I think the issue is that libraries can do this, not necessarily user code, meaning there’s less control. Your point stands, however, for library authors, especially; they need to take extra care and add extra documentation.

 

[joepasquariello]

I think the issue is that libraries can do this, not necessarily user code, meaning there’s less control. Your point stands, however, for library authors, especially; they need to take extra care and add extra documentation.

Yes, I agree with you and @jmarsh, the responsibility is on the programmer, but if EventResponder was not called from yield(), at least the users would not face the issue of calls to yield from additional places each time they add the use of another library that uses yield().

 

[shawn]

Yes, I agree with you and @jmarsh, the responsibility is on the programmer, but if EventResponder was not called from yield(), at least the users would not face the issue of calls to yield from additional places each time they add the use of another library that uses yield().

My view of yield() is that it could be called at any time from anywhere, including multiple times. Or were you referring to yield() calls being called in a reentrant manner, I.e. someone calls yield(), and then somewhere along the line, inside that yield() call, another yield() call is made?

In any case, I think it’s a reasonable assumption that some library is going to call yield() somewhere, so it’s to be expected. What’s wrong with yield() being called from additional places?

 

[jmarsh]

I think the issue is that libraries can do this, not necessarily user code, meaning there’s less control. Your point stands, however, for library authors, especially; they need to take extra care and add extra documentation.

I mean it's the responsibility of whoever is writing the EventResponders that are getting hooked in. Library authors shouldn't need to document whether they call yield() or not (they may not even be aware of it if it happens in some platform dependent code).

if EventResponder was not called from yield(), at least the users would not face the issue of calls to yield from additional places each time they add the use of another library that uses yield().

That doesn't make sense. If a library uses yield(), surely that counts as an additional place that it can be called from.
Even delay() calls yield(), so it really is necessary to assume it can be called from anywhere at any time.

Or were you referring to yield() calls being called in a reentrant manner, I.e. someone calls yield(), and then somewhere along the line, inside that yield() call, another yield() call is made?

That case is harmless since there's a static flag used to detect recursive calls.

 

[joepasquariello]

That case is harmless since there's a static flag used to detect recursive calls.

That's good. Nothing wrong with calling yield() when yield() is a task switch, because in that case it's not reentrant. Each yield() is just a point in a big loop that eventually returns.

 

[shawn]

I mean it's the responsibility of whoever is writing the EventResponders that are getting hooked in. Library authors shouldn't need to document whether they call yield() or not (they may not even be aware of it if it happens in some platform dependent code).

Yes, I was referring to EventResponders, not uses of yield().

 

[h4yn0nnym0u5e]

The hardware resource that is shared and requires protection is the SPI peripheral, not the external devices.

Of course. But if multiple external devices use a single SPI peripheral via libraries which deeply embed the peripheral access, how do you propose to keep those from tripping over one another? If one yields, and does not provide information that the SPI peripheral is busy, the other may try to execute and cause mayhem.

Calls to yield() are found in all Arduino cores and many libraries, and that fact has to temper what you do if you override it with something else, such as EventResponder.

And I'd be more than happy to do so. But there is no documentation, and no information provided by the yield call as to whether, in any specific instance, I need to temper what I do. The net result is I cannot use EventResponder yield mode at all, in any circumstances other than the very simplest i.e. tell the user of my library that they don't have SD card access any more.

This would be a breaking change, but I think it's the right thing to do. Modify EventResponder to not override yield(), and instead just have a function that must be called from loop().

I'm trying very hard not to advocate any breaking changes. I think that's true of the changes I outlined in post #50, but happy to be challenged on that point. I am also happy to extend EventResponder to add an option for a trigger response function that's explicitly called from loop().

I mean it's the responsibility of whoever is writing the EventResponders that are getting hooked in. Library authors shouldn't need to document whether they call yield() or not (they may not even be aware of it if it happens in some platform dependent code).

And as a writer of an EventResponder-hooked library, I'm happy to take on the responsibility. But why on earth are other library writers absolved of the responsibility to document the code they write? If they use yield() they should document it, along with any other library dependencies so at least if there's platform-dependent code the user can eventually trace the full dependency tree.

Lack of up-to-date documentation is a major stumbling block. I've been working on my code for over two years, and never even considered that it would be considered acceptable to put a yield() call right in the middle of the SdFat library at the deepest level of hardware access.

That doesn't make sense. If a library uses yield(), surely that counts as an additional place that it can be called from.
Even delay() calls yield(), so it really is necessary to assume it can be called from anywhere at any time.

Indeed. And as things stand, we have to deal with that. But is it so unreasonable to hope that non-breaking improvements can be made such that anything that hooks yield() can at least get some information as to the state of the system when the hook code is entered? It can then decide either to execute or wait for the next opportunity.

 

[h4yn0nnym0u5e]

The problem in this case seems to lie with hooking extra things into yield via EventResponder without appropriate safety checks. If you're going to do that you have to be prepared for the consequences, similar to accessing a non-threadsafe resource from two separate threads - hence why EventResponder can be extended to add any extra functionality required.

Sorry, forgot I was going to add this to my response...

As noted, I can't do any safety checks - I don't have the information available at the point my hook code is called, with the system as-is.

And ... I don't see how EventResponder can be extended to add extra functionality. There's not a lot can be done by deriving from it, and as noted in post #50 I've not found much luck with proposed changes, so I don't expect much joy from submitting a PR.

 

[jmarsh]

Well you haven't really explained or provided an example of what the problem is - all we know is that the SD library calling yield() causes your EventResponder to be executed which then attempts to use the SD library. But how is the SD library being invoked in the first place, i.e. where is the actual conflict coming from? That code should be masking/unmasking the EventResponder. If that means wrapping all access to SD then so be it, that's the price for using a different thread-model than the one it was designed for.
EventResponders attaching to yield should be treated like ISRs; assume they can be triggered at any time and design the code appropriately. It's not up to the library authors to make their code threadsafe because they literally have no knowledge of what thread model the library will be run under.

 

[h4yn0nnym0u5e]

Sure no problem. I know a piece of code is canonical, but there are a lot of moving parts so I'll try in words to begin with:

• EventResponder hook set up to read audio data from SD card when triggered
• audio library update() [interrupt] triggers EventResponder
• yield() calls EventResponder hook
• hook buffers more audio data from SD to where audio update can read it next time around

Buffers are big enough that slowish SD response within hook doesn't matter.

Now, the user code:

• has started playing audio from SD
• wants to read or write data on SD, e.g. a play list or something
• opens file, starts access
• access is going to take some time, according to SdFat, so it calls yield()
• which calls EventResponder hook
• which tries to buffer more SD data, but crashes because it doesn't know the yield() came fromSdFat

By your reckoning, before the user does the playlist access, they have to unhook all the EventResponders, then hook them back up again when they're done. There's one per playback object - on the forum thread where this came to light, there were 32 objects instantiated. My library currently doesn't keep a list of them, because I didn't foresee the need to do so - it only hooks into yield(), after all, and that only happens at loop() end, delay() and calling yield() directly, right? Well, wrong, of course but ... documentation? Hell no.

EventResponder as it stands doesn't provide any masking function. That's the last item in my proposed list in post #50. In fact, if you unhooked all the EventResponders, you'd lose any pending triggers, so the library would probably break anyway.

I am not asking library writers to make their code thread safe. I am asking that when yield() is called some information can be provided as to the source of that call. If no information is provided then maybe EventResponder shouldn't be called at all? No library changes needed, except for end of loop() which calls default_yield_from(YieldSource::Loop), and delay() which calls default_yield_from(YieldSource::Delay). EventResponder won't be called quite as often, but no-one should be depending on any specific time-frame anyway.

In the absence of that information, the EventResponder hook writer cannot "design the code appropriately" - it's just unusable for a vast array of potential use cases.

I don't anticipate this getting resolved and a PR accepted any time soon, so for the time being I'm just going to have to grit my teeth, re-invent a slightly rounder version of the square wheel currently on offer, document it, and support everyone who doesn't use it when needed.

 

[jmarsh]

My library currently doesn't keep a list of them, because I didn't foresee the need to do so - it only hooks into yield(), after all, and that only happens at loop() end, delay() and calling yield() directly, right?

No. That is a misconception. yield() is a common OS-style API function that is free to be called by any code, and anything connected to an EventResponder should be treated with the same respect as an ISR - in fact this would probably be more "safe" since there's no guarantee yield() will actually get called between audio updates (how would more audio data be loaded in the case that the main thread is busy inside a large loop?), although obviously that will still suffer the same problem if the user is accessing the SD card outside the ISR.

With regards to masking all of your EventResponders - all you need to do is put a single static variable in your derived class and toggle it on and off to notify all of the derived objects that the SD library is currently unusable.

 

[shawn]

No. That is a misconception. yield() is a common OS-style API function that is free to be called by any code, and anything connected to an EventResponder should be treated with the same respect as an ISR - in fact this would probably be more "safe" since there's no guarantee yield() will actually get called between audio updates (how would more audio data be loaded in the case that the main thread is busy inside a large loop?), although obviously that will still suffer the same problem if the user is accessing the SD card outside the ISR.

With regards to masking all of your EventResponders - all you need to do is put a single static variable in your derived class and toggle it on and off to notify all of the derived objects that the SD library is currently unusable.

The whole phrase looks like it was (may have been edited, I'm not sure): "it only hooks into yield(), after all, and that only happens at loop() end, delay() and calling yield() directly, right? Well, wrong, of course but ... documentation? Hell no."

 

[shawn]

So I've made some latest changes to the QNEthernet library. There's a new QNETHERNET_DO_LOOP_IN_YIELD configuration macro. I'd love all'y'all's thoughts on it, including what's in the README. Suggestions and improvements welcome. (Simply search the source for that word.) (@jmarsh, @h4yn0nnym0u5e, @joepasquariello)

 

[h4yn0nnym0u5e]

No. That is a misconception. yield() is a common OS-style API function that is free to be called by any code, and anything connected to an EventResponder should be treated with the same respect as an ISR - in fact this would probably be more "safe" since there's no guarantee yield() will actually get called between audio updates (how would more audio data be loaded in the case that the main thread is busy inside a large loop?), although obviously that will still suffer the same problem if the user is accessing the SD card outside the ISR.

With regards to masking all of your EventResponders - all you need to do is put a single static variable in your derived class and toggle it on and off to notify all of the derived objects that the SD library is currently unusable.

As noted by @shawn, you’ve edited what I said in a slightly misleading manner. I did think that, I now know better…

I think the way to think of it is that user code is running in an ISR, and yield() acts as the ISR return. The EventResponder triggers are like lower priority ISR flags which only get serviced on yield(). Either way, access to the card is happening at two levels, and this is a no-no (although the audio library has been doing exactly that for ages…).

Indeed, it’s a “feature” that a slow loop() will starve the streaming audio. I documented the fact, and not ideal. You can sprinkle yield() calls around if you must, or let loop() exit regularly.

Oh, for the simplicity of a static flag to say “SD unsafe”! By the time you set it, events are already pending (set in the audio update ISR), and yield() will execute them. No flag in EventResponder to prevent that.

 

[h4yn0nnym0u5e]

Oh, for the simplicity of a static flag to say “SD unsafe”! By the time you set it, events are already pending (set in the audio update ISR), and yield() will execute them. No flag in EventResponder to prevent that.

However … there is a flag to prevent yield() from running EventResponder. It’s probably not intended for libraries to use directly, and blocking all response execution is definitely overkill, like putting in a critical section when there’s only one other task you don’t want scheduled. But having that option while the experimental EventResponder API is being improved is … worth considering.