More a suggestion or offer ... of incorporating my local changes of 1.4.3 into 1.5.1

[brewmanz]

I've been using AceRoutine 1.4.3 for a while (and made a few local tweaks), and considering the upgrade to 1.5.1.
TBH I haven't checked your latest code, but reading your change descriptions, I expect my tweaks are not fully compatible (which is my problem, not yours).
My local changes were done primarily to save ROM & RAM, keep track of status changes, and timings, for debugging purposes.
Anyway ... are you interested in me adding my tweaks as a Pull Request, once I've done the work? (Yes, I should have done this a year ago, but ...)
FWIW, squashing my changes and looking at them, they involve:

• add void Coroutine_##name::printName(Print* pPrinter) const override;
• '#define ACE_RUN_TICKS 3' for timing in mS, or 6 for uS, for timing displays.
• store line number when changing status; nice to know WHO made the status change!
• add class ICoroutineVisitor, to visit all existing CoRoutines (this is handy for displaying stats)
• split status Yielding into Yielding and Cold (never started)
• add status Awaiting (it's ready to run but not been scheduled)
• been 'creative' with status values; using a variety of compile-time options of:
• • single characters, such as 'Y' for Yielding; makes display/print of human readable status easier
• • 32 bit values hammered into sz 3-letter Words such as 'Yld' and 'Sus'
• • Use F() strings to display full status name from single byte status e.g. 'Terminated' or 'Trmntd' (choice via compile time option)
• keep track of running time without yield or delay
• keep description (as F() string) of reason (or time delay period) for each status change

Regards

Bryan 'brewmanz' White

PS I could do them as multiple Pull Requests, by feature

[bxparks]

It seems like some (but maybe not all?) of those could be accepted into the upstream. Instead of squashing all those changes, it would be nice if you could separate each feature into a separate commit so that I can take a look at them one at a time. Also, you'd have to rebase your changes on top of the current HEAD.

In regards to specifics... see interleaved comments below:

I've been using AceRoutine 1.4.3 for a while (and made a few local tweaks), and considering the upgrade to 1.5.1. TBH I haven't checked your latest code, but reading your change descriptions, I expect my tweaks are not fully compatible (which is my problem, not yours). My local changes were done primarily to save ROM & RAM, keep track of status changes, and timings, for debugging purposes. Anyway ... are you interested in me adding my tweaks as a Pull Request, once I've done the work? (Yes, I should have done this a year ago, but ...) FWIW, squashing my changes and looking at them, they involve:

* add void Coroutine_##name::printName(Print* pPrinter) const override;

There's a new Coroutine::printNameTo() which I think does the same as this. Not sure why yours is virtual, but I like to avoid virtual if at all possible, to save memory, not necessarily for the runtime overhead.

* '#define ACE_RUN_TICKS 3' for timing in mS, or 6 for uS, for timing displays.

Not exactly sure what this is, but I avoid passing configuration options through #define macros, because it's clumsy. Rather, I prefer an instance variable, or sometime, a template parameter.

* store line number when changing status; nice to know WHO made the status change!

You mean save the value of the __LINE__ macro? Seems like you'd want to save the __FILE__ as well, no? The concern here would be the additional bookkeeping overhead, though it's only 2 bytes for __LINE__`` and 2 bytes for FILE`, plus the string of the file name itself.

* add class ICoroutineVisitor, to visit all existing CoRoutines (this is handy for displaying stats)

I am not a fan of the visitor pattern. For most people, too difficult understand, too hard to follow the code execution, hard to maintain, hard to debug. The Coroutine::getRoot() is publically exposed, so the client application can walk the singly-linked list. Is that not sufficient?

* split status Yielding into Yielding and Cold (never started)

Sound useful, even though I don't think I've ever needed it.

* add status Awaiting (it's ready to run but not been scheduled)

Not sure what is the difference between Cold and Awaiting.

* been 'creative' with status values; using a variety of compile-time options of:

* * single characters, such as 'Y' for Yielding; makes display/print of human readable status easier

* * 32 bit values hammered into sz 3-letter Words such as 'Yld' and 'Sus'

Seems like micro-optimizing for not that much gain? There is now a Coroutine::statusPrintTo() which prints out the status as a human-readable string. If that isn't sufficient, the client application can always provide code to do the mapping anyway they want.

* * Use F() strings to display full status name from single byte status e.g. 'Terminated' or 'Trmntd' (choice via compile time option)

I think this is the same as Coroutine::statusPrintTo().

* keep track of running time without yield or delay

Could be useful to some people... if we can keep the memory size low..

* keep description (as F() string) of reason (or time delay period) for each status change

Not exactly sure what this feature is.

It looks like many of these features that you added were for debugging purposes. The concern I would have there is memory consumption. It that becomes an issue, I wonder if there is a way to create a "debug" version of Coroutine, or some other way for the client application to opt into some or all of these extra features, so that they pay for only what they use. Often, I implement this using C++ template parameters, so that the compiler can optimize out blocks of code which aren't used.

[bxparks]

It's been over 1.5 years, so I will assume that work is dead. But the discussion is interesting, so I'm moving this to the Discussions tab, which allows me to clean up the backlog of Issues.

[bxparks]

Edit: I can't subtract, it was 8 months ago.