xanderfrangos / twinkle-tray

Easily manage the brightness of your monitors in Windows from the system tray
https://twinkletray.com
MIT License
5.3k stars 170 forks source link

Handle Apple Studio Display Brightness #748

Closed jridgewell closed 3 weeks ago

jridgewell commented 5 months ago

This uses studio-display-control to communicate with an Apple Studio Display via usb. I basically just ported over https://github.com/juliuszint/asdbctl into a node package.

studio-display

I'm kinda abusing the serial number to uniquely identify the display. I don't have any DDC/CI displays, so I'm not really sure what the key and id values are supposed to be in the normal paths. I also don't know how to support renaming the display, but I'm happy to update if you can tell me how.

Fixes #705 Fixes #713 Fixes #489

xanderfrangos commented 5 months ago

This is great, thank you! I'll review this in more detail over the weekend, but at a glace it's a solid start.

Ideally, we would want to match the USB connection with the display ID in Windows (e.g. \\?\DISPLAY#GSM7707#5&6646027&1&UID4355). I don't have a Studio Display to test with, so I'll need some help figuring out how to identify them.

jridgewell commented 5 months ago

Ideally, we would want to match the USB connection with the display ID in Windows (e.g. \\?\DISPLAY#GSM7707#5&6646027&1&UID4355)

Can you explain what the individual values are in that string? I'm not sure what they would correspond to in the USB protocol. Or why the code sometimes uses key and sometimes id?

It seems id can be a string or number in a few code paths, but I only ever see it set to a string. key appears to be the last segment of the display id. I think both are meant to uniquely identify the display. There are a few things that need hwid (from which id is constructed) in order to send ddc messages. I think id and key might be redundant? All cases could be served by the serial number.

jridgewell commented 5 months ago
'5&2930019f&1&UID4357': {
  id: '\\\\?\\DISPLAY#APPAE3A#5&2930019f&1&UID4357',
  key: '5&2930019f&1&UID4357',
  num: 2,
  brightness: 50,
  brightnessMax: 100,
  brightnessRaw: 50,
  type: 'none',
  connector: 'displayport_external',
  min: 0,
  max: 100,
  hwid: [
    '\\\\?\\DISPLAY',
    'APPAE3A',
    '5&2930019f&1&UID4357',
    '{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}'
  ],
  name: 'StudioDisplay',
  serial: '2034519119',
  features: {
    luminance: false,
    brightness: false,
    contrast: false,
    powerState: false,
    volume: false
  },
  brightnessType: 0,
  brightnessValues: [ 50, 100 ]
},
'00008030-001E41402628802E': {
  id: '00008030-001E41402628802E',
  key: '00008030-001E41402628802E',
  num: 3,
  brightness: 100,
  brightnessMax: 100,
  brightnessRaw: 50,
  type: 'studio-display',
  connector: 'unknown',
  min: 0,
  max: 100,
  hwid: [],
  name: 'Apple Studio Display',
  serial: '00008030-001E41402628802E'
}

I dug through WMI and the USB interface trying to associate them. But the monitor's information doesn't match the USB interfaces information, so there doesn't seem to be a way. I could just guess and assign monitors that have the name: StudioDisplay and hwid[1]: APPAE3A values to a USB Studio Display (I'll need to overwrite the serial number so I know which one to talk to to get/set brightness). But that might get tricky if the user happens to have multiple studio displays. What happens if the lists return in a different order?

I think it'd be safer to stick with the serial number as key/id/serial approach.

xanderfrangos commented 5 months ago

Took me a little longer than expected to dig into this, but I think this is good enough to merge in. I'll make some additions/adjustments when I work it into v1.16.0. Apple Studio Display users won't get access to all of the Twinkle Tray features, but that's better than not having it work at all.

Thanks again for the PR! I'll tag you in the v1.16.0 beta issue when I have a build ready, if you don't mind testing.

svyatogor commented 2 months ago

I was excited to see Apple studio display controls being added. I tried the latest beta (7 at this time) and while the display shows in the menu and displays correct brightness the slider is unresponsive.

Let me know if I can help to further debug the issue.

Here are some logs:

 M  Monitors.js starting. If you see this again, something bad happened!
 M  Skipping DDC/CI test...
 M  getMonitorsWMIC() Total: 59ms
 M  getMonitorsWin32() Total: 0ms
 M  getStudioDisplay() Total: 64ms
 M  Error reading VCP code 0x10 for \\?\DISPLAY#APPAE3A#5&15499a66&0&UID260. Reason: Error: Failed to get VCP code value
An error occurred because the field length of a DDC/CI message contained an invalid value.

 M  Error reading VCP code 0x13 for \\?\DISPLAY#APPAE3A#5&15499a66&0&UID260. Reason: Error: Failed to get VCP code value
An error occurred because the field length of a DDC/CI message contained an invalid value.

 M  Error reading VCP code 0x6B for \\?\DISPLAY#APPAE3A#5&15499a66&0&UID260. Reason: Error: Failed to get VCP code value
An error occurred because the field length of a DDC/CI message contained an invalid value.

 M  Error reading VCP code 0x12 for \\?\DISPLAY#APPAE3A#5&15499a66&0&UID260. Reason: Error: Failed to get VCP code value
An error occurred because the field length of a DDC/CI message contained an invalid value.

 M  getFeaturesDDC() Total: 780ms
 M  getBrightnessWMIC() Total: 54ms
 M  getAllMonitors() total: 960ms
 M  Monitors found: 5&15499a66&0&UID260,00008030-000329320A33802E
 M  getBrightnessWMIC() Total: 53ms
 M  getStudioDisplay() Total: 14ms
jridgewell commented 2 months ago

I'll try to take a look this weekend.

jridgewell commented 1 month ago

Found the problem with the betas, https://github.com/xanderfrangos/twinkle-tray/pull/792 resolves it.

xanderfrangos commented 1 month ago

The fixes from @jridgewell will be in the next update, but in the meantime you can use this build: https://github.com/xanderfrangos/twinkle-tray/actions/runs/10336256927/artifacts/1824936257