Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

1.3.2 regression: kraken_two.get_status() returns wrong fan RPM value #87

Closed
leinardi opened this issue Feb 16, 2020 · 12 comments
Closed

1.3.2 regression: kraken_two.get_status() returns wrong fan RPM value #87

leinardi opened this issue Feb 16, 2020 · 12 comments
Labels
bug Apparent bug in liquidctl

Comments

@leinardi
Copy link

leinardi commented Feb 16, 2020 •
edited

I'm migrating GKraken from liquidctl 1.1.0 to 1.3.2 and I just noticed that the call to get_status() returns a wrong value for the RPM fan:
image

The very weird thing is that the values seems to be delayed: if I set the fan to 100% I see no immediate update but if I wait around 3 minutes I see the RPM go up. I'm polling liquidctl every 3 seconds.

If I downgrade to version 1.2.0 or 1.1.0 everything works fine as before.

Just FYI, I'm lazy loading the driver instance and reusing it every time I poll (so I'm creating and connecting to it only once and then polling it every 3 seconds).

I'm able to reproduce the issue with this simple script:

#!/usr/bin/env python3
from time import sleep

from liquidctl.driver.kraken_two import KrakenTwoDriver

if __name__ == "__main__":
    dev = next((dev for dev in KrakenTwoDriver.find_supported_devices()), None)
    print(dev.description)
    dev.connect()
    print(dev.get_status())
    dev.set_fixed_speed('fan', 100)
    sleep(4)
    print(dev.get_status())
    dev.set_fixed_speed('fan', 60)
    sleep(4)
    print(dev.get_status())
    dev.disconnect()

1.1.0 and 1.2.0 work correctly, 1.3.2 does not.
@leinardi leinardi changed the title 1.3.2 regression: kraken_twe.get_status() returns wrong fan RPM value 1.3.2 regression: kraken_two.get_status() returns wrong fan RPM value Feb 16, 2020
@jonasmalacofilho
Copy link
Member

I can reproduce this, but only while a connection has held, and only when using the (currently) default hid=hidraw backend. Thanks for reporting this!

This leads to me believe that the reports are being queued, and that we're seeing stale data (as you mentioned: updates are delayed). I remember thinking a bit about this when checking the HIDAPI sources, as it has a queue and I wasn't sure whether this would cause issues or not; but apparently I only checked the liquidctl CLI, which doesn't keep the connection long enough for it to matter, and is thus unaffected.

I'll try to fix this ASAP.

@jonasmalacofilho jonasmalacofilho added the bug Apparent bug in liquidctl label Feb 16, 2020
jonasmalacofilho added a commit that referenced this issue Feb 16, 2020
@jonasmalacofilho
Traverse all hid/hidraw queued reports and return the last one
2050d0b 
As incoming HID reports are queued by the OS, HidapiDevice.read() would
return stale data to the drivers.  The drivers assume the reads aren't
queued because that's not necessary with the currently supported
devices.

This didn't affect the liquidctl CLI as the connection to HIDAPI
wouldn't last long enough for too much data to be queued and lost.  But
for downstream projects like GKraken this was a significant regression
from liquidctl 1.1.0.

To keep the behavior consistent between the HIDAPI and Libusb backends,
traverse all queued (incoming) reports until the last one is found, and
only return it.

Related: #87 ("1.3.2 regression: kraken_two.get_status() returns wrong
fan RPM value")
Reported-by: @leinardi
@jonasmalacofilho
Copy link
Member

@leinardi

Can you give the latest 'master' a try? It passes my tests (using a slightly more aggressive example than yours), but please double check.

Assuming it's indeed fixed, I'll do a patch 1.3.3 release.

@jonasmalacofilho
Copy link
Member

@CaseySJ

Can you also give the latest 'master' a try with your Smart Device V2 on macOS?

I'm worried that the fix I just pushed might break devices like the SDV2.

@leinardi
Copy link
Author

@jonasmalacofilho sure! I'll try to test it soon and report back :)

@CaseySJ
Copy link
Contributor

CaseySJ commented Feb 17, 2020 •
edited

@CaseySJ

Can you also give the latest 'master' a try with your Smart Device V2 on macOS?

I'm worried that the fix I just pushed might break devices like the SDV2.

@jonasmalacofilho,

Just downloaded and tested the Master branch. The changes are working correctly except for one possible issue. For the Smart Device V2 it looks like the initialize function returns information about LED channels only. And the status function returns information about FAN channels only. Not sure if this is the expected behavior.

$ liquidctl list
Device ID 0: NZXT Smart Device V2 (experimental)
Device ID 1: NZXT Kraken X (X42, X52, X62 or X72)

This is the Smart Device V2. Both LED channels are used and only FAN2 is used.
But no information about FAN2 appears below. Is this normal?
$ liquidctl -d 0 initialize   
NZXT Smart Device V2 (experimental)
├── Firmware version                      1.5.0  
├── LED 1 accessory 1    HUE 2 LED Strip 300 mm  
├── LED 1 accessory 2    HUE 2 Underglow 200 mm  
├── LED 1 accessory 3    HUE 2 Underglow 200 mm  
├── LED 2 accessory 1          AER RGB 2 140 mm  
├── LED 2 accessory 2          AER RGB 2 140 mm  
├── LED 2 accessory 3          AER RGB 2 140 mm  
└── LED 2 accessory 4          AER RGB 2 120 mm  

$ liquidctl -d 1 initialize   <-- this is Kraken X52
<no output -- I think this is normal for Kraken?>

