Ignition and Fuel schedulers

Upgraded ignition and fuel schedulers. This PR includes:

1. Get fuel and ignition schedules to work as originally intended: dwell timing and fuel injection pulsewidth directly applied to the timers. Apparently there were some computational hardships and problems with having all integer types correct, to have integer math on the timers work correctly (regarding need to take account that all math must work correctly with timer counter that rolls back to 0 periodcally). That is all honed out by now.
2. No change in loop speed on atmega2560 or only a very slight drop.
3. Noticeable dwell timing accuracy improvement, observeable with logic analyser. No observeable random extra ignition events visible during testing. Tested with all speed ranges from 60rpm to 16000rpm.
4. Smooth, no misses, no glitches, transition from cranking to running ignition dwell and timing tested with simulator and logic analyser.
5. Replaces #727 and #578 and #653 , related to #686
6. New ignition mode also working and tested. With 60-2 trigger (harder case) seems to be working fine.
7. on Atmega3560 compared to master:133 Bytes less RAM used, 4324 bytes less flash used.
8. Updated ignition dwell limiter. update information 3.2023: Dwell limiter works also against possible race contitions when port register read-modify-write type writes are done in some other parts of the code. Fixed possible race condition in dwell limiter itself when next ignition pulse falls within dwell limiter range, to prevent dwell limiter affecting the next ignition pulse at high RPM.
9. New Unity tests for the new schedulers (468 tests total for schedulers on atmega2560, more for boards that have more channels)
10. (update information 3.2023) Fuel injection duty cycle range extended to 100% (previously limited to 95%). This means continuous ON fuel flow from injectors at extreme cases.

Sounds promising 🙂

I've added comments for a few things I've spotted whilst reviewing this, mostly where something is fundamentally changed from the current functionality.

Have tested through some of the most common setups and things generally seem to be OK. The resulting code is certainly cleaner, but there's quite a few areas where edge case checks have been removed, which makes me a little nervous. I'll need to do some more testing around these though

Here's a few more general comments anyway:

