The libwacom database assigns LEDs into groups that are each associated
with a ring or touchstrip control. The number of LEDs defines the number
of modes that the control makes available. The total number of StatusLEDs
is thus just the sum of the number of modes on each control. Note that
some devices have controls with multiple modes but no LEDs associated, so
simply counting *all* modes doesn't work; you must only count those modes
on controls returned by libwacom_get_status_leds().
Details
Details
Diff Detail
Diff Detail
- Repository
- R530 Wacom tablet support
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
I have a couple of nitpicks, but otherwise it should be good, thanks!
I assume this is also not going to work very well with Wacom Cintiq 24HD, but it's mostly fault of how StatusLEDs property is treated right now (see procsystemadaptor.cpp, ProcSystemAdaptor::setProperty if you're curious). We assume that LED banks always have 4 LEDs because we use old LED API that exports 0-2 4 LED banks. However this code will reports six leds, because apparently 1 led in each bank on Cintiq 24HD is invisible, and instead of firing up 4th LED (1st LED in 2nd bank) we will fire up the "missing" 4th LED in 1st bank. This is just an assumption, I can't test this theory because I don't have the device.
But this seems to be the only device affected by this and good way to fix would require reviewing whole StatusLEDs property, so the patch should be good enough.
If you don't have commit access and need landing this for you, say so, I'll just address the nitpicks myself.
| src/common/libwacomwrapper.cpp | ||
|---|---|---|
| 91 | Initialize to zero just in case | |
| 95 | Add explicit do-nothing case in the end for WACOM_STATUS_LED_UNAVAILABLE to suppress a warning and so we'll catch when new led groups will be added | |
| 111 | I think it's worth putting a TODO or FIXME comment somewhere that it needs checking with Cintiq 24HD (see my general comment) | |
Comment Actions
Thanks for the review! Those all look like sensible suggestions. I don't believe I have commit access, so I'd need you to land the set for me.
With specific regard to the Cintiq 24HD, I have one on my desk so I could take a closer look at its operation under KDE. The LEDs on the 24 actually work a bit differently than any other tablet in that rather than using a single "mode switch" button to cycle through the list, each of the 6 LEDs is built in to a button. I haven't played around too much with this KDE code, but it would need to have the ability to set specific LEDs in response to button presses rather than simply cycling. If it has (or gains) such an ability, the two "missing" LEDs shouldn't be a problem since they won't be mapped to any physical button.