Add experimental support for H110i GT #637
Conversation
|
This is almost a 1:1 version of the original PR with simplified code in some parts. |
|
fwiw: |
|
Per tests with @CRCinAU there is something broken in the driver when writing the fan/pump settings. The original version works fine. |
|
Per last tests, this version now correctly changes the pump mode when using |
|
Pump mode is now entirely supported with |
|
Custom fan curves now work properly with max RPM per channel being read on demand. |
|
@aleksamagicka Changes done per review. I see macOS tests are failing but it's not related to this PR, cannot rerun them either. Any clue ? |
|
I reran them and the issue persists, I'm not sure why that's happening as I've had very little contact with Mac machines generally. Perhaps Github changed something about their runners. |
|
If you're OK with this peculiar situation, I suggest we move forward with the PR. We can ask Jonas for additional input if you prefer to be on the safe side. |
|
This doesn't seem to be tied to this PR specifically, I just reran it in another PR and it fails as well. Seems like libusb or something like it is missing? @jonasmalacofilho, tagging you since you surely know more about this than me :) |
|
The problem isn't related to this PR, but rather to a change in what the Homebrew has a patch for its Python formulas to fix this issue, but we're using There doesn't seem to be an obvious way to handle this... we want to look in the Homebrew library path, but we also need to decide whether to give it priority or not, and also check that the architecture matches (at least 67E6 if that isn't the last path tried). It's quite surprising that something like this was left for downstreams to solve. And it seems that some are resorting to shipping their own copies of the relevant dylibs... Anyway, this is something I'll try to handle in PyUSB, so it really isn't related to liquidctl at all. In the meantime, @aleksamagicka, maybe we should pin our tests to run on Mac 12 (still Intel)? My reasoning is that outdated tests are better than no tests at all. We could instead disable the affected test cases, but I think they are worth more than having all the others running on the new runner. The whole point of these affected tests is that they are the only ones that actually test (something) about the platform, all others are almost exclusively using mocks. |
I think we should, no reason not to let them run - and not get CI errors each time until it's resolved. (And as I understand, that's what they've been running on anyway up to now?) |
|
And they did detect a real issue as soon as they ran on the affected platform. : ) (Too bad it's something that, in hindsight, PyUSB me should have fixed a long time ago... I probably underestimated the number of Mac users running non-brew Python: if that was truly as rare as I thought, actions/setup-python would probably use brew Python too). |
Touché :) Maybe, when this is fixed, we can leave macos-12 in with macos-latest (at least, until macos-12 is deprecated) and cover both. |
| """ | ||
| num = 1 | ||
| while True: | ||
| yield (num % 31) + 1 |
There was a problem hiding this comment.
Is this really needed for this device? In the protocol docs you said a simple counter is sufficient, and this is slightly more complex that that.
There was a problem hiding this comment.
At some point, we used random sequence numbers but it didn't work right. To simplify from the original PR, I changed it to a simple sequence outside of storage. We could change it to an even simpler yield num % 31, but I'm not sure if the current implementation is really that cumbersome.
There was a problem hiding this comment.
The fact that random sequence numbers didn't work well suggests that some jumps in sequence numbers aren't acceptable to the device. Thus restarting the sequence at every invocation should also sometimes cause issues, and persisting the sequence in storage may be necessary after all.
While numbers modulo 32 seem to work, why not using the simpler full byte range (i.e. modulo 256), as seen in the captures? Well, actually, the full range except 0, which doesn't appear to be used. Something like
yield (num % 255) + 1There was a problem hiding this comment.
We could use that but it implies testing the device again. Perhaps a quicker method (that's known to work) is to use the old implementation.
There was a problem hiding this comment.
Fair point. And since this appears to work, and from sanmai's comment there that in general any per-command sequence seems to work, let's just keep things as is. At least for now/until bugs are reported...
|
Just a few minor notes (see above).
Please add the data dumps to https://github.com/liquidctl/collected-device-data. |
Ack. I think they might not be necessary with the fix being made (and tested) in PyUSB. But since Intel Macs are much more relevant to our project than most (because of Hackintoshes), maybe the extra CI jobs are worth it. |
Related: pyusb/pysub#511 Related: #637 (comment)
Related: pyusb/pysub#511 Related: #637 (comment)
|
|
Thanks! |
|
Finally ! Thank you all, especially @CRCinAU for the many hours spent reversing and debugging :) |
Describe what the changes are meant to address.
If the implementation is not trivial, please also include a short overview.
Fixes: n/a
Closes: n/a
Related: #142 #147
Checklist:
docs/*guide.mddevice guides, with "new/changed in" notesliquidctl.8Linux/Unix/Mac OS man pagedocs/developer/protocolNew CLI flag?
extra/completions/New device?
extra/linux/71-liquidctl.rules(instructions in the file header)e) andgitMRLVNew driver?
docs/developer/protocol/