-
Notifications
You must be signed in to change notification settings - Fork 208
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.
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
Temperature sensor offset support for the Aquacomputer driver #534
base: main
Are you sure you want to change the base?
Temperature sensor offset support for the Aquacomputer driver #534
Conversation
Since temp offsets can be negative (for these devices they can range from -15 to 15C), what would be the best way to support negative values in the CLI? |
7c3e4a9
to
2d09554
Compare
( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since temp offsets can be negative (for these devices they can range from -15 to 15C), what would be the best way to support negative values in the CLI?
One alternative I'm aware of is enabling --
to separate the positional arguments:
diff --git a/liquidctl/cli.py b/liquidctl/cli.py
index 52a3f762dc48..35ad7c9be5b1 100644
--- a/liquidctl/cli.py
+++ b/liquidctl/cli.py
@@ -4,10 +4,10 @@ Usage:
liquidctl [options] list
liquidctl [options] initialize [all]
liquidctl [options] status
- liquidctl [options] set <channel> speed (<temperature> <percentage>) ...
- liquidctl [options] set <channel> speed <percentage>
- liquidctl [options] set <channel> color <mode> [<color>] ...
- liquidctl [options] set <channel> screen <mode> [<value>]
+ liquidctl [options] set <channel> speed [--] (<temperature> <percentage>) ...
+ liquidctl [options] set <channel> speed [--] <percentage>
+ liquidctl [options] set <channel> color [--] <mode> [<color>] ...
+ liquidctl [options] set <channel> screen [--] <mode> [<value>]
liquidctl --help
liquidctl --version
With this change, the following should work:
$ python -m liquidctl -m kraken -v set pump speed -- -10
(
_perform_write()
could also be used for thenzxt-kraken3
driver in the other PR, since the same logic is being applied there.)
Maybe add it to liquidctl.util
or liquidctl.hwmon
, and (try) to use it in all hwmon-aware drivers?
Just make sure that the messages that are emitted and the guidelines in docs/developer/hwmon.md
are in sync.
@@ -8,6 +8,7 @@ | |||
liquidctl [options] set <channel> speed <percentage> | |||
liquidctl [options] set <channel> color <mode> [<color>] ... | |||
liquidctl [options] set <channel> screen <mode> [<value>] | |||
liquidctl [options] set <channel> tempoffset [<value>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this generic over the "property name", in order to avoid having to continuously add new commands for very-device-specific features (see also #297)?
Additionally, at this point this CLI consistently favors fully spelled out names and avoiding abbreviations: e.g. temperature-offset
. (If this remains a command, temperature offset
would also be an alternative).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I've just rebased this branch to stay on top of things.
I've looked at the linked issue and the related ones, and AFAICS: there isn't a clear way to do that currently, but it should be resolved somehow because we're hitting into it already. I'm not quite sure what you mean by "property name" - do you maybe mean that there should be a _device_set_misc()
handler that the drivers would use to their will? speed
, color
and screen
are general enough, but now we have temp offsets (that other devices don't) for starters.
The aquacomputer devices can, off the top of my head, have one fan configured to follow another, use PID calculations for setting fan speed, save retrieve/sensor names etc, which I don't think has been the case for the devices that are currently supported (or at least, not implemented yet). I'm listing these features so we have an idea of what can be supported and plan the CLI approach for supporting that (I'd say) relatively unique stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure what you mean by "property name" - do you maybe mean that there should be a
_device_set_misc()
handler that the drivers would use to their will?
Yeah, something like that.
By property name I meant modelling these miscellaneous features as name-value pairs:
# private for now
def _set_property(self, name: str, value: Any):
# ...
It should support value
being either:
- a string, for when it's coming from the CLI, which won't know how to parse it;
- or a suitably typed value for the given driver ×
name
pair, for other/direct callers.
And on the CLI side:
liquidctl [options] set <property> <value>
Also maybe
liquidctl [options] set <property> <value> ...
if we feel that lists are common enough. But then _set_property
also needs to handle when value
is a list of strings.
…em when running `status`
…mp offset writing through hwmon
af1c9b2
to
626ca47
Compare
Add support for reading and setting temperature sensor offsets for the currently supported Aquacomputer devices.
This adds a new CLI option,
set <channel> tempoffset <value>
.Checklist:
docs/*guide.md
device guides, with "new/changed in" notesliquidctl.8
Linux/Unix/Mac OS man pagedocs/developer/protocol
New CLI flag?
extra/completions/
New device?
extra/linux/71-liquidctl.rules
(instructions in the file header)e
) andgit
MRLVNew driver?
docs/developer/protocol/