Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect host OS based on USB fingerprint #18463

Merged
merged 7 commits into from
Dec 8, 2022
Merged

Detect host OS based on USB fingerprint #18463

merged 7 commits into from
Dec 8, 2022

Conversation

KapJI
Copy link
Contributor

@KapJI KapJI commented Sep 23, 2022

Description

Feature to detect host OS using wLength field from getDescriptor USB packets.

Context: https://www.reddit.com/r/olkb/comments/x1ezbg/way_to_detect_host_os_in_qmk/

I'd appreciate if someone can write better documentation, I added only some very basic description.

Types of Changes

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

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).

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few areas that this won't compile, and I've flagged those.

tmk_core/protocol/chibios/usb_main.c Outdated Show resolved Hide resolved
quantum/os_detection.c Show resolved Hide resolved
quantum/os_detection.c Outdated Show resolved Hide resolved
quantum/os_detection.c Outdated Show resolved Hide resolved
quantum/os_detection.c Outdated Show resolved Hide resolved
quantum/os_detection.c Outdated Show resolved Hide resolved
quantum/os_detection.c Outdated Show resolved Hide resolved
quantum/os_detection.c Outdated Show resolved Hide resolved
docs/feature_os_detection.md Outdated Show resolved Hide resolved
docs/feature_os_detection.md Outdated Show resolved Hide resolved
quantum/os_detection.h Outdated Show resolved Hide resolved
quantum/os_detection/tests/os_detection.cpp Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team September 24, 2022 19:10
@drashna
Copy link
Member

drashna commented Sep 24, 2022

Should be noted that this is originally from keyboardio's arduino module:
https://github.com/keyboardio/FingerprintUSBHost

It's licensed under the MIT License and should probably retain that licensing.

Also, should be noted that there hasn't been any changes to the code since Aug 2018.

Also tagging @algernon.

@algernon
Copy link
Contributor

We haven't done much with the FingerprintUSBHost module in a looong time. As far as I remember, it was more of a proof of concept, rather than a fully fleshed out solution. It's mostly @obra's work, by the way. If there are any licensing questions, he's the right person to ask.

I'm happy to chime in and answer what I can, but my memories of that module are kinda hazy by now.

@obra
Copy link

obra commented Sep 24, 2022 via email

@drashna
Copy link
Member

drashna commented Sep 25, 2022

Thanks for the feedback! And yeah, the detection is very ... fragile, at best.

@KapJI
Copy link
Contributor Author

KapJI commented Sep 25, 2022

Please note that I have used none of the FingerprintUSBHost code. Only the general idea of using wLength field. All detection heuristics are brand new.

I believe it should be reliable enough to cover most real life cases. I have tried using KVM switch but haven't tried passing device to virtualized host. With more collected data it should be easy to support more less popular cases, I added debugging section specifically for this.

@drashna drashna requested a review from a team September 30, 2022 17:58
Co-authored-by: Drashna Jaelre <drashna@live.com>
@drashna drashna requested a review from a team September 30, 2022 20:01
@tzarc tzarc merged commit 85ee55f into qmk:develop Dec 8, 2022
@KapJI KapJI deleted the guess-os branch December 8, 2022 16:52
@PeterHindes
Copy link
Contributor

Btw this is mis-recognizing an M1 Max studio as an iOS device (only tested on Ventura).

@tzarc
Copy link
Member

tzarc commented Jan 12, 2023

Btw this is mis-recognizing an M1 Max studio as an iOS device (only tested on Ventura).

Thus the initial reluctance including it -- it's fragile.
If you want to do some more digging and extend it further, great -- a PR would be lovely.

@KapJI
Copy link
Contributor Author

KapJI commented Jan 12, 2023

I guess it's impossible to distinguish ARM Macs from iOS/iPadOS devices as they use the same USB stack. So OS_IOS is better called something like OS_APPLE_ARM and OS_MACOS is OS_APPLE_INTEL.

@tzarc
Copy link
Member

tzarc commented Jan 12, 2023

It could be OS version specific, too.
As documented it's "best guess", nothing more.

@PeterHindes
Copy link
Contributor

I will check with my Ventura Intel MacBook. I might make a pr if I have time.

omikronik pushed a commit to omikronik/qmk_firmware that referenced this pull request Jan 22, 2023
Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Nick Brassel <nick@tzarc.org>
@mreel

This comment was marked as outdated.

@fauxpark
Copy link
Member

It's already been merged.

abstracterror pushed a commit to abstracterror/qmk_firmware that referenced this pull request May 27, 2023
Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Nick Brassel <nick@tzarc.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants