Trigger System Redesign

It's all about the code!
Post Reply
mck1117
running engine in first post
running engine in first post
Posts: 241
Joined: Mon Jan 30, 2017 2:05 am
Location: Seattle-ish
Soldering skill: yes
Coding skill?: yes

Trigger System Redesign

Post by mck1117 » Sat Oct 27, 2018 11:54 pm

There have been a few discussions lately about how Rusefi's trigger system runs on a significant amount of magic. This is an attempt to reduce the amount of magic.

Difficulties with the existing trigger system:
  1. Cam vs. crank decoding: All trigger types use angles in the range [0, 720), even if they're only decoding the crankshaft. Via some magic these are effectively compressed down to [0, 360), then repeated. If you're on a 2 stroke it's compressed but not repeated, and if it's a true cam sensor system none of this happens and the true values are passed.
  2. The trigger system relies on determining the index of each tooth, then planning out future events (fuel, ign, etc) based on index. These events are natively angles (not trigger teeth!), so a conversion from angle -> tooth -> angle is happening. Listeners to the trigger system don't care what kind of tooth we just saw, or what index it is in the sequence, but only where we are in the engine's cycle, and how long we have until we should expect to see another tooth.
  3. The 'zero point' of a trigger is not well defined. It is determined by the trigger system at startup, and as a result requires running the wheel synchronization algorithm to determine what the #1 TDC offset should be. This means that the angles provided during trigger setup effectively mean nothing, and are merely relative offsets, not absolute as is implied.
  4. It's not clear if more complicated sync patterns are currently possible. The GM 24x pattern has no "special" tooth for synchronization. There's one point in the pattern where remembering the length of the past 4 teeth will resolve position uniquely, or remembering the past 5 teeth can resolve any position uniquely. Adding support for this case to the current trigger system would require either a) a completely different trigger system that gets selected based on whether you're using a gm24x, or b) hack logic to sync more complex trigger shapes that has to run when any trigger shape is decoded, which may have significant performance cost. All triggers currently pay the performance cost of the most complicated trigger.
Proposed design:
  1. Instead of passing edge type and index to ShaftPositionListeners, pass:
    1. The current sync state (synced, not synced)
    2. The current engine phase as of the event we just decoded. (angle_t)
    3. The expected arrival of the next trigger edge. (angle_t)
    4. The current engine speed, RPM.
    We don't need to know what kind of edge it was or the full shape of the trigger wheel to schedule events, so that information should not be leaked out of the trigger system.
  2. Decouple cam and crank logic. Four stroke engines require a cam decoder (I know what your question is, hang on a sec), and two stroke require a crank decoder. By 'cam decoder' I don't mean something that actually requires reading a camshaft, but instead something that resolves angles to the range [0, 720).

    The simplest cam decoder could just consume a crank decoder, then every other revolution of the crankshaft optionally add 360 degrees. This lets a crankshaft decoder have the sole job of decoding a [0, 360) range. For native cam-only setups (for example Nissan/DSM CAS sensors), a cam-only decoder could provide native [0, 720) angles without the use of a crank decoder.
  3. Allow a cam sensor to consume an existing crank sensor. On the physical engine, the cam sensor and crank sensor are physically separate (if you have both). One can be replaced without the other. As such, the implementation of the trigger system should reflect this: If I add a cam sensor to my engine, I should be able to use the same crank decoder (and settings), but add the capability decode the cam as well.

    I implemented this model using inheritance and templates. A base class defines the interface for feeding edges to a trigger, and receiving results back (or ignored edges). Two classes inherit the base: two bases for cam and crank decoders. Classes for trigger types that inherit TriggerDecoder only have to implement two methods: One to determine if a particular edge should be decoded, and one to decode an edge, and return the current position of the engine.

    If defining a new cam sensor that is an add-on (ie, requires a crank sensor), inherit from CamshaftWheelAdder<TCrank>. All the class inheriting from CamshaftWheelAdder has to do is a) interpret cam signals b) use those to determine engine phase ("A" side or "B" side of a four-stroke) then c) figure out whether or not to add 360 to the result of the crank decoder's output.

    What does this actually look like? Here's the inheritance diagram generated by doxygen:
    Image

    If you wanted to run my Volvo, I have a 60-2 missing tooth wheel, and no cam sensor. So the decoder would be initialized like this:

    Code: Select all

    CamlessDecoder<MissingToothTriggerDecoder> decoder(60, 2);
    This could probably also be implememented without templates, which may improve runtime configurability, but that loses the complier optimization opportunities of templates.
