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

detect and ignore doubled trigger edges #4656

Merged
merged 7 commits into from
Jan 10, 2023

Conversation

mck1117
Copy link
Contributor

@mck1117 mck1117 commented Oct 11, 2022

Detect when we get a "doubled edge" trigger input.

Will enable the second check (very wrong tooth timing) later, as it causes problems at the moment.

#987 #2222 #4650

@mck1117
Copy link
Contributor Author

mck1117 commented Oct 13, 2022

this looks also OK, proteus CI still sad 🙁

@mck1117 mck1117 marked this pull request as ready for review October 13, 2022 18:38
@rusefillc
Copy link
Contributor

What do we perceive to be root cause here? Is it expected to get double edges or it is some specific hardware issue?

Do we want to changelog this improvement?

@rusefillc
Copy link
Contributor

@ElDominio tell me about 4b11 is it Hall or VR?

@ElDominio
Copy link
Collaborator

Hall

@rusefillc
Copy link
Contributor

@mck1117 please blink if alive

@mck1117
Copy link
Contributor Author

mck1117 commented Nov 1, 2022

I'm alive! Was racing this weekend and getting ready before that.

@rusefillc
Copy link
Contributor

What do we perceive to be root cause here? Is it expected to get double edges or it is some specific hardware issue?

Do we want to changelog this improvement?

@rusefillc
Copy link
Contributor

@mck1117 is this abandoned?

@ElDominio
Copy link
Collaborator

My only question with this is how do some other ECU companies trigger properly on both edges (no crazy sync loss)

@mck1117
Copy link
Contributor Author

mck1117 commented Dec 1, 2022

My only question with this is how do some other ECU companies trigger properly on both edges (no crazy sync loss)

sort of unrelated discussed offline re: #4635

@mck1117
Copy link
Contributor Author

mck1117 commented Jan 10, 2023

What do we perceive to be root cause here? Is it expected to get double edges or it is some specific hardware issue?

It is a case we can detect cheaply, so why not detect it? Perhaps it will be a useful metric to detect issues we know about but haven't narrowed down to a root cause.

@rusefillc
Copy link
Contributor

What do we perceive to be root cause here? Is it expected to get double edges or it is some specific hardware issue?

It is a case we can detect cheaply, so why not detect it? Perhaps it will be a useful metric to detect issues we know about but haven't narrowed down to a root cause.

I like this justification! My preference would be to have this filtering enabled by default but have an option to turn it off, what do you think? I am fine adding the option later myself.

@mck1117
Copy link
Contributor Author

mck1117 commented Jan 10, 2023

I like this justification! My preference would be to have this filtering enabled by default but have an option to turn it off, what do you think? I am fine adding the option later myself.

Right now it only filters truly doubled edges (no trigger should have 1/2 degree tooth spacing), but sure

@rusefillc rusefillc merged commit eb56c28 into rusefi:master Jan 10, 2023
rusefillc pushed a commit that referenced this pull request Jan 10, 2023
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