[Core] Add OS detection callbacks

[andrebrait]

Description

This adds callbacks for OS detection, similar to other mechanisms we already have to allow the KB/user to run custom code on an event.

The main use is withholding taking action until the OS detection has run, so the user does not have to craft some sort of periodic checks for the current OS, etc. As an example (and my usage), to set the current default layer depending on whether the detected OS is Windows, macOS, etc.

This also eliminates any questions on whether OS_UNSURE means the detection ran and it resulted in that, or if it just happened to be the default value.

Since the detection runs when the USB device descriptor is being assembled, I added the option to also automatically reboot the keyboard when the USB link goes back to a non-configured state, due to e.g. KVM switching when the KVM does not cut power to the USB devices. This will trigger the initialization again and thus allows the detection to run again for the new host device. This is disabled by default and can be enabled by defining OS_DETECTION_KEYBOARD_RESET.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[andrebrait]

Ah, it's missing one change I made yesterday. Gonna update it, but the general picture is unchanged.

[andrebrait]

@sigprof ping

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[andrebrait]

@drashna is there anyone else who can take a look at this PR? There is already at least one other user who's tested this on other boards in #22288 and @sigprof doesn't seem to have the time to do it.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[andrebrait]

@drashna any chance you can help this PR leave limbo? 😜

[andrebrait]

Or is it expected to be stale until the breaking change merges start?

[pfn]

@pfn did you try rebasing this on top of develop? I can do it right now.

Yes, I tried it with it rebased on latest develop

commit 141429b803380b01967c793392019844edfd302c (os_detection)
Merge: 5cd31fda9c 63e165aa8e
Author: Perry Nguyen <pfnguyen@hanhuy.com>
Date:   Wed Jan 31 13:10:48 2024 -0800

    Merge remote-tracking branch 'remotes/andrebrait/improvement/os_detection' into os_detection