I'm happy to hear suggestions before I go implement too much more of this :D

mck1117
running engine in first post
running engine in first post
Posts: 241
Joined: Mon Jan 30, 2017 2:05 am
Location: Seattle-ish
Soldering skill: yes
Coding skill?: yes

Re: Trigger System Redesign

Post by mck1117 » Sun Oct 28, 2018 12:50 am

Here's a compare of progress so far: https://github.com/rusefi/rusefi/compar ... r?expand=1

Start reading at TriggerDecoder.h.

And before things like std::optional cause panic, don't worry, they optimize away to pretty much nothing ;)

User avatar
russian
Site Admin
Posts: 9674
Joined: Wed Aug 28, 2013 1:28 am
Location: Jersey City
Soldering skill: yes
Coding skill?: yes
Contact:

Re: Trigger System Redesign

Post by russian » Sun Oct 28, 2018 9:29 pm

mck1117 wrote:
Sat Oct 27, 2018 11:54 pm
[*] It's not clear if more complicated sync patterns are currently possible. The GM 24x pattern has no "special" tooth for synchronization. There's one point in the pattern where remembering the length of the past 4 teeth will resolve position uniquely, or remembering the past 5 teeth can resolve any position uniquely.
I've merged your PR https://github.com/rusefi/rusefi/pull/614 and to my surprise unit tests are passing and new trigger image is generated.

While yes, existing implementation is not trivial by any means - so far it is supporting any trigger wheel we are aware of, and it also works. I know it would sound awful - but at the moment I have zero motivation to change it :( I would much rather document existing implementation better or refactor it to make it more readable if you have any specific questions or proposals.

Obviously feel free to propose a complete rewrite of this layer but it would have to pass existing unit and integration testing.

Again, I did not intend to sound hostile. I have very limited to play with rusEfi so I am trying to identify the most cost effective changes I can make at any given point. Trigger decoder just does not seem to be broken to me at the moment.
Attachments
trigger_27.png
trigger_27.png (6.83 KiB) Viewed 883 times
https://rusefi.com/s/howtocontribute
very limited telepathic abilities - please post logs & tunes where appropriate - http://rusefi.com/s/questions
my skype is arro239

alexander-n8hgeg5e
Posts: 50
Joined: Mon Oct 01, 2018 4:42 pm
Soldering skill: yes
Coding skill?: yes

Re: Trigger System Redesign

Post by alexander-n8hgeg5e » Wed Nov 21, 2018 7:37 pm

As I understand the sugessted cam decoder is a decoder that can decode 720 degree? , however it does that.
I suggest that it should be named 720-decoder , so it can not be confused with a real on.
I like to think about these things in detail, what should get passed and how should it be named and so on.
If the 720 decoder uses some crank sensor only, it should refuse it and say " i'm a 720, need more input, use 360 instead."

User avatar
mobyfab
Posts: 137
Joined: Tue Oct 29, 2013 10:09 am
Location: Versailles, France
Soldering skill: yes
Coding skill?: yes

Re: Trigger System Redesign

Post by mobyfab » Tue Dec 25, 2018 1:45 am

Excellent implementation :)

Russian, it's possible to integrate this while keeping the old system and letting user choose whatever they prefer at compile time.
Then you can slowly migrate/deprecate all trigger patterns to the new system, to finally remove the older one.

User avatar
russian
Site Admin
Posts: 9674
Joined: Wed Aug 28, 2013 1:28 am
Location: Jersey City
Soldering skill: yes
Coding skill?: yes
Contact:

Re: Trigger System Redesign

Post by russian » Tue Dec 25, 2018 2:44 am

mobyfab wrote:
Tue Dec 25, 2018 1:45 am
Russian, it's possible to integrate this while keeping the old system
Short answer is NO.

No, it's not possible to have old complete solution and new less complete solution. Right now as we speak a similar proposal takes place in another subsystem. So there is a chance to have (old + 0.5 new) in one subsystem and (old + 0.5 new) in another one. Shall we take this route and eventually have (old + 0.5 new) repeated too many times? That would be really challenging.

I am a strong believer in small incremental changes - you can see my style with how small my average commit it and how often I am making a lot of these small commits. I would prefer a similar approach for any redesign - I would prefer to isolate old code behind some clear API, get things covered by tests, and replace old implementation for new implementation in one change which would not be touching tests but just swapping old for new.
https://rusefi.com/s/howtocontribute
very limited telepathic abilities - please post logs & tunes where appropriate - http://rusefi.com/s/questions
my skype is arro239