All of these commands run correctly:
$ liquidctl -d 0 set led1 color backwards-spectrum-wave --speed normal
$ liquidctl -d 0 set led2 color backwards-spectrum-wave --speed normal
$ liquidctl -d 1 set fan speed 20 25 32 40 42 52 52 70 60 100
$ liquidctl -d 1 set pump speed 20 60 60 100
$ liquidctl -d 1 set ring color spectrum-wave
$ liquidctl -d 1 set logo color spectrum-wave

$ liquidctl -d 0 status
NZXT Smart Device V2 (experimental)
├── Fan 2 duty       50  %
├── Fan 2 speed    1041  rpm
└── Noise level      78  dB

$ liquidctl -d 1 status
NZXT Kraken X (X42, X52, X62 or X72)
├── Liquid temperature     23.8  °C
├── Fan speed               658  rpm
├── Pump speed             1894  rpm
└── Firmware version      6.0.2

UPDATE: On second thought, that behavior is correct. LED configurations do not change from call to call. Only fan status changes between calls. So it makes sense for status to show only current fan speed and overall noise level.

@leinardi
Copy link
Author

@jonasmalacofilho just tested and 2050d0b seems to work fine!

@jonasmalacofilho
Copy link
Member

jonasmalacofilho commented Feb 17, 2020 •
edited

@leinardi, @CaseySJ

Thank you both!

@CaseySJ, that has been like that for a while already. Moving the LED part of the output to initialize allowed us to fix the flickering that trying to request that information caused. It it also made get_status lighter and faster.

jonasmalacofilho added a commit that referenced this issue Feb 18, 2020
@jonasmalacofilho
Traverse all hid/hidraw queued reports and return the last one
5de9973 
As incoming HID reports are queued by the OS, HidapiDevice.read() would
return stale data to the drivers.  The drivers assume the reads aren't
queued because that's not necessary with the currently supported
devices.

This didn't affect the liquidctl CLI as the connection to HIDAPI
wouldn't last long enough for too much data to be queued and lost.  But
for downstream projects like GKraken this was a significant regression
from liquidctl 1.1.0.

To keep the behavior consistent between the HIDAPI and Libusb backends,
traverse all queued (incoming) reports until the last one is found, and
only return it.

Related: #87 ("1.3.2 regression: kraken_two.get_status() returns wrong
fan RPM value")
Reported-by: @leinardi
jonasmalacofilho added a commit that referenced this issue Feb 18, 2020
@jonasmalacofilho
Only explicitly clear already enqueued HID reports
fd4ccd7 
Commit 2050d0b ("Traverse all hid/hidraw queued reports and return the
last one") aggressively started to clear all enqueued reports whenever a
read was done on a HID using hidapi.

While that seemed to work ok, it could cause important reports to be
lost, which might eventually break some drivers.  As no contributor has
enough devices to test all drivers, it is better to be conservative and
only explicitly clear enqueued reports.

Notably, it is wiser to clear the queue before any writes that precede
one or more reads.  This will clear stale data that has already been
queued, but will not risk loosing expected responses.  And drivers that
expect reads to match specific writes should already be prepared to
handle what is left.

The objective is not definitively clear all previous reports, but simply
to avoid acting on (or forwarding to the user) stale data.  The subtle
difference is that _stale_ presumes a delay of more than infinitesimal
length.

Related: #87 ("1.3.2 regression: kraken_two.get_status() returns wrong
fan RPM value")
jonasmalacofilho added a commit that referenced this issue Feb 18, 2020
@jonasmalacofilho
Only explicitly clear already enqueued HID reports
803582f 
Commit 2050d0b ("Traverse all hid/hidraw queued reports and return the
last one") aggressively started to clear all enqueued reports whenever a
read was done on a HID using hidapi.

While that seemed to work ok, it could cause important reports to be
lost, which might eventually break some drivers.  As no contributor has
enough devices to test all drivers, it is better to be conservative and
only explicitly clear enqueued reports.

Notably, it is wiser to clear the queue before any writes that precede
one or more reads.  This will clear stale data that has already been
queued, but will not risk loosing expected responses.  And drivers that
expect reads to match specific writes should already be prepared to
handle what is left.

The objective is not definitively clear all previous reports, but simply
to avoid acting on (or forwarding to the user) stale data.  The subtle
difference is that _stale_ presumes a delay of more than infinitesimal
length.

Related: #87 ("1.3.2 regression: kraken_two.get_status() returns wrong
fan RPM value")
@jonasmalacofilho
Copy link
Member

This has been fixed in liquidctl v1.3.3.

By the way, I ended up reworking yesterday's fix to be more conservative, because I was still worried about potential problems with some of drivers I can't test myself.

@CaseySJ
Copy link
Contributor

CaseySJ commented Feb 18, 2020

@jonasmalacofilho,

Are you going to issue a Pull Request to Homebrew-Core for v1.3.3?

@jonasmalacofilho
Copy link
Member

@CaseySJ can you still take care of those (homebrew-core and our tap)?

I rather you did it because you can actually test the result on macOS.

@CaseySJ
Copy link
Contributor

CaseySJ commented Feb 20, 2020

@jonasmalacofilho

I should be able to submit 1.3.3 to homebrew-core this evening. Will update the tap as well.

@jonasmalacofilho
Copy link
Member

@CaseySJ

Thank you, 1.3.3 is already up on homebrew-core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Apparent bug in liquidctl
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@leinardi @jonasmalacofilho @CaseySJ