• The cleanup of the scheduler code is certainly overdue, but does come with some (Probably inevitable) downsides such as the increased time spent within ISRs and the additional context switching that will be required due to the extra function calls. The scheduler code in particular sees quite a number of function calls added to the ISRs, which is something that will need to be considered (Noting that the calls won't be inlined)
• Pretty much all the unit testing is broken in this PR as it stands at the moment. This will need to be fixed before pulling this in and obviously all the checks will need to be verified also.
• I'm seeing some weirdness on the fuel pulses at high rpm/load (Beyond the duty cycle limiter) where pulses are either not starting or not completing correctly. Need to dig into this more though.

@noisymime thanks for the comments. Today I managed to reorganize things so that I got rid of the small helper functions, and also got rid of the need for using the function pointers to the timer helper functions in the structs. The result is that there is less calls in the interrupt code (checked that from the disassembly listing). Also the code is even more clean now. And still can use the generic function for dealing with the schedulers. The good thing is that now calls(previously) are inlined as much as possible, so I consider there is no increased time in the ISR from this PR at the moment. This all for today. Looked with logic analyser, all seems to be operating as expected at least on atmega. Fuel duty cycle limiter behaviour still needs to be adressed and unit tests.

@TBAMax Re-reading my comment from the other day and I realised it probably came across a bit negative. Wasn't my intention as this is a really good PR. Just want to make sure it's not causing any issue when pulled in given it's size and how it's touching on so many critical areas.

@noisymime no problem. I hope I did not ruin the PR with the latest addition of structs with virtual functions. I researched into it and it still seems good solution in my eyes. Specially for the solution against having function calls that can not be inlined. With having virtual functions in the main struct and then overriding them in the derived child struct(class) allows those functions to be inlined, and it is being confirmed by looking at reduced amount of instructions (and icalls) in the dissassembly listing. So functionally and reduced codewise it looks good, but it is essentially not so conservative C coding, but rather C++.

@TBAMax I may have a way to remove the virtual function calls. Is this PR still being actively worked on?

@noisymime Is this PR likely to be accepted?

@TBAMax I may have a way to remove the virtual function calls. Is this PR still being actively worked on?

@adbancroft Bring on the info about alternative ways! Currently this PR is included to the https://github.com/TBAMax/speeduino/tree/mainbranch (with some other stuff) for testing (some lag of maintaining mergeability here). And I have been using it on the car to get the feel how it is behaving. Although some recent changes about the full/semi sequential switching not included there also. It was little hard to understand why some of the scheduler swithing was moved to the init.ino recently, and I need to chew that logic out yet, to bring this up to date.

@TBAMax I'm working on a patch for you. Question: the new fuel scheduler adds a new check when setting the next*Compare members: there has to be 400 timer ticks remaining in the current injection pulse. Why is this needed?

@TBAMax I'm working on a patch for you. Question: the new fuel scheduler adds a new check when setting the next*Compare members: there has to be 400 timer ticks remaining in the current injection pulse. Why is this needed?

@adbancroft This was needed to make 100%duty cycle possible, to cater every possible case. Indeed the hardcoded constant is not nice. Maybe it can be calculated as following: if ((timeout < MAX_TIMER_PERIOD) && ((COMPARE_TYPE)(targetSchedule->endCompare-targetSchedule->getFuelCounter())>(uS_TO_TIMER_COMPARE(INJECTION_REFRESH_TRESHOLD))) need to give it a thought, and then test everything again... When injection pulses overlap (injector always "ON" condition), then fuelchedule status is always RUNNING so all schedule setting in the main loop is actually done via nextSchedule. So the nextschedule update can not also be too close to the interrupt. Maybe it can be done simpler also. I was observing the duty cycle limit. It was previously set capped to 95% because schedulers could not take 100% and would have quite random behaviour at more than 95% (because of fluctuation of 'timePerDegree' causing overlap sometimes). So this was what I came up for now.

Most of the questions I posted above look like they still stand, even after the changes. Some look like they might just be copy/paste type things, but others might be a bit more fundamental.

Just a note here that recently merged changes from #876 probably affect this too, because ignition or fuel timer macros are changed there. So when rewriting this at some future point, need to look at those changes there. Currently I still consider the ignition and fuel schedulers presented here to be superior to those of the current speeduino/master. But it is not possible to maintain such a big change reliably as a mergeable PR, as it is practically maintaining a separate fork. So fresh rewrite starting from the master is needed for this, to avoid accidentally reversing some bugfixes that have been done meanwhile.

@TBAMax Teemo - I agree with this approach to the timers and the need to recode against latest master. What can I do to help?

A different branching strategy could make maintaining this easier: in your separate branch in your repo, rebase from master regularly. This keeps the both commit histories easily visible, rather than bulk merges that contain multiple commits under one "Merge branch 'master' into IgnAndFuel" comment. It may complicate in other ways though.

@TBAMax Teemo - I agree with this approach to the timers and the need to recode against latest master. What can I do to help?

A different branching strategy could make maintaining this easier: in your separate branch in your repo, rebase from master regularly. This keeps the both commit histories easily visible, rather than bulk merges that contain multiple commits under one "Merge branch 'master' into IgnAndFuel" comment. It may complicate in other ways though.

@adbancroft Some points with that was that it breaks the automated (unit)tests for the schedulers. I was uncertain how to deal with that. Either make new tests, or try to modify the code so that it can be squeezed to satisfy the existing testing pattern..., or combined both. Other issues I consider pretty much fixed (irregular pulses beyond the duty cycle limiter, function calls in the ISR inlining).

@TBAMax I fixed the unit tests with the PR I submitted to your main branch: https://github.com/TBAMax/speeduino/pull/11

@TBAMax I fixed the unit tests with the PR I submitted to your main branch: TBAMax#11

@adbancroft Ok, thanks for re linking that in again and drawing attention to it!

Overdwell protection currently under inspection here. Objectives: 1.Gather all information about current status of the overdwell protection and its working mechanisms. 2.If missing pieces or places of possible improvement found, make nessesary changes. 3.Update documentation of the PR about the topic.

Please do look at the other comments I made in the review above as well. These may have been addressed already, but there were a few things there that looked questionable.

The tests are no longer passing. It seems they need to be updated.

The tests are no longer passing. It seems they need to be updated.

Is there a technical reference of how to run those tests?

Is there a technical reference of how to run those tests?

https://docs.platformio.org/en/stable/advanced/unit-testing/index.html

In vscode platformio

Have a look at @adbancroft 's PR. It seems to have a lot of testing stuff in it.

Update

@DeionSi @adbancroft I have now upgraded the tests here , and managed to run them on atmega physically also. I must say this is quite fun! Took the more advanced route and included the crankmath into the test, so the schedules can be tested at various RPMs.

@noisymime, when looking over the checklist now :

• The function calls in the interrupts are minimized. The virtual functions are here to stay. I looked the alternative of using function pointers or references to the setCompare(x) and getTtimer(), this was not good, and resulted in indirect function calls from the interrupt routine vs. with virtual function style as now all is inlined.( avr-objdump -d -S firmware.elf >firmware.lst) So all that is left from the function calls inside the interrupt routine is startFunction() and endfunction() (indirect calls) but nothing to do about those because we need to be able to reconfigure what those point to.
• Dwell limiter is now fixed and reintroduced. Though there is possibility to add corresponding unity test for it in the future. Current present day status of this is that I have not tested it on the real hardware.
• Fuel schedules performance should be resolved with the allowing of the PW up to 100%. So that it do not have any more weirdness at near or even over 100% duty cycle. I remember doing extensive testing on those with the real board and Ardustim, so it should be fine now.

So thats all for today. Thanks for watching! Remember to click "Like" and "Subscribe"!

@TBAMax

The virtual functions are here to stay. I looked the alternative of using function pointers

I ran a quick test and the references/function pointers solution saved 54 bytes and was 2 to 3% faster loop/sec (Ardustim, 60-2 missing tooth, multiple combinations of ignition configurations). I submitted a PR you can test.

Also, I'm seeing some ignition drop outs:

I ran a quick test and the references/function pointers solution saved 54 bytes and was 2 to 3% faster loop/sec (Ardustim, 60-2 missing tooth, multiple combinations of ignition configurations). I submitted a PR you can test.

Ok, the 54 bytes itself is not critical. But 2 to 3% loops/sec? it should be not noticeable. Possible that it comes from the counter and compare used as pointers to register. I have to check that out, seems good optimization! Timer start and timer stop are still funcitons and so make function calls, but those are not actually used regularily when engine is running.

Also, I'm seeing some ignition drop outs:

The drop outs are more serious issue to address. What is the configuration of ignition shown in the picture? It seems coils 1,3,4 firing simultaneously and only 2 is shifted? Drop outs may be from the new dwell limiter...

What is the configuration of ignition shown in the picture?

60-2 wheel, rising edges, wasted spark, old ignition mode, Ardustim at 4K RPM.

60-2 wheel, rising edges, wasted spark, old ignition mode, Ardustim at 4K RPM.

When you turn off the dwell limiter do you see the dropouts dissapear?

Also, I'm seeing some ignition drop outs:

It is a dwell limiter running into the next impulse. This is possible due to he following: This is due to the nested interrupts, the scheduler interrupt happening inside the timer2 interrupt because the timer2 interrupt is with the argument ISR_NOBLOCK So the scheduler interrupt happens right in the middle of the dwell limiter. I originally missed to notice that there is nested interrupts on atmega... When the scheduler interrupt happens right in the middle of dwell limiter, between the limiter comparison and limiter activation(call to the EndFunction), the new pulse gets disabled immediately by the limiter. So kind of a race condition. @adbancroft good that you observed carefully and noticed! So now this is a known issue. Only to figure out now how to resolve this.

I have pushed the fix for the dwell limiter. Feel free to check out!

Oh crap it did not work on stm32. Because cli() and sei() is not applicable there... So new version.

Oh crap it did not work on stm32. Because cli() and sei() is not applicable there... So new version.

The helper functions have their relative substitutes, if it didn't work isn't because of that. Keep in mind ARM cores have 15 levels of interrupt nesting.