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

AP_EFI: Hirth: fix sensor health flags #28075

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

robertlong13
Copy link
Collaborator

@robertlong13 robertlong13 commented Sep 11, 2024

There is no crankshaft sensor status reported by this EFI. This line is misleading and should be removed. The sensor health bitmask is already logged elsewhere.

There is no crankshaft sensor status reported by this EFI. This line is
misleading and should be removed. The sensor health bitmask is already
logged elsewhere.
@peterbarker
Copy link
Contributor

How did we end up with this in here, then?

Is it in the datasheet?

@robertlong13
Copy link
Collaborator Author

robertlong13 commented Sep 11, 2024

How did we end up with this in here, then?

I really don't know how this got there. I asked around but didn't get a confident answer.

Is it in the datasheet?

No, that sensor_ok bitmask holds 4 bits for 4 different sensor healths, none of which are crankshaft, and all 4 of which are logged a couple lines later correctly. From the bitwise & here, you'd think that CRANK_SHAFT_SENSOR_OK held a single bit, but it doesn't: its 0x0F (4 LSB set). So this line is effectively marking crankshaft sensor healthy if any of these 4 sensors are healthy, and unhealthy if all are unhealthy.

@robertlong13 robertlong13 changed the title AP_EFI: Hirth: remove crankshaft sensor status AP_EFI: Hirth: fix sensor health flags Sep 12, 2024
@robertlong13
Copy link
Collaborator Author

While I was here, I noticed that our other sensor health bitmask is actually wrong. The datasheet shows bits 1-4, not bits 0-3, and I have confirmed with my Hirth EFI that bit 0 always reports 0.

I have pushed another commit to this PR to address that too.

@peterbarker peterbarker merged commit 72a0139 into ArduPilot:master Sep 13, 2024
95 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants