#2838 heading_indicator.cxx precession / offset not stored, so not applied

[Benedikt Hallinger]

Hi, while investigating https://github.com/HHS81/c182s/issues/525 I probably found a bug in heading_indicator.cxx .

Look at this:
https://sourceforge.net/p/flightgear/flightgear/ci/next/tree/src/Instrumentation/heading_indicator.cxx
Line 96 to 98. The offset is read from the node, then calculatoins are done and the resulting offset from this frame is applied. The offset property is not written, so with next call of update() again the initial offset is read, applying another single very! small) offset to the final heading.

The expected behaviour would be to write the resulting offset to the property, so update() can read the correct value in the next run:

Fix:
In line 98 add:

_offset_node->setDoubleValue(offset);

Consequences for Aircraft devs
If the heading indicator model rotates the offset-knob via the offset-deg property, it will slowly rotate with precession too.
While this is not noticeable in realtime, it can in accelerated time.
Proper fix for this is easy, just tie the rotation of the knob to another

[Benedikt Hallinger]

Actually just tested this.
With just the fix, the precession works, but is also adjusted when there is no spin.

so wen need an additional conditional to check for the spin:

--- a/src/Instrumentation/heading_indicator.cxx
+++ b/src/Instrumentation/heading_indicator.cxx
@@ -94,8 +94,11 @@ HeadingIndicator::update (double dt)

                                 // Next, calculate time-based precession
     double offset = _offset_node->getDoubleValue();

-    offset -= dt * (0.25 / 60.0); // 360deg/day
-    SG_NORMALIZE_RANGE(offset, -360.0, 360.0);
+    if (spin >= 0.2) {
+        offset -= dt * (0.25 / 60.0); // 360deg/day
+        SG_NORMALIZE_RANGE(offset, -360.0, 360.0);
+        _offset_node->setDoubleValue(offset);
+    }

                                 // TODO: movement-induced error

[James Turner]

Yep these seem like good fixes, the 'knob will spin' is an issue but as you say the rate is going to be slow, so probably worth living with.

Code comments: prefer SGMiscd::normalizePeriodic for new code, since it's a real function and not a macro.

[Benedikt Hallinger]

Code comments: prefer SGMiscd::normalizePeriodic for new code, since it's a real function and not a macro.

I didn't touch that, its a copy.
But I assume its safe to just replace this yourself when you merge?

[James Turner]

Sure, but this is why copy-and-paste is the root of all evil :)

[Benedikt Hallinger]

How do we proceed here, do you need something from me?
(just asking in case you are waiting for me to do something :) )

If I'm right, the change to the patch would be like this:

--- a/src/Instrumentation/heading_indicator.cxx
+++ b/src/Instrumentation/heading_indicator.cxx
@@ -94,8 +94,11 @@ HeadingIndicator::update (double dt)

                                 // Next, calculate time-based precession
     double offset = _offset_node->getDoubleValue();

-    offset -= dt * (0.25 / 60.0); // 360deg/day
-    SG_NORMALIZE_RANGE(offset, -360.0, 360.0);
+    if (spin >= 0.2) {
+        offset -= dt * (0.25 / 60.0); // 360deg/day
+        offset = SGMiscd::normalizePeriodic(-360.0, 360.0, offset);
+        _offset_node->setDoubleValue(offset);
+    }

                                 // TODO: movement-induced error

Or must it read: offset = SGMiscd::normalizePeriodic(-180.0, 180.0, offset); like in heading_indicator_dg.cxx ?

[James Turner]

Re-reading this: are you it's not intended to be a fixed offset, from the node?

The 'time-based precession' offset seems like a different factor to me, which was combined into 'offset' maybe incorrectly.

I ,e it should be:

offset = offsetNode->getDoubleValue();
if (spin > 0.2) {
secondsSpining += dt;
timePrecssionOffset = secondsSpining * (0.25 / 60);
} else {
secondsSpinning = 0.0; // reset
}

heading = heading + offset - timePrecessionOffset

This woudl avoid the write back to offsetNode which is kind of a smell.

[James Turner]

Note for this, 'secondsSpinning' needs to be a member variable so it persistes between update() calls.

[Benedikt Hallinger]

That could very well be.
The heading_indicator_dg.cxx has an additional "align" property, which is used to adjust the correction. However that would break backward compatibility for existing aircraft, so its probably better to keep it simple here and put it back to "offset".

[Benedikt Hallinger]

Hello James,
just checked tickets/2828 commit dea806d7adc45aca5b9c87eb618203c05c1742a0

Works like intended!
Thank you!

[Benedikt Hallinger]

Okay, made an attempt on this: https://sourceforge.net/p/flightgear/flightgear/ci/dea806d7adc45aca5b9c87eb618203c05c1742a0/
(Either cherry-pick the patch or pull the branch to test)

Another thing: the calculated precession error is not stored to the prop tree.
What would be good:

• Initialize the precession error from the node in case the aircraft had provided a stored value
• write the error to said property, so the aircraft can store it for the next session.

Currently the error is just 0 at sim start. I'd love to have that error accumulating between sessions, when the aircraft saves the prop :)

[Benedikt Hallinger]

So, I think I found another bug in the simple heading_indicator.cxx.
The precession is not calculated correctly.

My research led to a working and good solution in the heading_indicator_dg.cxx; see the other ticket: https://sourceforge.net/p/flightgear/codetickets/2839/#5728
I would suggest that you port that over here.

[James Turner]

I'll port the precession fix over from the heading_indiciate_dg.cxx changes when I merge those, yep.

[xDraconian]

@jmturner, per your last comment, I assume this ticket can close. Please confirm this was addressed during the remediation of ticket #2839

[Gijs]

@benih can you confirm whether this was committed?

[Benedikt Hallinger]

No, I think it is not yet.
However, as much work has been done in the new instrument, I think we
should leave this one here alone for backward compat reasons.

Applying it has the side effect of rotating the knob slowly without the
aircraft dev making adjustments.
So i would say, keep the instrument as is, mark it as "legacy" or
"simple" or so, and hint aircraft devs to use the more advanced HI
instead.

[Gijs]

Thanks! Closing this issue then. Could you add some instructions for aircraft maintainers on how to switch to the new instrument to https://wiki.flightgear.org/Aircraft_maintenance#Instruments?

[Benedikt Hallinger]

Thanks! Closing this issue then. Could you add some instructions for
aircraft maintainers on how to switch to the new instrument to
https://wiki.flightgear.org/Aircraft_maintenance#Instruments?

Sure!