Mathieu Othacehe <m.othac...@gmail.com> writes:
> * gnu/services/pm.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add gnu/services/tlp.scm.
> * doc/guix.texi (Power management Services): New section.
Hi Mathieu,
Very nice!
On my x200, with tlp, powertop reports:
The battery reports a discharge rate of 10.1 W
The estimated remaining time is 4 hours, 52 minutes
Without tlp:
The battery reports a discharge rate of 11.9 W
The estimated remaining time is 4 hours, 11 minutes
Here are a few comments:
[...]
> +(define-module (gnu services pm)
> + #:use-module (guix gexp)
> + #:use-module (guix packages)
> + #:use-module (guix records)
> + #:use-module (gnu packages linux)
> + #:use-module (gnu services)
> + #:use-module (gnu services base)
> + #:use-module (gnu services configuration)
> + #:use-module (gnu services shepherd)
> + #:use-module (gnu system shadow)
> + #:export (tlp-service-type
> + tlp-configuration
> + generate-tlp-documentation))
I don't think generate-tlp-documentation needs to be exported.
> +(define (uglify-field-name field-name)
> + (let ((str (symbol->string field-name)))
> + (string-join (string-split
> + (string-upcase
> + (if (string-suffix? "?" str)
> + (substring str 0 (1- (string-length str)))
> + str))
> + #\-)
> + "_")))
> +
> +(define (serialize-field field-name val)
> + (format #t "~a=~a\n" (uglify-field-name field-name) val))
> +
> +(define (serialize-boolean field-name val)
> + (serialize-field field-name (if val "1" "0")))
> +(define-maybe boolean)
> +
> +(define (serialize-string field-name val)
> + (serialize-field field-name val))
> +(define-maybe string)
> +
> +(define (space-separated-string-list? val)
> + (and (list? val)
> + (and-map (lambda (x)
> + (and (string? x) (not (string-index x #\space))))
> + val)))
> +(define (serialize-space-separated-string-list field-name val)
> + (serialize-field field-name
> + (format #f "~s"
> + (string-join val " "))))
> +(define-maybe space-separated-string-list)
> +
> +(define (non-negative-integer? val)
> + (and (exact-integer? val) (not (negative? val))))
> +(define (serialize-non-negative-integer field-name val)
> + (serialize-field field-name val))
> +(define-maybe non-negative-integer)
> +
> +(define (on-off-boolean? val)
> + (boolean? val))
> +(define (serialize-on-off-boolean field-name val)
> + (serialize-field field-name (if val "on" "off")))
> +(define-maybe on-off-boolean)
> +
> +(define (y-n-boolean? val)
> + (boolean? val))
> +(define (serialize-y-n-boolean field-name val)
> + (serialize-field field-name (if val "Y" "N")))
> +
> +(define-configuration tlp-configuration
> + (tlp
> + (package tlp)
> + "The TLP package.")
> +
> + (tlp-enable?
> + (boolean #t)
> + "Set to true if you wish to enable TLP.")
>From this point, the indentation is broken.
> + (tlp-default-mode
> + (string "AC")
> + "Default mode when no power supply can be detected. Alternatives are
> +AC and BAT.")
> +
> + (disk-idle-secs-on-ac
> + (non-negative-integer 0)
> + "Number of seconds Linux kernel has to wait after the disk goes idle,
> +before syncing on AC.")
> +
> + (disk-idle-secs-on-bat
> + (non-negative-integer 2)
> + "Same as @code{disk-idle-ac} but on BAT mode.")
> +
> + (max-lost-work-secs-on-ac
> + (non-negative-integer 15)
> + "Dirty pages flushing periodicity, expressed in seconds.")
> +
> + (max-lost-work-secs-on-bat
> + (non-negative-integer 60)
> + "Same as @code{max-lost-work-secs-on-ac} but on BAT mode.")
> +
> + (cpu-scaling-governor-on-ac
> + (maybe-space-separated-string-list 'disabled)
> + "CPU frequency scaling governor on AC mode. With intel_pstate
> +driver, alternatives are powersave and performance. With acpi-cpufreq driver,
> +alternatives are ondemand, powersave, performance and conservative.")
Please, could you put two spaces between phrases? :) As in "and
performance. With...". I've seen this in other parts of your patch as
well.
[...]
> +(define (tlp-shepherd-service config)
> + (let* ((tlp-bin (file-append
> + (tlp-configuration-tlp config) "/bin/tlp"))
> + (tlp-action (lambda args
> + #~(lambda _
> + (zero? (system* #$tlp-bin #$@args))))))
> + (list (shepherd-service
> + (documentation "Run TLP script.")
> + (provision '(tlp))
> + (requirement '(syslogd user-processes))
I have not been able to see anything related to tlp in
/var/log/messages. And if I set trace mode (TLP_DEBUG, in /etc/tlp), I
have an error message: "logger: unknown facility name: debug", which I
think could be patched in tlp source, maybe by removing "-p debug". Or
maybe it is logger that needs to be patched, I don't know.
Otherwise syslogd would not be needed here. WDYT?
BTW, could you consider adding TLP_DEBUG to the service?
> + (start (tlp-action "init" "start"))
> + (stop (tlp-action "init" "stop"))))))
> +
> +(define (tlp-activation config)
> + (let* ((config-dir "/etc")
> + (config-str (with-output-to-string
> + (lambda ()
> + (serialize-configuration
> + config
> + tlp-configuration-fields))))
> + (config-file (plain-file "tlp" config-str)))
> + #~(begin
> + (use-modules (guix build utils))
> + (mkdir-p #$config-dir)
> + (copy-file #$config-file (string-append #$config-dir "/tlp")))))
Here I think it is better to wrap the gexp in (with-imported-modules
'((guix build utils)) ...), as in the CUPS service for example, even
though I don't understand fully the consequences of not doing it.
[...]
Thanks for this :)
Clément
