The rest of my changes for G731GW #26
Conversation
Report errors if sending any of the messages fails (although it never seems to). Add verbose printing for the SET and APPLY messages. Add newlines to some errors and verbose logs.
Refactor argument parsing to allow different kinds of scalars (not just speed). Define brightness as a scalar type and expose a feature to set the brightness dynamically. This feature is completely necessary to operate this tool on some systems, where the brightness is reset to zero as soon as GRUB is loaded. Does some other minor tweaking that adds little value but is generally unobjectionable.
Rewrites argument storage to handle colors and scalars with a tagged union. Unifies an argument descriptor to set these apart in function records. Adds default argument values and allows handlers to ignore an inapplicable argument string when a default value is set. Assigns defaults when insufficient arguments are specified. Built-in rainbow effect now takes an optional SPEED argument. single_breathing now sets COLOR2 and SPEED to optional. COLOR2 defaults to COLOR1. SPEED defaults to Medium. Adds a few more color values and allows passing color names instead of hex strings. Allows specifying Low/Medium/High for speed and brightness. Also allows argument-specific constants like slow and fast, or dim and bright.
There was a problem hiding this comment.
This is a really cool patch. I love the natural language arguments and added colour names, that makes things easier to use.
On my hardware, the following functions don't work:
- single_pulsing
- multi_breathing
- rainbow
- brightness
That is, they don't change the keyboard backlight state at all. I'd be really curious to know if there is different behaviour on different hardware. On the other hand, this change seems like a regression for my particular machine. I'm not quite sure at this point what the right way of integrating these changes is, so that all versions of the keyboard are maximally supported. Thoughts?
I've also highlighted a couple of warnings that should be cleaned up as well.
src/rogauracore.c
Outdated
| } | ||
| printf("\n\nCOLOR argument(s) should be given as hex values like ff0000\n"); | ||
| printf("SPEED argument should be given as an integer: 1, 2, or 3\n"); | ||
| printf(" rogauracore ", pDesiredFunc->szName); |
src/rogauracore.c
Outdated
| return -1; | ||
| } | ||
| *pResult = (Speed)nSpeed; | ||
| V(printf("Parsed as %d\n", nScalar)); |
There was a problem hiding this comment.
format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long int’
src/rogauracore.c
Outdated
| for (int i = 0; i < func->nArgs; ++i) { | ||
| const ArgumentDef *arg = &func->args[i]; | ||
| printf(arg->defaultValue.type ? " [" : " "); | ||
| printf(arg->name); |
There was a problem hiding this comment.
format not a string literal and no format arguments
|
Good catches! I got sloppy with printf because I didn't realize I didn't have -Wall enabled for this build. Never build without it, myself. Anyway, I'm way more worried about these hardware differences. I was afraid this would be the case... it looked as though you'd have found those other three message formats if they existed on your hardware. On my hardware, the multi_ methods do not work, which includes master's implementation of rainbow. It would be nice if we could query for a version string to tell the two protocols apart. This weekend I will poke around with Armory Crate and try to figure out how it identifies the keyboard. Do you have brightness adjustment on your keyboard at all, or does your firmware just control that by passing around dimmer colors (like 808080 instead of FFFFFF)? Interestingly, on my hardware, passing any of the mode changing methods (even just changing the color from blue to red) causes a half-second blank, during which the keyboard is not illuminated at all. But changing the brightness happens smoothly. I can dial it up or down at will. Another interesting different: you send SET and then APPLY for each change. My ASUS software passes SET, APPLY, SET. ...it makes no difference, even on my machine, and I have no idea why it's happening. As for multi_breathing... I am completely lost as to how I broke that. It's clear to me that the pulsing functionality and firmware rainbow are new additions (or I guess possibly old deletions)... but I am not sure why multi_breathing itself broke. Rainbow no longer calls it, but I thought it would still work. The code also seems correct.... It would be difficult for me to debug that because none of the multi_ functions work on my version, before or after this change. Maybe short-term, we can have a rainbow and an asus_rainbow (or firmware_rainbow or something) until we can figure out what changes between our versions. |
|
If you have a dual boot and some kind of Asus software you can sniff, we might stand half a chance of finding out how to get a version number or how to query the hardware capabilities. I'm single-boot Ubuntu, so to make rogauracore, I put the Asus Aura Core on a Win10 inside a virtualbox and recorded it with wireshark. I got traces of all the functions the software showed me, but clearly different machines might have different hardware capabilities, and so what I was able to reverse engineer is not going to be all there is to know about this family of RGB controllers. However, between not having a MS system on, and only having one kind of hardware to test, I've gotten used to thinking of myself as not the right person to solve this. That is also my excuse for the message ordering; I believe that I just copied down what I was seeing in the captures, which was the RGB value message, followed by the SET message. Initially, I didn't have the APPLY message at all, but I added it after I noticed that the keyboard state would reset on reboot or restore from suspend. I think the Aura Core software maybe only sent APPLY when the program exited, though I'm not sure about this anymore. My gut feeling is that the exact number of SET and APPLY messages isn't really important. On my system, the keyboard LED brightness seems to be handled by asus-wmi. To be honest, I don't know if WMI controls the keyboard via USB or in some other way. Brightness changes using Fn+Up/Fn+Down are smooth. For me using rogauracore, the keyboard colour state updates instantly with no noticeable delay, but there is clearly a microcontroller getting reset somewhere (you can see this in the way the "programs" like single_colorcycle "start over" after the update). Maybe other microcontrollers take a bit longer to reset. On this patch, I'll take a closer look at multi_breathing. The rainbow function is nothing special, it's a 4-zone static colour scheme which happens to be how my keyboard acted when I bought the machine. Not having Windows, I had that same look for 2 years until I wrote rogauracore, so I wrote in the rainbow function to be able to "put things back" to the way they were. In the 9 months since I've written rogauracore, my keyboard has always been on single_colorcycle slow, so I guess I don't really need the rainbow ;) |
|
You were the best person for the job because you were the one to care enough to write this. Had you not, I wouldn't have had a starting point and probably wouldn't have bothered. I booted windows, activated Wireshark, and installed ArmouryCrate. When it launched, there was a good amount of back and forth as it initialized the keyboard. I notice that it started off by sending 5d, then repeated itself with 5e. The keyboard acknowledged both times, repeating the input message. I suspect this was a handshake / version check. Literally, just sending the string I need to look over asus-wmi more. It appears to be different from the messages my device uses. It doesn't really recognize my device messages, at all. It throws away the message payloads that contain the interesting data about, eg, what button I'm pressing. It can't interpret my brightness buttons at all. It understands the multimedia keys, but none of the other special keys on my keyboard. It seems they intended to support this, though... Eventually, we might want to integrate with that driver, or else team up with ckb-next team... (although Corsair runs thick in their codebase). For now, RGB keyboards seem to be lost on that driver file, and it can't handle my keyboard at all. Not sure if we'll have any luck convincing them to support more advanced features. Their brightness function is utterly wrong for my device, best I can tell, but for all I know, it'd work if it understood the brightness keys on my keyboard at all. As for this patch, maybe the rainbow isn't the most practical effect, but it'd be nice if it worked on all hardware versions. I wouldn't check this change in and take that away from people with a different firmware version. Want to just revert rainbow and add a rainbow_asus for my firmware? |
|
Thank you! this rainbow functionality works for GU502GU |
|
Thanks, this works in G531GT but the capitalization in hotPink and deepPink broke the program. You can solve it by changing to hotpink and deeppink |
|
Good catch. I've pushed a fix for the underlying problem—it should be fine to capitalize letters in those strings, I just wrote a bad case comparison function (should have just grabbed |
|
multi_breathing returns segmentation fault on manjaro linux. Any fix for this? |
|
Well, at least all commands working on g712lw, but... If hardware split is needed - seems like dmidecode is our friend |
|
This also got rainbow working properly on the GU502G here, just letting you know! |
This is a lot of changes which you may or may not be interested in, but I figured I'd send them your way.
I redid argument representation. Both colors and scalars are now stored as a tagged union.
There's an analog struct for parameter notation, which allows functions to specifically describe what arguments they accept. Using this mechanism, I added support for optional arguments.
Functionally...
rainbowfunction now uses the built-in rainbow effect and takes an optional SPEED argument. It defaults to Fast.single_breathingfunction now setsCOLOR2andSPEEDto optional.COLOR2defaults toCOLOR1.SPEEDdefaults to Medium.Redinstead ofFF0000).