[BUG]: SAS turns off without inputs

[PBechon]

Description of the bug

Several users report some cases of SAS turning itself off without any inputs.

Nothing seems to appear in the logs when it happens. It seems to happen only when SimPit is installed and has been reported even when no controller is plugged in. It happens even if the controller does not send any command (meaning it is probably not a command that is badly interpreted).

No instance of SAS turning itself on has been reported.

How to reproduce

Not clear yet

[PBechon]

Thanks to Theresa on Discord, we have a screenshot of it happening. There is nothing in the KSP logs but there is an on screen message appearing 'SAS Disengaged" that is similar to the one triggered by having no EC left. In the screenshot, it is clear there is EC left.

This is not deterministic as the same save reloaded just before the bug does not happen again everytime, but only sometimes.

It happened without any controller plugged.

[PBechon]

Using the save provided by Theresa, I was able to reproduce it once in 8 tries. The message seems to be the same to the one triggered when no EC or no coms are available. Which is not the case the in save provided.

By quickly reviewing the code running when no controller is plugged, nothing seems out of the ordinary.

No link to Simpit code or features is established except for the fact that this seem to only happen when Simpit is installed (even when no controller is plugged).

[rettoph]

I'm 99% sure this is a result of FlightGlobals.ActiveVessel.Autopilot.CanSetMode not being thread safe.

Even without any controllers plugged in, KerbalSimpitAxisController::SASInfoProvider gets invoked every ~Config.RefreshRate thanks to KSPit.EventDispatchThread.

KerbalSimpitAxisController::SASInfoProvider will iterate through all Autopilot modes and prepare an output SASModeInfoStruct message for the SASInfoChannel. In doing so it calls CanSetMode multiple times for every mode, including AutopilotMode.StabilityAssist

Bear with me here...
KerbalSimpitAxisController::SASInfoProvider => VesselAutopilot::CanSetMode => VesselAutopilot.AutopilotMode::AvailableAtLevel has several lines that look a little something like this:

vesselValues.AutipilotSkill.value is calculated like so:

public T value
{
	get
	{
		if (Time.frameCount == this.currentFrame)
		{
			return this.lastValue;
		}
		T t = this.defaultValue;
		int count = this.host.Parts.Count;
		while (count-- > 0)
		{
			Part part = this.host.Parts[count];
			T t2 = this.valueAccessor(part);
			if (this.comparer(t2, t))
			{
				t = t2;
			}
		}

		this.currentFrame = Time.frameCount;
		this.lastValue = t;
		return t;
	}
}

Notice that this.currentFrame = Time.frameCount; is set BEFORE this.lastValue = t;, meaning that if another thread attempts to get the value of t it may get an incorrect value because of the earlier if (Time.frameCount == this.currentFrame)

It just so happens that VesselAutopilot wiithin KSP will check the vesselValues.AutipilotSkill.value each frame and disable SAS if the vessel does not have a pilot with the skill for SAS.

In the event of KerbalSimpitAxisController::SASInfoProvider running in its own thread at nearly exactly the same time as VesselAutopilot::Update I believe it is possible for this race condition to occur.

[rettoph]

Disregard that PR, the solution is not as simple as I thought

[rettoph]

I've pushed 33a0a7e to my personal fork demonstrating a "working" fix for this issue.

I assumed I could simply wrap KerbalSimpitAxisController::SASInfoProvider within UnityMainThreadDispatcher.Instance() and it would just work. Unfortunately as i started pulling that thread more "toSerial-*" providers started throwing issues. KerbalSimpitTelemetryProvider::RotationProvider being the most severe example.

As is, the code "works" again. I find wrapping individual toSerial providers within UnityMainThreadDispatcher kind of silly though, as it seems to contradict the entire intentions behind running these messages on a separate thread.

Unfortunately I don't think we should just start passing the messages along the main update thread, unless we can disregard the EventDispatchThread TimeSlice (TimeSlice = Config.RefreshRate / EventCount;) delay. Obviously such a delay would be unacceptable on the main thread.

Additionally, using the Main Thread limits the refresh rate minimum to the game framerate, which may or may not be a concern.

[rettoph]

It also just occurred to me that passing KerbalSimpitAxisController::SASInfoProvider and KerbalSimpitTelemetryProvider::RotationProvider to UnityMainThreadDispatcher.Instance() kind of disregards the EventDispatchThread TimeSlice delay already, as the actual messages will be broadcasted back-to-back with no delay on the main thread anyway.

Its almost like we need some sort of async Tasks that can be started by our Event Thread, run on the main thread, then pass message content back to the Event thread that can then broadcast the messages to the respective serial ports with the proper TimeSlice delay

I am NOT a Unity dev... is async/await even allowed in Unity?