Regarding the CI failures:

• a big part of the black failure is due to renaming krakenx3; this shouldn't be an issue once you revert it, but should you need to know how to manage the ignored/keep-old-style list, it's in pyproject.toml;

• the test failures appear to be because the new dependency is missing; both the workflow file and the README should be adjusted to indicate that this new dependency has to be manually installed when setting up a local test environment.

Thank you for the detailed feedback and the diff comments

So a small architectural change I think is necessary is to have a second "raw device" (e.g. self.hid_device) that's a HidapiDevice (like normally with UsbHidDriver) , and not a PyUsbDevice (which is what self.device is when inheriting UsbDriver).

I switched to using a second hid device and can confirm the new way works on macos

I don't see this, after all, hwmon was not yet implemented for the KrakenZ3: its implementation of get_status() would unconditionally did raw I/O.

yes and there were checks in the initialize function for hwmon that applied to both drivers and is now removed

That will be important. Do you perhaps have a fan you can use just to test this?

my ram sticks makes the space next to wire ports very tight so I removed the fan and nzxt wires completely. I will check on discord if someone is able to test the fan and color control for the rgb fans. if not, I will try to do it by next weekend

update, I tried changing fan speed and the fan duty does seem to change however I am still unable to physically test it yet

Also, is the pump control working?

pump control is working

For the time being the old *_not_totally_broken test should be easily adaptable, and it would at least guarantee that the calls don't unexpectedly throw with good input.

sounds good I will add it

I switched to using a second hid device and can confirm the new way works on macos

I'm glad to hear that, as I wouldn't be able to test it myself.

yes and there were checks in the initialize function for hwmon that applied to both drivers and is now removed

Oh, I forgot about those.

I see why it makes sense to remove that code, but I think we need to replace it with a warning in case a hwmon kernel driver is found to be bound to the device.

if self._hwmon:
    _LOGGER.warning("%s is bound to the %s kernel driver,", self.description,
            self._hwmon.module)
    _LOGGER.warning("and this liquidctl version may interfere with it; to fix the kernel")
    _LOGGER.warning("state at any point, unload and reload the %s module",
            self._hwmon.module)
    _LOGGER.warning("please report this to liquidctl")

Otherwise we could significantly mess with its expectations, without alerting the user, or, potentially, even finding out that we need to implement support for that hwmon driver.

I see why it makes sense to remove that code, but I think we need to replace it with a warning in case a hwmon kernel driver is found to be bound to the device.

the base UsbDriver does not have a self._hwmon, so i am not sure it would be expected. If needed I can replicate what the initializer for UsbHidDriver is doing

        self._hwmon = HwmonDevice.from_hidraw(device.path)
        if self._hwmon:
            _LOGGER.debug('has kernel driver: %s (%s)', self._hwmon.module, self._hwmon.path)

the base UsbDriver does not have a self._hwmon, so i am not sure it would be expected. If needed I can replicate what the initializer for UsbHidDriver is doing

self._hwmon = HwmonDevice.from_hidraw(device.path)

That should work as long as device is the HID device.

I'll need a couple more days to really review the latest changes. By the way, did you see my hidapi and serial number questions?

In the meantime, one conceptually small (but practically large and annoying) architectural change I kinda want to suggest is to use the HID interface as the main device in both drivers. This would make the overall patch much smaller, and, as we tend to prefer HID over direct USB access, it would also keep these drivers more similar to other ones in the repository.

I imagine you initially went with PyUsbDevice (via UsbDriver) because you planned on using both interfaces through it. Is there still another reason, given the need to use both backends simultaneously (to support Mac OS)?

I'll need a couple more days to really review the latest changes. By the way, did you see my hidapi and serial number questions?

I think i responded to the serial number question in the other comment but here it is again "I had a the black model that I exchanged for the white one and both had different serial numbers. this my current serial number for the white z53: 354F336B3131"

In the meantime, one conceptually small (but practically large and annoying) architectural change I kinda want to suggest is to use the HID interface as the main device in both drivers. This would make the overall patch much smaller, and, as we tend to prefer HID over direct USB access, it would also keep these drivers more similar to other ones in the repository.

ok done, tests are now passing as well

an important note is that on windows, this only works when using this branch of libusb: https://github.com/Tpimp/libusb/tree/win-composite-child-support. otherwise I get a NotImplementedError('Operation not supported or unimplemented on this platform')

I think i responded to the serial number question in the other comment