User avatar
russian
Site Admin
Posts: 9674
Joined: Wed Aug 28, 2013 1:28 am
Location: Jersey City
Soldering skill: yes
Coding skill?: yes
Contact:

Re: Trigger System Redesign

Post by russian » Tue Dec 25, 2018 2:59 am

mck1117 wrote:
Sat Oct 27, 2018 11:54 pm
Difficulties with the existing trigger system:
...
Proposed design:
I agree with most of the difficulties and mot of the proposals make sense, but I am not comfortable with the approach of trying to redesign the system right away. I believe that it should be possible to execute the migration in a series of smaller changes.

Would it be possible to identify couple of areas which could be improved independently and start by improving those areas?

It could be that a good start would be to better isolate trigger decoding system and give it a cleaner and more clear API. So, any takers for a smaller useful PR?
https://rusefi.com/s/howtocontribute
very limited telepathic abilities - please post logs & tunes where appropriate - http://rusefi.com/s/questions
my skype is arro239

User avatar
russian
Site Admin
Posts: 9674
Joined: Wed Aug 28, 2013 1:28 am
Location: Jersey City
Soldering skill: yes
Coding skill?: yes
Contact:

Re: Trigger System Redesign

Post by russian » Tue Dec 25, 2018 3:06 am

mck1117 wrote:
Sat Oct 27, 2018 11:54 pm
Cam vs. crank decoding: All trigger types use angles in the range [0, 720), even if they're only decoding the crankshaft. Via some magic these are effectively compressed down to [0, 360), then repeated. If you're on a 2 stroke it's compressed but not repeated, and if it's a true cam sensor system none of this happens and the true values are passed.
This could be a good place to start. Let's identify triggers which should not be defined using 720 range and refactor the code so that these ones are defined using 360 range on the shape definition layer.
https://rusefi.com/s/howtocontribute
very limited telepathic abilities - please post logs & tunes where appropriate - http://rusefi.com/s/questions
my skype is arro239

User avatar
russian
Site Admin
Posts: 9674
Joined: Wed Aug 28, 2013 1:28 am
Location: Jersey City
Soldering skill: yes
Coding skill?: yes
Contact:

Re: Trigger System Redesign

Post by russian » Tue Dec 25, 2018 4:19 am

https://github.com/rusefi/rusefi/issues/635 suggest first refactoring step - let's decouple TriggerShape from engine

https://github.com/rusefi/rusefi/commit ... 09db72098c actually shows how to do this exactly - 'initializeMazdaMiataNaShape' does not depend on 'DECLARE_ENGINE_PARAMETER_SUFFIX' any more and all triggers should be redone in a similar manner once 'useOnlyRisingEdgeForTrigger' logic is placed into 'calculateExpectedEventCounts' method and that should not be that hard.
https://rusefi.com/s/howtocontribute
very limited telepathic abilities - please post logs & tunes where appropriate - http://rusefi.com/s/questions
my skype is arro239

mck1117
running engine in first post
running engine in first post
Posts: 241
Joined: Mon Jan 30, 2017 2:05 am
Location: Seattle-ish
Soldering skill: yes
Coding skill?: yes

Re: Trigger System Redesign

Post by mck1117 » Tue Dec 25, 2018 8:39 am

russian wrote:
Tue Dec 25, 2018 2:44 am
mobyfab wrote:
Tue Dec 25, 2018 1:45 am
Russian, it's possible to integrate this while keeping the old system
Short answer is NO.

No, it's not possible to have old complete solution and new less complete solution.
That's absolutely possible-wrap the old system inside an instance of the new system. As systems get touched, cut them over to the new implementation. When no references to the old exist, it can be safely removed.

alexander-n8hgeg5e
Posts: 50
Joined: Mon Oct 01, 2018 4:42 pm
Soldering skill: yes
Coding skill?: yes

Re: Trigger System Redesign

Post by alexander-n8hgeg5e » Tue Dec 25, 2018 7:04 pm

russian wrote:
Tue Dec 25, 2018 4:19 am
https://github.com/rusefi/rusefi/issues/635 suggest first refactoring step - let's decouple TriggerShape from engine

https://github.com/rusefi/rusefi/commit ... 09db72098c actually shows how to do this exactly - 'initializeMazdaMiataNaShape' does not depend on 'DECLARE_ENGINE_PARAMETER_SUFFIX' any more and all triggers should be redone in a similar manner once 'useOnlyRisingEdgeForTrigger' logic is placed into 'calculateExpectedEventCounts' method and that should not be that hard.
code looks nice

Post Reply