commit 5cd31fda9c1302044f4423d940be7c888d15ad1a (origin/riot, origin/develop, develop)
Author: Joel Challis <git@zvecr.com>
Date:   Sun Jan 28 22:51:58 2024 +0000

    Begin removal of bootmagic lite terminology (#22970)

[andrebrait]

@pfn got it. The weirdest part is that nothing should have changed there. All I did was making setups_data and usb_setups static and detected_os static and volatile.

Could you undo that, recompile and try again?

[pfn]

Could you undo that, recompile and try again?

I'll give it a try tomorrow or Friday

[pfn]

I just fetched the latest from the branch onto develop. Still getting OS_UNSURE on Sonoma. Rebooting the keyboard while attached to the Mac still results in OS_UNSURE.

Attached is an image of hid console from qmk toolbox and it ignores copy/paste... (running qmk console on windows crashes my keyboard).

[andrebrait]

I just fetched the latest from the branch onto develop. Still getting OS_UNSURE on Sonoma. Rebooting the keyboard while attached to the Mac still results in OS_UNSURE.

Attached is an image of hid console from qmk toolbox and it ignores copy/paste... (running qmk console on windows crashes my keyboard).

Ok. There's definitely something weird going on here, because the very messages on macOS are corrupted.

Is the screenshot from Windows ialso with the code from this PR?

[pfn]

That screenshot on Windows is the negotiation dump from OS detection debugging after switching to mac saving the packet, switching back to Windows and dumping. Resetting the keyboard while on mac produces the same packet. I didn't print out the contents of Windows negotiation in the screenshot. Yes, all dumps are with code from current PR.

All I did was making setups_data and usb_setups static and detected_os static and volatile.
Could you undo that, recompile and try again?

I haven't tried this yet, will try again tomorrow.

[andrebrait]

That screenshot on Windows is the negotiation dump from OS detection debugging after switching to mac saving the packet, switching back to Windows and dumping. Resetting the keyboard while on mac produces the same packet. I didn't print out the contents of Windows negotiation in the screenshot. Yes, all dumps are with code from current PR.

All I did was making setups_data and usb_setups static and detected_os static and volatile.
Could you undo that, recompile and try again?

I haven't tried this yet, will try again tomorrow.

Never mind. It's an actual bug. Got it.

Could you please replace line 133 from os_detection.c with:

if (guessed != OS_UNSURE) detected_os = guessed;

and then try it?

I think current develop code is actually detecting OS_IOS when packet number 4 comes in, and then the others simply result in it not detecting anything else. My code unintentionally diverges in that it may revert back to OS_UNSURE if a future packet arrives that makes it not make sense. In this case, it was the fact that the length is 6 instead of 5, because of the 0x101 packet.

EDIT: yeah, you actually said yourself that it detected OS_IOS. My bad for not paying attention.

That likely caused it to revert back to OS_UNSURE.

For backwards compatibility sake, I'll revert this behavior and I'll add your packets to the unit testing suit so we do not regress this way again.

Do you have an actual iOS device to test as well? I'd need to check if it's safe to change the condition for OS_MACOS from count == 5 to count >= 5, since this isn't iOS.

[pfn]

Never mind. It's an actual bug. Got it.

Could you please replace line 133 from os_detection.c with:

if (guessed != OS_UNSURE) detected_os = guessed;

and then try it?

Yes! This change has my keyboard detecting my Mac is OS_IOS again now after switching. Makes sense why this happens.

I think current develop code is actually detecting OS_IOS when packet number 4 comes in, and then the others simply result in it not detecting anything else. My code unintentionally diverges in that it may revert back to OS_UNSURE if a future packet arrives that makes it not make sense. In this case, it was the fact that the length is 6 instead of 5, because of the 0x101 packet.

EDIT: yeah, you actually said yourself that it detected OS_IOS. My bad for not paying attention.

Same behavior on master, detecting as OS_IOS (just mentioning).

Do you have an actual iOS device to test as well? I'd need to check if it's safe to change the condition for OS_MACOS from count == 5 to count >= 5, since this isn't iOS.

I do not have an iOS device to test.

[andrebrait]

@pfn thanks for testing it, providing debug data (extremely valuable!) and confirming the fix. It's really appreciated.

I'll push it alongside with a test case later today or tomorrow.

[pfn]

I've tested it as much as I could on my own keyboard. Keymap update is to follow. As far as I can tell, everything works correctly, except when switching machines with the KVM because the USB descriptor data isn't reprocessed, and that's currently when the OS detection runs (need to figure that out).

Thanks, works great with everything up-to-date.

Re the description:

I've tested it as much as I could on my own keyboard. Keymap update is to follow. As far as I can tell, everything works correctly, except when switching machines with the KVM because the USB descriptor data isn't reprocessed, and that's currently when the OS detection runs (need to figure that out).

Is it worth updating the PR description with regards to KVM switching? It works for my cheap USB switcher.

[andrebrait]

@pfn yeah, done it. I also removed the part where I said tests were pending, since the PR has contained tests for a while now.

It seems fixing the issue you had also fixed the weird instability I very rarely found under macOS.

[aadcg]

Amazing feature, thanks!

A simple way to set the behaviour of CTRL and GUI across OSs.

#ifdef OS_DETECTION_ENABLE
bool process_detected_host_os_kb(os_variant_t detected_os) {
  if (!process_detected_host_os_user(detected_os)) {
    return false;
  }
  switch (detected_os) {
  case OS_MACOS:
    keymap_config.swap_lctl_lgui = keymap_config.swap_rctl_rgui = true;
    break;
  case OS_IOS:
    keymap_config.swap_lctl_lgui = keymap_config.swap_rctl_rgui = true;
    break;
  case OS_WINDOWS:
    keymap_config.swap_lctl_lgui = keymap_config.swap_rctl_rgui = false;
    break;
  case OS_LINUX:
    keymap_config.swap_lctl_lgui = keymap_config.swap_rctl_rgui = false;
    break;
  case OS_UNSURE:
    keymap_config.swap_lctl_lgui = keymap_config.swap_rctl_rgui = false;
    break;
  }
  return true;
}
#endif

[andrebrait]

@aadcg I didn't even know this exists. Gonna check it out later.