I couldn't find it even searching for the serial number string. Maybe it go lost or is in a limbo? These past few weeks I've noticed some weird issues with GitHub comments. For instance, I just lost this comment when I was nearly done with it (something which used to be nearly impossible to happen).

Anyway... thanks for pasting it again.

"I had a the black model that I exchanged for the white one and both had different serial numbers. this my current serial number for the white z53: 354F336B3131"

That's a relief.

You should bring back the check for it in initialize, when setting the secondary "bulk" PyUsbDevice.

tests are now passing as well

I think that's only because you have one of these devices.

For CI and the rest of us, you'll need to mock a PyUsbDevice and/or monkeypatch the PyUsbDevice enumeration. For a somewhat similar problem and solution, you can take a look at the test_backward_compatibility_11 module.

an important note is that on windows, this only works when using this branch of libusb: https://github.com/Tpimp/libusb/tree/win-composite-child-support. otherwise I get a NotImplementedError('Operation not supported or unimplemented on this platform')

Given that there's probably still quite a bit for that to be merged (the work on which it's based on is still in review), we'll need to support falling back to a simpler status & cooling only mode.

What's the call stack of that exception? Can we detect in a convenient place, and disable the screen functionality then (at runtime)?

Also, can we differentiate between the user has not yet replaced the Windows driver with Zadig (I mean, I'm assuming that's necessary on the bulk interface... is this correct?), and they have but aren't running this experimental patch to LibUSB?

Given that there's probably still quite a bit for that to be merged (the work on which it's based on is still in review), we'll need to support falling back to a simpler status & cooling only mode.

What's the call stack of that exception? Can we detect in a convenient place, and disable the screen functionality then (at runtime)?

the error only occurs in the _bulk_write method so I think it should be fine as other functions would work as usual

Also, can we differentiate between the user has not yet replaced the Windows driver with Zadig (I mean, I'm assuming that's necessary on the bulk interface... is this correct?), and they have but aren't running this experimental patch to LibUSB?

with the experimental libusb patch there is no need to replace any drivers.

If liquidctl bundles its own copies of libusb cant it just include the version with the patches?

@ShadyNawara,

Shouldn't this

WARNING: Failed to switch active bucket

be an error (instead of a warning)?

I think it should be yes. although, it doesnt really tell us why it failed to switch buckets whether there is a file error, a memory misalignment or something else

@ShadyNawara,

Does the response from

response = self._write_then_read([0x38, 0x1, mode, bucketIndex])

