Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger: "Single 60deg tooth before TDC" for Saruman, Sovek ignition #5347

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

@mck1117
Copy link
Contributor

mck1117 commented Jun 21, 2023

why not just use the single tooth decoder?

@AvatarSD
Copy link
Contributor Author

AvatarSD commented Jun 21, 2023

This variation created with 'SyncEdge::Both" option to be able to become synchronized by one 60 degree tooth gap. Single tooth give me some warnings on synchronization, because it expect 50% duty. Seems my scope approve this trigger workability. Please, check if my approach resonable.

imageup.ru
Here: Vin(😁) - inverted sensor input, Vbat - inverted ignition output

@AvatarSD
Copy link
Contributor Author

I'm revert SyncEdge::Both to SyncEdge::Rise after several practical tests. With this settings i have stable sync with less than 360deg of cranck.
imageup.ru
Note: signals on scope inverted.

@rusefillc
Copy link
Contributor

With SyncEdge::Rise this really looks like existing single tooth trigger just with settings hard-coded?

While it was ::Both this PR was adding something previously impossible. With Rise this is too custom of a use-case to have custom code I believe

@AvatarSD
Copy link
Contributor Author

With a single tooth I have WARNING: tooth #1 error of -118.8 and worse sync-up, and I have no idea how to fix it with common features. That's why I made this PR. Seems, have a better tooth constructor in MS will be widely usefull, but it's natural for it to grow.

With ::Both I have another issue. My main idea was to sync crank by 60 deg before TDC and be ready immediatly after. It might only take 60 degrees to sync and propery fire first full cycle withous entire previous one. ::Both led me to unexpencted behaviour when crank starts rotation from wrong side of blind, as shown in the diagrams below.

When this situation began(blue trigger start from low on graphs), the trigger made a wrong decision about it's own position and point of sync. What do you think about this cause?

As result my current PR gives me best solution, and it handle unique situation. My suggestion is to leave it for now, and than, save this variant till the time, when ::both will work properly along with new field of tooth width in degree trough MS communication will be implement. Seems this variant will be widely usefull for old motors.

imageup.ru
Note: yellow - fuel.
imageup.ru

@rusefillc
Copy link
Contributor

But TT_ONE initializeSkippedToothTrigger(this, 1, 0, triggerOperationMode, SyncEdge::Rise); is supposed have same exactly logic as this PR. At the moment I am failing to comprehend why would TT_ONE and RISE version of this PR behave any differently on any data?

Please understand where I am coming from: my priority is on understanding root causes of issues. Technically the proper way to prove different behavior would be to have a unit test showing different behavior.

@AvatarSD
Copy link
Contributor Author

AvatarSD commented Jun 22, 2023

The root cause of why TT_ONE behave diffently is because it setup events as 180 and 360 degrees.

initializeSkippedToothTrigger() internally calls addSkippedToothTriggerEvents(TriggerWheel::T_PRIMARY, s, totalTeethCount, skippedCount, 0.5, 0, getEngineCycle(operationMode), NO_LEFT_FILTER, NO_RIGHT_FILTER); with toothWidth = 0.5 which creates two events by 180 and 360 deg. Compared to my 300 and 360 degrees respectively.

SyncEdge::Rise as I understand it trying to align phase on both edges which produce phase error with non-symmetric blind which have different length of open and closed sections. In this cause there no way to setup configuration proper. Changing duty from 83.%(300/360) to 50% for my configuration and vise versa for TT_ONE lead to lack of sync in a different ways.

There are two ways to solve: rewrite TT_ONE to use SyncEdge::RiseOnly or create separate case. I think toching something fundamental is bad, thats why creating another trigger seems a good idea. As I mention above have a something like TT_ONE_PHASED trigger with externally setuped blind width will be a good approach to utilize ::Rise(and::Both in future) with both edges sync, but to stay simple I suggest just to use another enum for each trigger type.

My approach was to utilize ::Both especially for single tooth and manual kikstarter, to be ready on both sides of blind. But unfortuneally, my experiments show me the Trigger can't become syncronized by 'last' and folowed 'first' events only. Also I observe phase-aligment mehanism is trying to consume a longer side of trigger as latest before TDC.

I wish to setup SyncEdge::Both for my TT_60DEG_TOOTH after this case of scenario become work well. For now, ::Rise work well for my 60 degree trigger and both edges phase sync work as mush as expected for my engine startup.

@rusefillc
Copy link
Contributor

@AvatarSD can you please some summary of that great comment ^^^ into the PR as code comment and I will merge?

AvatarSD pushed a commit to AvatarSD/rusefi that referenced this pull request Jun 23, 2023
@rusefillc rusefillc merged commit 087a1c2 into rusefi:master Jun 23, 2023
57 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants