search

keyboardio / Chrysalis-Firmware-Bundle

Firmware sketches for boards supported by Chrysalis
GNU General Public License v2.0
31 stars 25 forks source link

SpaceCadet sometimes ends up being accidentally enabled #28

Closed algernon closed 2 years ago

algernon commented 2 years ago

We've been getting a number of reports lately with misbehaving shifts, which all ended up being caused by SpaceCadet getting enabled unintentionally.

It should default to off. We should investigate why it gets turned on.

algernon commented 2 years ago

I think I know why this happens, but I'm yet to verify it. We have this bit of code:

void SpaceCadetConfig::disableSpaceCadetIfUnconfigured() {
  if (Runtime.storage().isSliceUninitialized(settings_base_, sizeof(SpaceCadet::settings_)))
    ::SpaceCadet.disable();
}

It feels like this doesn't disable SpaceCadet. The function is getting called from the Model100's setup(), so the only way this can fail is if .isSliceUninitialized() returns false. Now why would it return false when we didn't touch any SpaceCadet configuration? Why would it return false only sometimes?

I believe the answer for that is sizeof(). SpaceCadet::settings_ is an uint8_t and an uint16_t. That's 3 bytes, but with padding, it's 4. So we end up checking one extra byte, and if that isn't 255, we will not disable SpaceCadet. Now, what comes after SpaceCadetConfig in the EEPROM layout in the Model 100 firmware? LEDPaletteTheme. If the first color in the palette is set to any color where the red component isn't 0, then that byte will trip the .isSliceUninitialized() setting over.

Unfortunately, SpaceCadet::settings_ does not appear in the size-map, so I'll have to check it some other way. But I'm fairly confident this is the case.

algernon commented 2 years ago

And indeed, sizeof(SpaceCadet::settings_) is 4... however, that is also the size we requested, so unfortunately this is a dead end.

algernon commented 2 years ago

Doing a factory reset and reconnecting with Chrysalis does not turn it on. Perhaps this is a remnant of the old corruption bug? If the eeprom is not properly cleared when upgrading from bad firmware to good, there's a very good chance that SpaceCadet would turn on.

algernon commented 2 years ago

Having looked at a number of cases where SpaceCadet ended up being turned on, I'm very confident that this is not a bug in SpaceCadetConfig, but a result of either corruption, or incomplete / missing storage layout migration, both of which are outside of the control of the plugin.

Closing.