contain more information? If so, you could ask the user to enable --debug (if you can't reproduce the issue even with the provided failing GIF).

Additionally, could dumping (more accurately: logging) self._query_buckets() before exiting help in this scenario?

@ShadyNawara
If I upload gifs after a couple of times I also get this:
WARNING: Failed to switch active bucket
I was debugging a little bit and noticed that the bucket headers are not always the same. Look at this example of the first bytes of the first 4 buckets:

0: [55, 3, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 1, 0, 0, 0, 0, 0, 0, ...
1: [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 0, 1, 2, 0, 0, 84, 2, 1, 1, 0, 0, 0, ...
2: [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 1, 0, 0, 0, 0, 0, 0, 0, 0,  ...
3: [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 2, 0, 0, 0, ... 

Bucket 0 starts with the first byte set to "55" instead of "49". In this case Byte 15 and 16 also do not represent the "bucket index" and "asset index". I modified the _find_next_unoccupied_bucket function to also check for the fist byte being "49":

    def _find_next_unoccupied_bucket(self, buckets):
        """
        finds the first available unoccupied bucket
        buckets are unoccupied when bytes 14 onward are 0x0
        returns -1 if unoccupied buckets are found
        """
        for bucketIndex, bucketInfo in buckets.items():
            if not any(bucketInfo[15:]):
                if (bucketInfo[0] == 49):
                    return bucketIndex
        return -1

With this I can upload more often (using a bucket starting with 55 leads to the "failed to switch active bucket"-error) however after a while still some memory corruption happens and the error shows up again. I will still be debugging and trying to find maybe more - but implementing a partially reversed engineered protocol is really like poking around in the dark. I pretty confident that the problem is somewhere in that whole bucket memory management code.

Thank you so much for developing on this! I really can't wait to have this working :)
Cheers

Edit: I'm running on Manjaro Linux

@ShadyNawara,

Does the response from

response = self._write_then_read([0x38, 0x1, mode, bucketIndex])

I think it was just if it failed or not. Nothing else. For the gif issue though i tested it out yesterday and it was failing on ubuntu but works fine on windows and mac. I need to look into it more on why is that.

@donbernhardo

Which OS are you mainly using?

@donbernhardo

Which OS are you mainly using?

Manjaro Linux (It's an Arch based distribution)

Manjaro Linux (It's an Arch based distribution)

Thank you, i think there is a possibility that its a linux specific issue, I will investigate. Does it fail after a specific number of gifs or is it random?

Manjaro Linux (It's an Arch based distribution)

Thank you, i think there is a possibility that its a linux specific issue, I will investigate. Does it fail after a specific number of gifs or is it random?

For the same set of gifs as test inputs, the error is deterministic, repeatable and appears after a couple of iterations. The bigger the gif-files are, the earlier the error occurs:

[PCMR bernhard]# ./kraken_test.sh
Uploading: /home/bernhard/.kgifs/08y.gif Filesize: 1195680 AccumulatedSize: 1195680
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 1, 0, 0, ...
Uploading: /home/bernhard/.kgifs/0sZ.gif Filesize: 2051051 AccumulatedSize: 3246731
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 2, 0, 0, ...
Uploading: /home/bernhard/.kgifs/0xN.gif Filesize: 1008552 AccumulatedSize: 4255283
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 3, 0, 0, ...
Uploading: /home/bernhard/.kgifs/11-2.gif Filesize: 1380871 AccumulatedSize: 5636154
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 4, 0, 0, ...
Uploading: /home/bernhard/.kgifs/11643.gif Filesize: 6517522 AccumulatedSize: 12153676
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 5, 0, 0, ...
Uploading: /home/bernhard/.kgifs/12-2.gif Filesize: 261527 AccumulatedSize: 12415203
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 6, 0, 0, ...
Uploading: /home/bernhard/.kgifs/13-2.gif Filesize: 353512 AccumulatedSize: 12768715
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 7, 0, 0, ...
Uploading: /home/bernhard/.kgifs/1-3.gif Filesize: 1881147 AccumulatedSize: 14649862
   writing to bucket:  [49, 4, 37, 0, 55, 0, 10, 80, 49, 75, 51, 51, 51, 32, 8, 0, 0, ...
WARNING: Failed to switch active bucket

BTW: After the error occured the kraken needs a power cycle to respond again to any command. re-initializing or just a reboot (without power cycle) keeps the device unresponsive.

Manjaro Linux (It's an Arch based distribution)

Thank you, i think there is a possibility that its a linux specific issue, I will investigate. Does it fail after a specific number of gifs or is it random?

UPDATE: I am able to replicate the same error on Windows 11 and the behaviour is identical as if I run it on Linux:

C:\Users\bernhard\Documents>kraken_test.bat
WARNING: Failed to switch active bucket

If you need any of the output capturing with --debug mode then let me know.

@donbernhardo
can you send me your input test files?

@donbernhardo can you send me your input test files?

Here are the input gif files (and a script to upload them one after another):
gifs.zip

Here is the test record when running the included shell script on linux:

[bernhard@PCMR gifs]$ ./kraken_test.sh                                                                                                          INT ✘  11s  
Uploading: /home/bernhard/gifs/0sZ.gif Filesize: 2051051 AccumulatedSize: 2051051
Uploading: /home/bernhard/gifs/0xN.gif Filesize: 1008552 AccumulatedSize: 3059603
Uploading: /home/bernhard/gifs/1-3.gif Filesize: 1881147 AccumulatedSize: 4940750
Uploading: /home/bernhard/gifs/1apV.gif Filesize: 503024 AccumulatedSize: 5443774
Uploading: /home/bernhard/gifs/1cwq.gif Filesize: 2023483 AccumulatedSize: 7467257
Uploading: /home/bernhard/gifs/1dJe.gif Filesize: 5990 AccumulatedSize: 7473247
Uploading: /home/bernhard/gifs/1Gyi.gif Filesize: 309037 AccumulatedSize: 7782284
Uploading: /home/bernhard/gifs/1iAp.gif Filesize: 904303 AccumulatedSize: 8686587
Uploading: /home/bernhard/gifs/1iMz.gif Filesize: 252324 AccumulatedSize: 8938911
Uploading: /home/bernhard/gifs/2unv.gif Filesize: 697787 AccumulatedSize: 9636698
Uploading: /home/bernhard/gifs/3-2.gif Filesize: 573199 AccumulatedSize: 10209897
WARNING: Failed to switch active bucket
Uploading: /home/bernhard/gifs/3adb.gif Filesize: 1372645 AccumulatedSize: 11582542
Uploading: /home/bernhard/gifs/3TjN.gif Filesize: 211147 AccumulatedSize: 11793689
WARNING: Failed to switch active bucket
Uploading: /home/bernhard/gifs/3U3O.gif Filesize: 445537 AccumulatedSize: 12239226
WARNING: Failed to switch active bucket
Uploading: /home/bernhard/gifs/4Cb2.gif Filesize: 297271 AccumulatedSize: 12536497
WARNING: Failed to switch active bucket

@donbernhardo I just pushed an update that should resolve the issue if you can give it a try. I tested it by running your gif set on loop 600 times and it seems to be working fine.

fyi, when the device runs out of memory it will briefly switch to liquid mode before loading the gif. The nzxt cam software does a similar thing except they switch to their loading gif instead but I think liquid mode is faster and more reliable.

I'm not hitting any errors on the latest code. Ubuntu 22.04.

@ShadyNawara
I just tested with your latest update. On-device memory management seems to be rock solid now! I can't get it to break no matter how hard I hit it with gifs :)

Thanks a lot for this fix. I hope this branch will make it into a release soon. I have a feeling it will make a lot of people happy espeacially with all the automation one can do way beyond the nzxt cam software!

Love what you're doing here, I just have a question:
will there be a feature to have to a color configurable liquid reading like CAM offers?
I really like seeing my liquid temp, I would just prefer it not to be plain white.
In theory one could just create an image with this data and refresh it in certain intervals, but I'm not aware of a way to create an image like that in the first place.

@th3voic3

In theory one could just create an image with this data and refresh it in certain intervals, but I'm not aware of a way to create an image like that in the first place.

Thats actually what cam does. It creates an image for each frame and pushes it to the device.

you might want to check out: Kraken Z Playground i think it does what you are looking for.

After way too long (sorry about that), merged as is. In the next few days I'll probably push some minor complementary docs/readme changes directly to the main branch.

One remaining thing that you might want to look into is when there are multiple simultaneous driver instances in use. In part kernel restrictions should already serialize the accesses to the bulk interface, but that might not be sufficient for reliable manipulation of the screen from multiple concurrent instances.

@th3voic3

In theory one could just create an image with this data and refresh it in certain intervals, but I'm not aware of a way to create an image like that in the first place.

Thats actually what cam does. It creates an image for each frame and pushes it to the device.

you might want to check out: Kraken Z Playground i think it does what you are looking for.

I took a look, but in the end I just took a screenshot of the dual infographic and wrote a bash script that adds the CPU and liquid temps in two steps with imagemagick. It works 🤷‍♂️

I took a look, but in the end I just took a screenshot of the dual infographic and wrote a bash script that adds the CPU and liquid temps in two steps with imagemagick. It works man_shrugging

That sounds awesome - can you share it?

I took a look, but in the end I just took a screenshot of the dual infographic and wrote a bash script that adds the CPU and liquid temps in two steps with imagemagick. It works man_shrugging

That sounds awesome - can you share it?

Okay so this is my source image.

Then I have a systemd timer:

/etc/systemd/system/nzxtupdate.timer

[Unit]
Description=Timer to update NZXT AIO LCD

[Timer]
OnUnitInactiveSec=15s
OnBootSec=10s
#OnCalendar=*:*:0/10

[Install]
WantedBy=timers.target

/etc/systemd/system/nzxtupdate.service

[Unit]
Description=Update NZXT AIO LCD

[Service]
User=th3voic3
ExecStart=/home/th3voic3/.scripts/nzxtupdate/set_dual_info.sh
Restart=always
RestartSec=10s

and the script:
~/.scripts/nzxtupdate/set_dual_info.sh

#!/bin/bash
cpu=`sensors | awk '/Package/ { print $4 }' | sed -e 's/+\(.*\).0°C/\1/'`; convert -font helvetica -fill white -pointsize 40 -gravity west -draw "text 40,0 '$cpu'" ~/.scripts/nzxtupdate/dual_info_empty.png /tmp/dual_info_cpu.png
liquid=`liquidctl -m kraken status | awk '/temperature/ { print $(NF-1)} ' | sed -e 's/\(.*\)\..*/\1/'`; convert -font helvetica -fill white -pointsize 40 -gravity east -draw "text 49,0 '$liquid'" /tmp/dual_info_cpu.png /tmp/dual_info_liquid.png
liquidctl -m kraken set lcd screen static /tmp/dual_info_liquid.png

I really like seeing my liquid temp, I would just prefer it not to be plain white.

It would be awesome if we could add in the color option to the liquid mode. Even better would be to have the dual cpu/gpu mode with color option.