Temperature sensor offset support for the Aquacomputer driver#534
Temperature sensor offset support for the Aquacomputer driver#534aleksamagicka wants to merge 20 commits intoliquidctl:mainliquidctl/liquidctl:mainfrom aleksamagicka:aqc-temp-offset-supportaleksamagicka/liquidctl-aqc:aqc-temp-offset-supportCopy head branch name to clipboard
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
|
( |
jonasmalacofilho
left a comment
There was a problem hiding this comment.
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 --versionWith this change, the following should work:
$ python -m liquidctl -m kraken -v set pump speed -- -10
(
_perform_write()could also be used for thenzxt-kraken3driver 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.
| 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.
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.
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.
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 ×
namepair, 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.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/