Simpit-team / KerbalSimpitRevamped

A revamp of the Kerbal Simpit Mod for KSP
BSD 2-Clause "Simplified" License
26 stars 15 forks source link

[BUG]: CAG not behaving consistently when empty #82

Open PBechon opened 2 years ago

PBechon commented 2 years ago

Tested on latest version of Simpit, KSP1.12 w/ AGExt. Reported by @CodapopKSP

Using the logging, you can see the difference in behavior between AG1-10 and AG11+.

If you put a craft on the pad and a craft on the runway, both with nothing assigned to action groups, and then run that code and have it activate the CAG in the setup, then switch between the two, you'll see a few things:

Here is a script to reproduce it

#include "KerbalSimpit.h"

KerbalSimpit mySimpit(Serial);
unsigned long lastDebounceTime_w = 0;

bool Action_Dspl_b[10] = {};
bool Action2_Dspl_b[10] = {};

void setup() {
  Serial.begin(115200);
  while (!mySimpit.init()) {delay(100);}
  mySimpit.printToKSP("Connected", PRINT_TO_SCREEN);

  mySimpit.inboundHandler(messageHandler);
  mySimpit.registerChannel(CAGSTATUS_MESSAGE);

  delay(1000);

  mySimpit.toggleCAG(2);
  mySimpit.toggleCAG(3);
  mySimpit.toggleCAG(12);
  mySimpit.toggleCAG(13);
}

void loop() {
  mySimpit.update();

  if ((millis() - lastDebounceTime_w) > 1000) {
    for (int i = 0; i < 4; i++) {
      mySimpit.printToKSP("AG" + String(i) + ": " + String(Action_Dspl_b[i]), PRINT_TO_SCREEN);
      //mySimpit.printToKSP("AG2" + String(i+10) + ": " + String(Action2_Dspl_b[i]), PRINT_TO_SCREEN);
    }
    lastDebounceTime_w = millis();
  }
}

void messageHandler(byte messageType, byte msg[], byte msgSize) {
  switch(messageType) {
    case CAGSTATUS_MESSAGE:
      if (msgSize == sizeof(cagStatusMessage)) {
        cagStatusMessage myAG;
        myAG = parseCAGStatusMessage(msg);
        for (int i = 1; i < 11; i++) {
          Action_Dspl_b[i] = (myAG.is_action_activated(i));
          Action2_Dspl_b[i] = (myAG.is_action_activated(i+10));
        }
      }
      break;
  }
}
PBechon commented 2 years ago

Additional information after some digging. As we use different logic if AGExt is installed of not, we need to consider both cases

If AGExt is not installed

We call directly KSP API. In this case the controller can change the CAG state even when the group is empty, as with the keyboard. Switching vessels seems to work properly (i.e. if you have an empty CAG, it can be switched on an it stays on when you switch to another vessel and you switch back).

So I think there is no issue if AGExt is not installed.

If AGExt is installed

For stock CAG (1-10)

In this case for consistency, we use AGext API for all CAG, the stock on (1-10) and the 11+. AGExt seems to disregard activating empty CAG. This means that the controller cannot activate any CAG (even the 1-10 that we can activate via keyboard, since we are using a different API).

There is also an issue (reported by CaptnUndrpnts) that in some case the toggling of actions is not working when the action group was not set at launch but is modified during flight. I had the same issue with an action group being populated via the stock GUI that was not reflected in the AGExt GUI. When the CAG is populated with the AGExt GUI, I did not find any issue. I believe AGExt is not keeping in sync the content of each CAG with the content of the stock CAG ...

For non stock CAG (11+)

Some different things happened, as mentioned by Coda above.

AGExt allow to activate empty CAG (tested with 11), and the value of the CAG is even shared between several vessels (behavior not consistent with stock CAG). Meaning that if craft A and B have an empty CAG 11, it can be toggled on any craft and when switching craft, the value is kept.

If the CAG is populated on both craft, switching from one to the other works as expected.

Way forward

The strange behavior seems (to me) to come from AGExt and I'm not sure we can do anything about it on Simpit side, unfortunately ...

I suggest the following :