Roccat Kone AIMO driver #1003
Roccat Kone AIMO driver #1003
Conversation
|
This pull request introduces 10 alerts when merging 2fbb089 into 9fc6d12 - view on LGTM.com new alerts:
|
|
Thanks, I'll give it a look later! Also, please remove the Meson and gitignore changes, this is intended. |
|
This pull request introduces 9 alerts when merging 50a941f into b88be0a - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Thanks for fixing. Also, please don't forget to add the driver to libratbag-data.c, else it won't be detected. See here: #1006 (comment)
|
This pull request introduces 9 alerts when merging 481bb10 into b88be0a - view on LGTM.com new alerts:
|
|
I did with ratbagdctl.devel. I couldn't get piper.devel to work. I'm actually going to test one more time before I'm comfortable this being merged |
What I usually do is kill my system ratbagd daemon if running, and then just run the compiled |
|
This pull request introduces 9 alerts when merging 18ca742 into b88be0a - view on LGTM.com new alerts:
|
|
Ok, I tested it with piper, and was able to remap my scrollwheel successfully. But things I noticed:
Finally, for the space/tab diff thing. VSCode wont show it as anything other than a tab, so I am having trouble fixing it. I considered just running the Button Mapping: https://ocean.yuli.dev/f/4656ff71064d47eda3dd/ |
|
The code looks fine so far, you already mentioned the tab and space mix. Sadly I was not yet able to get it to run, I always get the error "ratbag error: ROCCAT ROCCAT Kone Aimo 16K: driver 'roccat-kone-aimo' does not exist". I checked the installed binary, it does contain the code. At least objdump says it does 😃 I'll try to get it to run and verify on my machine the next evenings. |
You can check if it works with |
|
Well, I don't know why git pull did not pull everything yesterday, but today it did. oO Nevermind, after a quick rebuild the service works now. I'm not shure what I did wrong since this was a fresh clone but it must have been something 😄 I could also verify that piper works with it, it does not show all buttons and while setting a macro to the back key my mouse crashed. I could reset it by unplugging and replugging it. I'll see if I can nail this down tomorrow. This is how it looks on my machine. Obviously the SVG is not existing and the language is german :D But pretty good so far. Thank you :) |
|
Macros are definitely not something I explored too in-depth. I might need to adjust data structs or data sizes for it. The goal for this was button mapping standard buttons and profile settings. This will need a lot more work but it is a starting point for basic support for newer Roccat mice that Roccat tools doesn't support @mweisshaupt1988 if you wish I can send you a quick guide on how I capture the data, or a video. |
|
That would be great, I miserably failed xD |
|
So, Ive been dealing with RL shit for a while so I apologize for the long silence. What are the expectations/goals of this PR? do we want to get just the mouse base functions working first and merge that, or are we waiting on LED support from me? I know I promised some guides but have been too busy to build anything. Just wanting to know what is expected/being waited on/etc. |
|
Would this driver support the Kona Aimo Remastered as well? I have a friend looking to switch (to Linux) and the mouse is the biggest block right now. |
@zefrof yes, I built this driver against the Kone AIMO Remastered, as that is what I have, so it should have 100% support for the Kone AIMO Remastered. |
|
That’s great to hear. Any ideas on when it might get merged? I completely understand if the answer is “don’t know.” |
|
@zefrof ya I don't know, I'm waiting on any feedback from the maintainers. |
|
@daegalus Can you also add support for the Kone AIMO (non-remastered)? We tried installing your branch and changing the inside |
|
@daegalus sorry for the delay, I've been busy. I thought I would have an opening the past month but I didn't. Can you rebase on master? If there are any missing features, like there being no LED support, please set the number of LEDs to 0. |
|
@D3SOX I don't have the mouse. Unless you can provide the dump of the mouse communication for me to look through, I can't make any changes as I don't know the format. @FFY00 will do, give me a little bit, as I switched back to windows on my primary computer. I still have a secondary that I can use to confirm things, I just need to setup the build environment on there again. |
|
@daegalus If you tell me how I can do that I'll provide it |
|
@D3SOX you can use a windows VM with swarm installed, but you need to use wireshark and usbpcap, to monitor the traffic of the USB connection. Once you find the right USB connection. You just need to monitor the packets for the Request and Response packets over usb. the remastered ones were around 190ish kb and 130ishkb, but you will see a pattern. Usually it will be a packet that changes only a little bit with every change you make in the swarm client. Thats the only way I know of doing it. |
|
No worries! I am still limited in time so I am not in a hurry 😁 |
|
This pull request introduces 9 alerts when merging 205643a into c39640d - view on LGTM.com new alerts:
|
|
@staticssleever668 Thank you for the patch, definitely saved me a lot of time getting the branch updated. I am not sure who the current active maintainers are, but I am throwing in the towel on macros. I have been away from the code for too long, and I am just not good enough with C. So, if I can just be advised on how we can proceed to getting this PR merged so someone more experienced with C and/or Ratbag can fill in the last little bit. All the data structures are accurate and commented to the best of my knowledge. I just couldn't figure out how to process 2 separate USB packets/reports as 1 for macros with what I knew. I can also be reached out to whoever picks this up for answering questions. If you would like me to disable Macros, I can add a DISABLE flag, and leave the current code in, but short circuit it on the first line of the function returning Anyways just let me know, and I will make the few changes needed to merge this in without macros working or with macros disabled. All other functionality should be working. |
For me it looks like you are already at the finish line, so let's just make a final push. By the way, conversations opened by amfern still hold true, please, address them. If there is anything you don't understand there, I will help you out. :)
Start out by making diff --git a/src/driver-roccat-kone-aimo.c b/src/driver-roccat-kone-aimo.c
index 97d2fdc8..39aa933d 100644
--- a/src/driver-roccat-kone-aimo.c
+++ b/src/driver-roccat-kone-aimo.c
@@ -146,8 +146,8 @@ struct roccat_macro_page2 {
} __attribute__((packed));
struct roccat_macro_combined {
- struct roccat_macro_page1 *page1;
- struct roccat_macro_page2 *page2;
+ struct roccat_macro_page1 page1;
+ struct roccat_macro_page2 page2;
} __attribute__((packed));
enum roccat_macro_action_type {
@@ -591,31 +591,23 @@ roccat_write_macro(struct ratbag_button *button,
{
struct ratbag_device *device;
struct roccat_macro *macro;
- struct roccat_macro_page1 *macroP1 = malloc(sizeof(struct roccat_macro_page1));
- struct roccat_macro_page2 *macroP2 = malloc(sizeof(struct roccat_macro_page2));
- struct roccat_macro_combined *macroC = malloc(sizeof(struct roccat_macro_combined));
+ // TODO: rename this to `macro_combined` or whatever fits.
+ struct roccat_macro_combined macroC;
struct roccat_data *drv_data;
- uint8_t *buf1;
- uint8_t *buf2;
- uint8_t *buf;
unsigned count = 0;
int rc;
+ // TODO: check of both of these actually make the code clearer,
+ // otherwise simply use `macroC.page1` and `macroC.page2` directly.
+ struct roccat_macro_page1* macroP1 = ¯oC.page1;
+ struct roccat_macro_page2* macroP2 = ¯oC.page2;
+
if (action->type != RATBAG_BUTTON_ACTION_TYPE_MACRO)
return 0;
device = button->profile->device;
drv_data = ratbag_get_drv_data(device);
macro = &drv_data->macros[button->profile->index][button->index];
- macroC->page1 = macroP1;
- macroC->page2 = macroP2;
- buf1 = (uint8_t*)macroP1;
- buf2 = (uint8_t*)macroP2;
- buf = (uint8_t*)macroC;
-
- memset(buf1, 0, ROCCAT_REPORT_SIZE_MACRO);
- memset(buf2, 0, ROCCAT_REPORT_SIZE_MACRO);
- memset(buf, 0, ROCCAT_REPORT_SIZE_MACRO * 2);
for (unsigned int i = 0; i < MAX_MACRO_EVENTS && count < ROCCAT_MAX_MACRO_LENGTH; i++) {
if (action->macro->events[i].type == RATBAG_MACRO_EVENT_INVALID)
@@ -675,22 +667,16 @@ roccat_write_macro(struct ratbag_button *button,
macroP2->profile = button->profile->index;
macroP2->page = 0x02;
- macroP2->checksum = roccat_compute_crc(buf, ROCCAT_REPORT_SIZE_MACRO * 2);
+ macroP2->checksum = roccat_compute_crc((uint8_t*) ¯oC, sizeof(macroC));
macroP2->terminator = 0x4A;
rc = ratbag_hidraw_set_feature_report(device, ROCCAT_REPORT_ID_MACRO,
- buf1, ROCCAT_REPORT_SIZE_MACRO);
+ (uint8_t*)macroP1, ROCCAT_REPORT_SIZE_MACRO);
if (rc < 0) {
- free(macroP1);
- free(macroP2);
- free(macroC);
return rc;
}
if (rc != ROCCAT_REPORT_SIZE_MACRO) {
- free(macroP1);
- free(macroP2);
- free(macroC);
return -EIO;
}
@@ -699,25 +685,16 @@ roccat_write_macro(struct ratbag_button *button,
log_error(device->ratbag,
"Error while waiting for the device to be ready: %s (%d)\n",
strerror(-rc), rc);
- free(macroP1);
- free(macroP2);
- free(macroC);
return rc;
}
rc = ratbag_hidraw_set_feature_report(device, ROCCAT_REPORT_ID_MACRO,
- buf2, ROCCAT_REPORT_SIZE_MACRO);
+ (uint8_t*)macroP2, ROCCAT_REPORT_SIZE_MACRO);
if (rc < 0) {
- free(macroP1);
- free(macroP2);
- free(macroC);
return rc;
}
if (rc != ROCCAT_REPORT_SIZE_MACRO) {
- free(macroP1);
- free(macroP2);
- free(macroC);
return -EIO;
}
@@ -726,15 +703,9 @@ roccat_write_macro(struct ratbag_button *button,
log_error(device->ratbag,
"Error while waiting for the device to be ready: %s (%d)\n",
strerror(-rc), rc);
- free(macroP1);
- free(macroP2);
- free(macroC);
return rc;
}
- free(macroP1);
- free(macroP2);
- free(macroC);
return rc;
} |
|
The |
|
This pull request introduces 9 alerts when merging 26766fc into c39640d - view on LGTM.com new alerts:
|
I believe I took care of that in my latest commit. added the asserts and unions. Fixed up some sizing after a secondary look at the USB packets.
Ok, I will push through. I just committed what I believe should work. I am currently trying to setup my ratbagd and piper to see if it actually works. My concerns currently are in the Also |
There is no magic at all, the data is received a single time on line 542 by a libratbag/src/driver-roccat-kone-aimo.c Line 542 in 26766fc If I understood you right, official software receives two packets separately, if so, I imagine is should be something like this: diff --git a/src/driver-roccat-kone-aimo.c b/src/driver-roccat-kone-aimo.c
index d6480133..4b2bae89 100644
--- a/src/driver-roccat-kone-aimo.c
+++ b/src/driver-roccat-kone-aimo.c
@@ -540,7 +540,16 @@ roccat_read_button(struct ratbag_button *button)
buf[0] = ROCCAT_REPORT_ID_MACRO;
buf[ROCCAT_REPORT_SIZE_MACRO] = ROCCAT_REPORT_ID_MACRO;
rc = ratbag_hidraw_get_feature_report(device, ROCCAT_REPORT_ID_MACRO,
- buf, ROCCAT_REPORT_SIZE_MACRO);
+ (uint8_t*)macro->msg.page1.data, ROCCAT_REPORT_SIZE_MACRO);
+ if (rc != ROCCAT_REPORT_SIZE_MACRO) {
+ log_error(device->ratbag,
+ "Unable to retrieve the macro for button %d of profile %d: %s (%d)\n",
+ button->index, button->profile->index,
+ rc < 0 ? strerror(-rc) : "not read enough", rc);
+ // FIXME: return an error here or handle this somehow.
+ }
+ rc = ratbag_hidraw_get_feature_report(device, ROCCAT_REPORT_ID_MACRO,
+ (uint8_t*)macro->msg.page2.data, ROCCAT_REPORT_SIZE_MACRO);
if (rc != ROCCAT_REPORT_SIZE_MACRO) {
log_error(device->ratbag,
"Unable to retrieve the macro for button %d of profile %d: %s (%d)\n",
I took a quick look, it's indeed quite messy... Your idea sounds good, but I'll take a more in-depth look once I have a bit more time. |
|
So I was making the changes, and I noticed a Roccat device was added a year ago, the Kone EMP. I was looking through it's code, and it has a lot of the same value and settings, it also handles the 2 packet macros. And while the LEDs don't match, and a lot of the extra settings. Those just rely on me updating the structs and LED code. My code was based on the Kone Pure driver, and its very very different. I am going to do a local test on maybe reworking the Kone EMP code to work with the data from the Kone AIMO. Since the Kone EMP was already merged, I already know it passed the review bar. That way, maybe some day they can be merged into the same generic Roccat driver and only the differences need to be handled. |
|
Ok, I ported it to the new code. I made the appropriate changes. Profile, Settings, and LEDs work fine. For some reason though, all the buttons show up as Unknown and I have yet to figure out why. So I can't check it macros work until buttons are parsed properly. This was an issue with the original code too. |
|
This pull request introduces 89 alerts when merging f5d9c37 into 925b2a2 - view on LGTM.com new alerts:
|
|
This pull request introduces 89 alerts when merging 3c28552 into 925b2a2 - view on LGTM.com new alerts:
|
|
Ok, did a small amount of log debugging, and Macros and Buttons are being read correctly, and based on the logging entries i made, they are correctly being converted to and from ratbag actions. Not sure why but if I do |
|
That's weird to say the least. How do you know they are read correctly? |
|
As for writing, the code seems OK, so I don't know what may be the reason. |
I added Ill try to do some more debugging. I am seeing the logs by running
Yes, the dumps are correct. and it even is dumpng out Macro debug info. In my logs it prints out the name, group, and button sequence of the macro. |
|
Could you post the dumps made with official software and with libratbag? Won't hurt to have more eyes comparing them. |
src/driver-roccat-kone-aimo.c
Outdated
| bank_buf[0] = ROCCAT_REPORT_ID_MACRO; | ||
| bank_buf[1] = ROCCAT_BANK_ID_2; | ||
| // The remaining macro structure is not big enough to fill the second bank | ||
| // Write the remaining, fill the end with 0 | ||
| unsigned int remaining_to_write = sizeof(struct roccat_macro)-ROCCAT_REPORT_SIZE_MACRO_BANK; | ||
| memcpy(bank_buf+2, &((uint8_t*)macro)[ROCCAT_REPORT_SIZE_MACRO_BANK], remaining_to_write); | ||
| memset(bank_buf+2+remaining_to_write, 0, ROCCAT_REPORT_SIZE_MACRO_BANK-(2+remaining_to_write)); |
There was a problem hiding this comment.
| bank_buf[0] = ROCCAT_REPORT_ID_MACRO; | |
| bank_buf[1] = ROCCAT_BANK_ID_2; | |
| // The remaining macro structure is not big enough to fill the second bank | |
| // Write the remaining, fill the end with 0 | |
| unsigned int remaining_to_write = sizeof(struct roccat_macro)-ROCCAT_REPORT_SIZE_MACRO_BANK; | |
| memcpy(bank_buf+2, &((uint8_t*)macro)[ROCCAT_REPORT_SIZE_MACRO_BANK], remaining_to_write); | |
| memset(bank_buf+2+remaining_to_write, 0, ROCCAT_REPORT_SIZE_MACRO_BANK-(2+remaining_to_write)); | |
| memset(bank_buf, 0, ROCCAT_REPORT_SIZE_MACRO_BANK); | |
| bank_buf[0] = ROCCAT_REPORT_ID_MACRO; | |
| bank_buf[1] = ROCCAT_BANK_ID_2; | |
| unsigned int remaining_to_write = sizeof(struct roccat_macro)-ROCCAT_REPORT_SIZE_MACRO_BANK; | |
| memcpy(bank_buf+2, &((uint8_t*)macro)[ROCCAT_REPORT_SIZE_MACRO_BANK], remaining_to_write); |
Pointer arithmetic is dangerous and has to be done with great care - let's prevent it when possible.
Also, the sizeof(struct roccat_macro)-ROCCAT_REPORT_SIZE_MACRO_BANK and bank_buf+2 look dodgy, too. I would like to see USB dumps, maybe I could figure something out.
There was a problem hiding this comment.
I may be misunderstanding something here, but if it works, replace &((uint8_t*)macro)[ROCCAT_REPORT_SIZE_MACRO_BANK] with memcpy(bank_buf+2, (uint8_t*)(macro + ROCCAT_REPORT_SIZE_MACRO_BANK), remaining_to_write); - makes the intent a bit clearer.
|
Ok, I dumped everything into the files below. The files without extensions are the hex dumps, they should just be binary files, any binary file editor should work with them. I use ImHex (opensource, crossplatform), which lets me create pattern files that highlight and breakout the data for me to easily read it. All the Finally, the I was thinking that I didn't like all the pointer nonsense also, so I was wondering if maybe I copy over the structs I was using originally, using unions to combine them, and adjust the Macro code. I now realize how easy it is to get both banks. When I was wiresharking the usb packets, both banks got returned every time (though i realize these are writes). Now I can request each individual bank, and get the information as needed, making parsing it super easy using the old structs I had. Let me know if that would be better than this current 1 struct mangling situation. We also don't lose data from the 2nd packet this way.
|
Sounds good. |
It is accessed through Anyways, latest commit I have macros parsing better, and buttons/macros, and other info show up correctly in I have attached my logs of I am also having a tough time getting Macro checksums to work. I can not find a combination of data that equals the expected checksum, its always off by about 130-160 in decimal. Since the checksum is not actual CRC, but just he sum of all the bytes into a uint16. but for example, the macro is expecting |
|
This pull request introduces 90 alerts when merging 3c74e84 into 925b2a2 - view on LGTM.com new alerts:
Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog. |
Deciphered as much as I could figure out and updated the driver as a copy of the current Roccat driver.
Checksum passes, and the data is pulled correctly.
LED support isn't added, but I have the profile struct updated for it. Added comments to the struct for the unknown stuff and for the known stuff.
Added a gitignore for the
builddirandbuildfolders that were created and the one by .vscode, but i can remove that if you don't want it in the codebase.Also updated the version constraint for meson_version as it was spamming me with warning about version incompatibilities with something.