[Request for comment] Abstracted timers from schedulers

[rob-deutsch]

This PR abstracts the 'timer functionality' out of the igntion scheduling functions, and creates a single common ignition scheduling function (setIgnitionSchedule) which is used for all ignitions.

This allows more flexibility for which/how timers are used, and will allow quicker/easier improvements to the scheduler. Not to mention, this is needed for the ESP32 port.

The same sort of abstraction could be added to the fuel scheduling functions.

[muuris]

I can't see ESP32 port being viable. There are other MCUs already that do the job and don't panic boot on every little exception. ESP32 is great for other, non-critical things.

[rob-deutsch]

I think that this abstraction is valuable regardless of whether Speeduino is ported to ESP32 or not. I've amended the OP to make that clearer.

[adbancroft]

I'm all for these abstractions.

(disclaimer: this is an area of the code I'm not familiar with)

It's an invariant that the callbacks are always in pairs, correct? And mostly processed by the code as pairs. So we could encode that invariant into the source code, so it can't be easily violated:

typedef extern void (*ignStartFunc)();
typedef extern void (*ignEndFunc)();

typedef struct IgnitionCallback {
  ignStartFunc startFunc;
  ignEndFunc endFunc;
} IgnitionCallback;

extern IgnitionCallback ignitionCallbacks[8];

It's the same memory footprint, just as fast (if not faster) and improves readability

[adbancroft]

Should the callback array be of size IGN_CHANNELS?

[noisymime]

I'm OK with the abstraction here, in particular wrapping the schedulers up in arrays and reducing set/interrupt functions down to singles. At a read over though, this will introduce some slower areas and the impact of these changes will need to be tested.

In particular increasing the number of function calls (even inline ones) from interrupts is something that should only be done if it can't be avoided. Likewise anything that introduces more overhead in interrupts (Such as what setIgnitionCompare() and getIgnitionCompare() are doing) isn't ideal. The benefits may outweigh the drawbacks, but it needs to be measured.

Some of this might be able to be avoided by using macros rather than functions, which avoids another context switch, but it may be difficult in other areas.

[rob-deutsch]

Thank you @noisymime, I appreciate the comments. I have some ideas and some thoughts to mull over. I'll submit improved updates.

@adbancroft, you are correct, and there are other areas of the codebase that can be improved (both readability and performance) because of this abstraction. I would like to keep this PR limited to the 'mininum' needed to implement to the abstraction, though, so its easier to understand and reason about. Future improvements can then be tackled in seperate PRs.

To respond to each of your particular suggestions:

1. Combining the two callbacks into a single struct seems neat. That hadn't occured to me. I also note that Ignition schedulers update #578 suggested adding references to the callback functions in struct Schedule.
2. Correct, the callback array should be size of IGN_CHANNELS. But, I would not suggest changing that in this PR, since we'd end up 'going down a rabbit hole' and also refactoring part of initialiseAll().

[DeionSi]

Did some performance comparisons on the latest commit (01f4eb9) versus this PR rebased on the same commit. This is on a mega2560 on a bench with ardu-stim.

At 1000 rpm, loops/sec drops from 944 to 913
At 3000 rpm, loops/sec drops from 882 to 854
At 6000 rpm, loops/sec drops from 770 to 745
In other words a performance hit of ~3,2%.

[DeionSi]

Why not include the register addresses in the Schedule struct? That way the getIgnitionCounter and setIgnitionCompare functions can be replaced by ignitionSchedule[x].ignitionCounter and ignitionSchedule[x].ignitionCompare. I'm not good with pointers but I think it should work and be pretty fast?

[DeionSi]

So I replaced getIgnitionCounter and setIgnitionCompare with arrays and macros. This gives no change in performance.