Subject: 28.0.50; Various battery.el improvements

From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 11 Jun 2020 16:47:40 +0100

X-Debbugs-Cc: Mattias Engdegård <[email-masked]>
Tags: patch

While working on bug#39491, I came across various opportunities for
improvement in battery.el.

The attached patch reorders battery-status-function checks based on
obsolescence, adds two new faces for use in battery-mode-line-format,
makes battery-update more robust, fixes documentation, indentation, and
various regexps used, and simplifies file searches and logic.

CCing Mattias to comment on the rx changes.

WDYT?  Thanks,

-- 
Basil

>From 676bd462516b930adccfa83309a4fb6ab9189750 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <[email-masked]>
Date: Thu, 11 Jun 2020 13:48:37 +0100
Subject: [PATCH] Various battery.el improvements

* lisp/battery.el: Mention BSD support in Commentary.  Don't load
preloaded lisp/emacs-lisp/timer.el.
(battery--files): New function.
(battery--find-linux-sysfs-batteries): Use it and make fewer
syscalls.
(battery-status-function): Perform GNU/Linux checks in increasing
order of obsolescence: sysfs, ACPI, and then APM.  Simplify Darwin
check.  Ensure w32-battery-status is functionp, not just fboundp.
Add :version tag now that battery-upower is the default.
(battery-echo-area-format, battery-mode-line-format): Mention %s.
(battery-load-low, battery-load-critical): New faces.
(battery-update): Display battery-mode-line-format even if
percentage is N/A.  Apply faces battery-load-low or
battery-load-critical according to the percentage, but append them
so they don't override user customizations.  Update all mode lines
since we are in global-mode-string.
(battery-linux-proc-apm-regexp): Mark as obsolete, replacing with...
(battery--linux-proc-apm): ...this new rx definition.
(battery-linux-proc-apm): Use it.  Fix indentation.  Simplify.
(battery--acpi-rate, battery--acpi-capacity): New rx definitions.
(battery-linux-proc-acpi): Use them.  Fix pathological whitespace
regexps.  Simplify.
(battery-linux-sysfs): Fix docstring and indentation.  Reduce number
of file searches.  Simplify.
(battery-bsd-apm): Fix docstring.  Simplify.
(battery-pmset): Fix docstring.  Simplify ID regexp.

* lisp/emacs-lisp/rx.el (rx-define): Indent as a defun.

* test/lisp/battery-tests.el (battery-linux-proc-apm-regexp): Test
new battery--linux-proc-apm rx definition.
(battery-acpi-rate-regexp, battery-acpi-capacity-regexp): New tests.
---
 lisp/battery.el            | 365 ++++++++++++++++++++-----------------
 lisp/emacs-lisp/rx.el      |   2 +-
 test/lisp/battery-tests.el |  39 +++-
 3 files changed, 230 insertions(+), 176 deletions(-)

diff --git a/lisp/battery.el b/lisp/battery.el
index b8855a8ce3..142d22ea15 100644
--- a/lisp/battery.el
+++ b/lisp/battery.el
@@ -23,18 +23,18 @@
 
 ;;; Commentary:
 
-;; There is at present support for GNU/Linux, macOS, and Windows.
+;; There is at present support for GNU/Linux, BSD, macOS, and Windows.
 ;; This library supports:
 ;; - UPower (https://upower.freedesktop.org) via D-Bus API.
-;; - the `/sys/class/power_supply/' files of Linux >= 2.6.39.
-;; - the `/proc/acpi/' directory structure of Linux 2.4.20 and 2.6.
-;; - the `/proc/apm' file format of Linux version 1.3.58 or newer.
+;; - The `/sys/class/power_supply/' files of Linux >= 2.6.39.
+;; - The `/proc/acpi/' directory structure of Linux 2.4.20 and 2.6.
+;; - The `/proc/apm' file format of Linux version 1.3.58 or newer.
+;; - BSD by using the `apm' program.
 ;; - Darwin (macOS) by using the `pmset' program.
 ;; - Windows via the GetSystemPowerStatus API call.
 
 ;;; Code:
 
-(require 'timer)
 (require 'dbus)
 (eval-when-compile (require 'cl-lib))
 
@@ -60,39 +60,41 @@ battery-upower-line-power-device
 (defconst battery-upower-dbus-service "org.freedesktop.UPower"
   "Well-known UPower service name for the D-Bus system.")
 
+(defun battery--files (dir)
+  "Return a list of absolute file names in DIR or nil on error.
+Value does not include \".\" or \"..\"."
+  (ignore-errors (directory-files dir t directory-files-no-dot-files-regexp)))
+
 (defun battery--find-linux-sysfs-batteries ()
-  (let ((dirs nil))
-    (dolist (file (directory-files "/sys/class/power_supply/" t))
-      (when (and (or (file-directory-p file)
-                     (file-symlink-p file))
-                 (file-exists-p (expand-file-name "capacity" file)))
-        (push file dirs)))
+  "Return a list of all sysfs battery directories."
+  (let (dirs)
+    (dolist (dir (battery--files "/sys/class/power_supply/"))
+      (when (file-exists-p (expand-file-name "capacity" dir))
+        (push dir dirs)))
     (nreverse dirs)))
 
 (defcustom battery-status-function
   (cond ((dbus-ping :system battery-upower-dbus-service)
          #'battery-upower)
         ((and (eq system-type 'gnu/linux)
-	      (file-readable-p "/proc/apm"))
-	 #'battery-linux-proc-apm)
+              (battery--find-linux-sysfs-batteries))
+         #'battery-linux-sysfs)
 	((and (eq system-type 'gnu/linux)
 	      (file-directory-p "/proc/acpi/battery"))
 	 #'battery-linux-proc-acpi)
 	((and (eq system-type 'gnu/linux)
-	      (file-directory-p "/sys/class/power_supply/")
-              (battery--find-linux-sysfs-batteries))
-	 #'battery-linux-sysfs)
+              (file-readable-p "/proc/apm"))
+         #'battery-linux-proc-apm)
 	((and (eq system-type 'berkeley-unix)
 	      (file-executable-p "/usr/sbin/apm"))
 	 #'battery-bsd-apm)
 	((and (eq system-type 'darwin)
-	      (condition-case nil
-		  (with-temp-buffer
-		    (and (eq (call-process "pmset" nil t nil "-g" "ps") 0)
-			 (> (buffer-size) 0)))
-		(error nil)))
+              (ignore-errors
+                (with-temp-buffer
+                  (and (eq (call-process "pmset" nil t nil "-g" "ps") 0)
+                       (not (bobp))))))
 	 #'battery-pmset)
-	((fboundp 'w32-battery-status)
+        ((functionp 'w32-battery-status)
 	 #'w32-battery-status))
   "Function for getting battery status information.
 The function has to return an alist of conversion definitions.
@@ -102,6 +104,7 @@ battery-status-function
 
 CONVERSION is the character code of a \"conversion specification\"
 introduced by a `%' character in a control string."
+  :version "28.1"
   :type '(choice (const nil) function))
 
 (defcustom battery-echo-area-format
@@ -113,12 +116,13 @@ battery-echo-area-format
 `battery-status-function'.  Here are the ones generally available:
 %c Current capacity (mAh or mWh)
 %r Current rate of charge or discharge
+%L AC line status (verbose)
 %B Battery status (verbose)
 %b Battery status: empty means high, `-' means low,
    `!' means critical, and `+' means charging
 %d Temperature (in degrees Celsius)
-%L AC line status (verbose)
 %p Battery load percentage
+%s Remaining time (to charge or discharge) in seconds
 %m Remaining time (to charge or discharge) in minutes
 %h Remaining time (to charge or discharge) in hours
 %t Remaining time (to charge or discharge) in the form `h:min'"
@@ -134,7 +138,7 @@ battery-mode-line-limit
   :type 'integer)
 
 (defcustom battery-mode-line-format
-  (cond ((eq battery-status-function 'battery-linux-proc-acpi)
+  (cond ((eq battery-status-function #'battery-linux-proc-acpi)
 	 "[%b%p%%,%d°C]")
 	(battery-status-function
 	 "[%b%p%%]"))
@@ -145,12 +149,13 @@ battery-mode-line-format
 `battery-status-function'.  Here are the ones generally available:
 %c Current capacity (mAh or mWh)
 %r Current rate of charge or discharge
+%L AC line status (verbose)
 %B Battery status (verbose)
 %b Battery status: empty means high, `-' means low,
    `!' means critical, and `+' means charging
 %d Temperature (in degrees Celsius)
-%L AC line status (verbose)
 %p Battery load percentage
+%s Remaining time (to charge or discharge) in seconds
 %m Remaining time (to charge or discharge) in minutes
 %h Remaining time (to charge or discharge) in hours
 %t Remaining time (to charge or discharge) in the form `h:min'"
@@ -170,6 +175,18 @@ battery-load-critical
 A battery load percentage below this number is considered critical."
   :type 'integer)
 
+(defface battery-load-low
+  '((t :inherit warning))
+  "Face used in mode line string when battery load is low.
+See the option `battery-load-low'."
+  :version "28.1")
+
+(defface battery-load-critical
+  '((t :inherit error))
+  "Face used in mode line string when battery load is critical.
+See the option `battery-load-critical'."
+  :version "28.1")
+
 (defvar battery-update-timer nil
   "Interval timer object.")
 
@@ -202,7 +219,7 @@ display-battery-mode
 		(delq 'battery-mode-line-string global-mode-string))
 	(add-to-list 'global-mode-string 'battery-mode-line-string t)
 	(setq battery-update-timer (run-at-time nil battery-update-interval
-						'battery-update-handler))
+                                                #'battery-update-handler))
 	(battery-update))
     (message "Battery status not available")
     (setq display-battery-mode nil)))
@@ -214,34 +231,42 @@ battery-update-handler
 (defun battery-update ()
   "Update battery status information in the mode line."
   (let* ((data (and battery-status-function (funcall battery-status-function)))
-         (percentage (car (read-from-string (cdr (assq ?p data))))))
-    (setq battery-mode-line-string
-	  (propertize (if (and battery-mode-line-format
-			       (numberp percentage)
-                               (<= percentage battery-mode-line-limit))
-			  (battery-format battery-mode-line-format data)
-			"")
-		      'face
-                      (and (numberp percentage)
-                           (<= percentage battery-load-critical)
-                           'error)
-		      'help-echo "Battery status information")))
-  (force-mode-line-update))
+         (percentage (car (read-from-string (cdr (assq ?p data)))))
+         (res (and battery-mode-line-format
+                   (or (not (numberp percentage))
+                       (<= percentage battery-mode-line-limit))
+                   (battery-format battery-mode-line-format data)))
+         (len (length res)))
+    (unless (zerop len)
+      (cond ((not (numberp percentage)))
+            ((< percentage battery-load-critical)
+             (add-face-text-property 0 len 'battery-load-critical t res))
+            ((< percentage battery-load-low)
+             (add-face-text-property 0 len 'battery-load-low t res)))
+      (put-text-property 0 len 'help-echo "Battery status information" res))
+    (setq battery-mode-line-string (or res "")))
+  (force-mode-line-update t))
+
 
 ;;; `/proc/apm' interface for Linux.
 
-(defconst battery-linux-proc-apm-regexp
-  (concat "^\\([^ ]+\\)"		; Driver version.
-	  " \\([^ ]+\\)"		; APM BIOS version.
-	  " 0x\\([0-9a-f]+\\)"		; APM BIOS flags.
-	  " 0x\\([0-9a-f]+\\)"		; AC line status.
-	  " 0x\\([0-9a-f]+\\)"		; Battery status.
-	  " 0x\\([0-9a-f]+\\)"		; Battery flags.
-	  " \\(-?[0-9]+\\)%"		; Load percentage.
-	  " \\(-?[0-9]+\\)"		; Remaining time.
-	  " \\(.*\\)"			; Time unit.
-	  "$")
+;; Regular expression matching contents of `/proc/apm'.
+(rx-define battery--linux-proc-apm
+  (: bol   (group (+ (not ?\s)))        ; Driver version.
+     " "   (group (+ (not ?\s)))        ; APM BIOS version.
+     " 0x" (group (+ xdigit))           ; APM BIOS flags.
+     " 0x" (group (+ xdigit))           ; AC line status.
+     " 0x" (group (+ xdigit))           ; Battery status.
+     " 0x" (group (+ xdigit))           ; Battery flags.
+     " "   (group (? ?-) (+ digit)) ?%  ; Load percentage.
+     " "   (group (? ?-) (+ digit))     ; Remaining time.
+     " "   (group (* nonl))             ; Time unit
+     eol))
+
+(defconst battery-linux-proc-apm-regexp (rx battery--linux-proc-apm)
   "Regular expression matching contents of `/proc/apm'.")
+(make-obsolete-variable 'battery-linux-proc-apm-regexp
+                        "it is no longer used." "28.1")
 
 (defun battery-linux-proc-apm ()
   "Get APM status information from Linux (the kernel).
@@ -261,12 +286,12 @@ battery-linux-proc-apm
 %m Remaining time (to charge or discharge) in minutes
 %h Remaining time (to charge or discharge) in hours
 %t Remaining time (to charge or discharge) in the form `h:min'"
-  (let (driver-version bios-version bios-interface line-status
-	battery-status battery-status-symbol load-percentage
-	seconds minutes hours remaining-time tem)
+  (let ( driver-version bios-version bios-interface line-status
+         battery-status battery-status-symbol load-percentage
+         seconds minutes hours remaining-time tem )
     (with-temp-buffer
       (ignore-errors (insert-file-contents "/proc/apm"))
-      (when (re-search-forward battery-linux-proc-apm-regexp)
+      (when (re-search-forward (rx battery--linux-proc-apm) nil t)
 	(setq driver-version (match-string 1))
 	(setq bios-version (match-string 2))
 	(setq tem (string-to-number (match-string 3) 16))
@@ -279,9 +304,7 @@ battery-linux-proc-apm
 	  (cond ((= tem 0) (setq line-status "off-line"))
 		((= tem 1) (setq line-status "on-line"))
 		((= tem 2) (setq line-status "on backup")))
-	  (setq tem (string-to-number (match-string 6) 16))
-	  (if (= tem 255)
-	      (setq battery-status "N/A")
+          (unless (= (string-to-number (match-string 6) 16) 255)
 	    (setq tem (string-to-number (match-string 5) 16))
 	    (cond ((= tem 0) (setq battery-status "high"
 				   battery-status-symbol ""))
@@ -298,7 +321,7 @@ battery-linux-proc-apm
 	    (setq minutes (/ seconds 60)
 		  hours (/ seconds 3600))
 	    (setq remaining-time
-		  (format "%d:%02d" hours (- minutes (* 60 hours))))))))
+                  (format "%d:%02d" hours (% minutes 60)))))))
     (list (cons ?v (or driver-version "N/A"))
 	  (cons ?V (or bios-version "N/A"))
 	  (cons ?I (or bios-interface "N/A"))
@@ -306,27 +329,31 @@ battery-linux-proc-apm
 	  (cons ?B (or battery-status "N/A"))
 	  (cons ?b (or battery-status-symbol ""))
 	  (cons ?p (or load-percentage "N/A"))
-	  (cons ?s (or (and seconds (number-to-string seconds)) "N/A"))
-	  (cons ?m (or (and minutes (number-to-string minutes)) "N/A"))
-	  (cons ?h (or (and hours (number-to-string hours)) "N/A"))
+          (cons ?s (if seconds (number-to-string seconds) "N/A"))
+          (cons ?m (if minutes (number-to-string minutes) "N/A"))
+          (cons ?h (if hours (number-to-string hours) "N/A"))
 	  (cons ?t (or remaining-time "N/A")))))
 
 
 ;;; `/proc/acpi/' interface for Linux.
 
+(rx-define battery--acpi-rate (&rest hour)
+  (: (group (+ digit)) " " (group ?m (in "AW") hour)))
+(rx-define battery--acpi-capacity (battery--acpi-rate ?h))
+
 (defun battery-linux-proc-acpi ()
   "Get ACPI status information from Linux (the kernel).
-This function works only with the `/proc/acpi/' format introduced
-in Linux version 2.4.20 and 2.6.0.
+This function works only with the `/proc/acpi/' interface
+introduced in Linux version 2.4.20 and 2.6.0.
 
 The following %-sequences are provided:
 %c Current capacity (mAh)
-%r Current rate
+%r Current rate of charge or discharge
+%L AC line status (verbose)
 %B Battery status (verbose)
 %b Battery status, empty means high, `-' means low,
    `!' means critical, and `+' means charging
 %d Temperature (in degrees Celsius)
-%L AC line status (verbose)
 %p Battery load percentage
 %m Remaining time (to charge or discharge) in minutes
 %h Remaining time (to charge or discharge) in hours
@@ -342,45 +369,51 @@ battery-linux-proc-acpi
     ;; information together since displaying for a variable amount of
     ;; batteries seems overkill for format-strings.
     (with-temp-buffer
-      (dolist (dir (ignore-errors (directory-files "/proc/acpi/battery/"
-						   t "\\`[^.]")))
-	(erase-buffer)
-	(ignore-errors (insert-file-contents (expand-file-name "state" dir)))
-	(when (re-search-forward "present: +yes$" nil t)
-	  (and (re-search-forward "charging state: +\\(.*\\)$" nil t)
+      (dolist (dir (battery--files "/proc/acpi/battery/"))
+        (ignore-errors
+          (insert-file-contents (expand-file-name "state" dir) nil nil nil t))
+        (goto-char (point-min))
+        (when (re-search-forward (rx "present:" (+ space) "yes" eol) nil t)
+          (and (re-search-forward (rx "charging state:" (+ space)
+                                      (group (not space) (* nonl)) eol)
+                                  nil t)
 	       (member charging-state '("unknown" "charged" nil))
 	       ;; On most multi-battery systems, most of the time only one
 	       ;; battery is "charging"/"discharging", the others are
 	       ;; "unknown".
 	       (setq charging-state (match-string 1)))
-	  (when (re-search-forward "present rate: +\\([0-9]+\\) \\(m[AW]\\)$"
+          (when (re-search-forward (rx "present rate:" (+ space)
+                                       (battery--acpi-rate) eol)
 				   nil t)
 	    (setq rate (+ (or rate 0) (string-to-number (match-string 1))))
 	    (when (> rate 0)
-	      (setq rate-type (or (and rate-type
-				     (if (string= rate-type (match-string 2))
-					 rate-type
-				       (error
-					"Inconsistent rate types (%s vs. %s)"
-					rate-type (match-string 2))))
-				  (match-string 2)))))
-	  (when (re-search-forward "remaining capacity: +\\([0-9]+\\) m[AW]h$"
+              (cond ((not rate-type)
+                     (setq rate-type (match-string 2)))
+                    ((not (string= rate-type (match-string 2)))
+                     (error "Inconsistent rate types (%s vs. %s)"
+                            rate-type (match-string 2))))))
+          (when (re-search-forward (rx "remaining capacity:" (+ space)
+                                       battery--acpi-capacity eol)
 				   nil t)
 	    (setq capacity
 		  (+ (or capacity 0) (string-to-number (match-string 1))))))
 	(goto-char (point-max))
 	(ignore-errors (insert-file-contents (expand-file-name "info" dir)))
-	(when (re-search-forward "present: +yes$" nil t)
-	  (when (re-search-forward "design capacity: +\\([0-9]+\\) m[AW]h$"
+        (when (re-search-forward (rx "present:" (+ space) "yes" eol) nil t)
+          (when (re-search-forward (rx "design capacity:" (+ space)
+                                       battery--acpi-capacity eol)
 				   nil t)
 	    (cl-incf design-capacity (string-to-number (match-string 1))))
-	  (when (re-search-forward "last full capacity: +\\([0-9]+\\) m[AW]h$"
+          (when (re-search-forward (rx "last full capacity:" (+ space)
+                                       battery--acpi-capacity eol)
 				   nil t)
 	    (cl-incf last-full-capacity (string-to-number (match-string 1))))
-	  (when (re-search-forward
-		 "design capacity warning: +\\([0-9]+\\) m[AW]h$" nil t)
+          (when (re-search-forward (rx "design capacity warning:" (+ space)
+                                       battery--acpi-capacity eol)
+                                   nil t)
 	    (cl-incf warn (string-to-number (match-string 1))))
-	  (when (re-search-forward "design capacity low: +\\([0-9]+\\) m[AW]h$"
+          (when (re-search-forward (rx "design capacity low:" (+ space)
+                                       battery--acpi-capacity eol)
 				   nil t)
 	    (cl-incf low (string-to-number (match-string 1)))))))
     (setq full-capacity (if (> last-full-capacity 0)
@@ -394,79 +427,70 @@ battery-linux-proc-acpi
 				   60)
 				rate))
 	       hours (/ minutes 60)))
-    (list (cons ?c (or (and capacity (number-to-string capacity)) "N/A"))
+    (list (cons ?c (if capacity (number-to-string capacity) "N/A"))
 	  (cons ?L (or (battery-search-for-one-match-in-files
-			(mapcar (lambda (e) (concat e "/state"))
-				(ignore-errors
-				  (directory-files "/proc/acpi/ac_adapter/"
-						   t "\\`[^.]")))
-			"state: +\\(.*\\)$" 1)
-
+                        (mapcar (lambda (d) (expand-file-name "state" d))
+                                (battery--files "/proc/acpi/ac_adapter/"))
+                        (rx "state:" (+ space) (group (not space) (* nonl)) eol)
+                        1)
 		       "N/A"))
 	  (cons ?d (or (battery-search-for-one-match-in-files
-			(mapcar (lambda (e) (concat e "/temperature"))
-				(ignore-errors
-				  (directory-files "/proc/acpi/thermal_zone/"
-						   t "\\`[^.]")))
-			"temperature: +\\([0-9]+\\) C$" 1)
-
+                        (mapcar (lambda (d) (expand-file-name "temperature" d))
+                                (battery--files "/proc/acpi/thermal_zone/"))
+                        (rx "temperature:" (+ space) (group (+ digit)) " C" eol)
+                        1)
 		       "N/A"))
-	  (cons ?r (or (and rate (concat (number-to-string rate) " "
-					 rate-type)) "N/A"))
+          (cons ?r (if rate
+                       (concat (number-to-string rate) " " rate-type)
+                     "N/A"))
 	  (cons ?B (or charging-state "N/A"))
-	  (cons ?b (or (and (string= charging-state "charging") "+")
-		       (and capacity (< capacity low) "!")
-		       (and capacity (< capacity warn) "-")
-		       ""))
-	  (cons ?h (or (and hours (number-to-string hours)) "N/A"))
-	  (cons ?m (or (and minutes (number-to-string minutes)) "N/A"))
-	  (cons ?t (or (and minutes
-			    (format "%d:%02d" hours (- minutes (* 60 hours))))
-		       "N/A"))
-	  (cons ?p (or (and full-capacity capacity
-			    (> full-capacity 0)
-			    (number-to-string
-			     (floor (* 100 capacity) full-capacity)))
-		       "N/A")))))
+          (cons ?b (cond ((string= charging-state "charging") "+")
+                         ((and capacity (< capacity low)) "!")
+                         ((and capacity (< capacity warn)) "-")
+                         ("")))
+          (cons ?h (if hours (number-to-string hours) "N/A"))
+          (cons ?m (if minutes (number-to-string minutes) "N/A"))
+          (cons ?t (if minutes (format "%d:%02d" hours (% minutes 60)) "N/A"))
+          (cons ?p (if (and full-capacity capacity (> full-capacity 0))
+                       (number-to-string (floor (* 100 capacity) full-capacity))
+                     "N/A")))))
 
 
 ;;; `/sys/class/power_supply/BATN' interface for Linux.
 
 (defun battery-linux-sysfs ()
-  "Get ACPI status information from Linux kernel.
+  "Get sysfs status information from Linux kernel.
 This function works only with the new `/sys/class/power_supply/'
-format introduced in Linux version 2.4.25.
+interface introduced in Linux version 2.4.25.
 
 The following %-sequences are provided:
 %c Current capacity (mAh or mWh)
-%r Current rate
+%r Current rate of charge or discharge
+%L Power source (verbose)
 %B Battery status (verbose)
 %b Battery status, empty means high, `-' means low,
    `!' means critical, and `+' means charging
 %d Temperature (in degrees Celsius)
 %p Battery load percentage
-%L AC line status (verbose)
 %m Remaining time (to charge or discharge) in minutes
 %h Remaining time (to charge or discharge) in hours
 %t Remaining time (to charge or discharge) in the form `h:min'"
-  (let (charging-state temperature hours percentage-now
-        ;; Some batteries report charges and current, other energy and power.
+  (let (;; Some batteries report charges and current, others energy and power.
         ;; In order to reliably be able to combine those data, we convert them
         ;; all to energy/power (since we can't combine different charges if
         ;; they're not at the same voltage).
 	(energy-full 0.0)
 	(energy-now 0.0)
 	(power-now 0.0)
-	(voltage-now 10.8))    ;Arbitrary default, in case the info is missing.
+        (voltage-now 10.8) ; Arbitrary default, in case the info is missing.
+        charging-state temperature hours percentage-now)
     ;; SysFS provides information about each battery present in the
     ;; system in a separate subdirectory.  We are going to merge the
     ;; available information together.
     (with-temp-buffer
-      (dolist (dir (ignore-errors
-                     (battery--find-linux-sysfs-batteries)))
-	(erase-buffer)
-	(ignore-errors (insert-file-contents
-			(expand-file-name "uevent" dir)))
+      (dolist (dir (battery--find-linux-sysfs-batteries))
+        (ignore-errors
+          (insert-file-contents (expand-file-name "uevent" dir) nil nil nil t))
 	(goto-char (point-min))
 	(when (re-search-forward
 	       "POWER_SUPPLY_VOLTAGE_NOW=\\([0-9]*\\)$" nil t)
@@ -502,7 +526,7 @@ battery-linux-sysfs
                                            voltage-now))
 		   (cl-incf energy-now  (* (string-to-number now-string)
                                            voltage-now)))
-		  ((and (progn (goto-char (point-min)) t)
+                  ((and (goto-char (point-min))
 			(re-search-forward
 			 "POWER_SUPPLY_ENERGY_FULL=\\([0-9]*\\)$" nil t)
 			(setq full-string (match-string 1))
@@ -511,7 +535,6 @@ battery-linux-sysfs
 			(setq now-string (match-string 1)))
 		   (cl-incf energy-full (string-to-number full-string))
 		   (cl-incf energy-now  (string-to-number now-string)))))
-	  (goto-char (point-min))
 	  (unless (zerop power-now)
 	    (let ((remaining (if (string= charging-state "Discharging")
 				 energy-now
@@ -519,9 +542,9 @@ battery-linux-sysfs
 	      (setq hours (/ remaining power-now)))))))
     (when (and (> energy-full 0) (> energy-now 0))
       (setq percentage-now (/ (* 100 energy-now) energy-full)))
-    (list (cons ?c (cond ((or (> energy-full 0) (> energy-now 0))
-			  (number-to-string (/ energy-now voltage-now)))
-			 (t "N/A")))
+    (list (cons ?c (if (or (> energy-full 0) (> energy-now 0))
+                       (number-to-string (/ energy-now voltage-now))
+                     "N/A"))
 	  (cons ?r (if (> power-now 0.0)
 		       (format "%.1f" (/ power-now 1000000.0))
 		     "N/A"))
@@ -532,27 +555,20 @@ battery-linux-sysfs
 		     "N/A"))
 	  (cons ?d (or temperature "N/A"))
 	  (cons ?B (or charging-state "N/A"))
-	  (cons ?b (or (and (string= charging-state "Charging") "+")
-		       (and percentage-now (< percentage-now battery-load-critical) "!")
-		       (and percentage-now (< percentage-now battery-load-low) "-")
-		       ""))
-	  (cons ?p (cond
-                    ((and percentage-now (format "%.1f" percentage-now)))
-                    (t "N/A")))
-	  (cons ?L (cond
-                    ((battery-search-for-one-match-in-files
-                      (list "/sys/class/power_supply/AC/online"
-                            "/sys/class/power_supply/ACAD/online"
-                            "/sys/class/power_supply/ADP1/online")
-                      "1" 0)
-                     "AC")
-                    ((battery-search-for-one-match-in-files
-                      (list "/sys/class/power_supply/AC/online"
-                            "/sys/class/power_supply/ACAD/online"
-                            "/sys/class/power_supply/ADP1/online")
-                      "0" 0)
-                     "BAT")
-                    (t "N/A"))))))
+          (cons ?b (cond ((string= charging-state "Charging") "+")
+                         ((not percentage-now) "")
+                         ((< percentage-now battery-load-critical) "!")
+                         ((< percentage-now battery-load-low) "-")
+                         ("")))
+          (cons ?p (if percentage-now (format "%.1f" percentage-now) "N/A"))
+          (cons ?L (pcase (battery-search-for-one-match-in-files
+                           '("/sys/class/power_supply/AC/online"
+                             "/sys/class/power_supply/ACAD/online"
+                             "/sys/class/power_supply/ADP1/online")
+                           (rx (in "01")) 0)
+                     ("0" "BAT")
+                     ("1" "AC")
+                     (_ "N/A"))))))
 
 
 ;;; `upowerd' interface.
@@ -675,19 +691,20 @@ battery-upower
 
 
 ;;; `apm' interface for BSD.
+
 (defun battery-bsd-apm ()
   "Get APM status information from BSD apm binary.
 The following %-sequences are provided:
+%P Advanced power saving mode state (verbose)
 %L AC line status (verbose)
 %B Battery status (verbose)
 %b Battery status, empty means high, `-' means low,
- `!' means critical, and `+' means charging
-%P Advanced power saving mode state (verbose)
-%p Battery charge percentage
-%s Remaining battery charge time in seconds
-%m Remaining battery charge time in minutes
-%h Remaining battery charge time in hours
-%t Remaining battery charge time in the form `h:min'"
+   `!' means critical, and `+' means charging
+%p Battery load percentage
+%s Remaining time (to charge or discharge) in seconds
+%m Remaining time (to charge or discharge) in minutes
+%h Remaining time (to charge or discharge) in hours
+%t Remaining time (to charge or discharge) in the form `h:min'"
   (let* ((os-name (car (split-string
                         ;; FIXME: Can't we use something like `system-type'?
                         (shell-command-to-string "/usr/bin/uname"))))
@@ -753,7 +770,7 @@ battery-bsd-apm
 	(setq seconds (string-to-number battery-life)
 	      minutes (truncate seconds 60)))
       (setq hours (truncate minutes 60)
-	    remaining-time (format "%d:%02d" hours (mod minutes 60))))
+            remaining-time (format "%d:%02d" hours (% minutes 60))))
     (list (cons ?L (or line-status "N/A"))
 	  (cons ?B (or (car battery-status) "N/A"))
 	  (cons ?b (or (cdr battery-status) "N/A"))
@@ -761,9 +778,9 @@ battery-bsd-apm
 		       "N/A"
 		     battery-percentage))
 	  (cons ?P (or apm-mode "N/A"))
-	  (cons ?s (or (and seconds (number-to-string seconds)) "N/A"))
-	  (cons ?m (or (and minutes (number-to-string minutes)) "N/A"))
-	  (cons ?h (or (and hours (number-to-string hours)) "N/A"))
+          (cons ?s (if seconds (number-to-string seconds) "N/A"))
+          (cons ?m (if minutes (number-to-string minutes) "N/A"))
+          (cons ?h (if hours (number-to-string hours) "N/A"))
 	  (cons ?t (or remaining-time "N/A")))))
 
 
@@ -778,21 +795,25 @@ battery-pmset
 %b Battery status, empty means high, `-' means low,
    `!' means critical, and `+' means charging
 %p Battery load percentage
-%h Remaining time in hours
-%m Remaining time in minutes
-%t Remaining time in the form `h:min'"
-  (let (power-source load-percentage battery-status battery-status-symbol
-	remaining-time hours minutes)
+%m Remaining time (to charge or discharge) in minutes
+%h Remaining time (to charge or discharge) in hours
+%t Remaining time (to charge or discharge) in the form `h:min'"
+  (let ( power-source load-percentage battery-status battery-status-symbol
+         remaining-time hours minutes )
     (with-temp-buffer
       (ignore-errors (call-process "pmset" nil t nil "-g" "ps"))
       (goto-char (point-min))
-      (when (re-search-forward "\\(?:Currentl?y\\|Now\\) drawing from '\\(AC\\|Battery\\) Power'" nil t)
+      (when (re-search-forward ;; Handle old typo in output.
+             "\\(?:Currentl?y\\|Now\\) drawing from '\\(AC\\|Battery\\) Power'"
+             nil t)
 	(setq power-source (match-string 1))
-	(when (re-search-forward "^ -InternalBattery-0\\([ \t]+(id=[0-9]+)\\)*[ \t]+" nil t)
+        (when (re-search-forward (rx bol " -InternalBattery-0" (+ space)
+                                     (* "(id=" (+ digit) ")" (+ space)))
+                                 nil t)
 	  (when (looking-at "\\([0-9]\\{1,3\\}\\)%")
 	    (setq load-percentage (match-string 1))
 	    (goto-char (match-end 0))
-	    (cond ((looking-at "; charging")
+            (cond ((looking-at-p "; charging")
 		   (setq battery-status "charging"
 			 battery-status-symbol "+"))
 		  ((< (string-to-number load-percentage) battery-load-critical)
diff --git a/lisp/emacs-lisp/rx.el b/lisp/emacs-lisp/rx.el
index aa4b2addd4..88bb0a8bd6 100644
--- a/lisp/emacs-lisp/rx.el
+++ b/lisp/emacs-lisp/rx.el
@@ -1381,7 +1381,7 @@ rx-define
 For more details, see Info node `(elisp) Extending Rx'.
 
 \(fn NAME [(ARGS...)] RX)"
-  (declare (indent 1))
+  (declare (indent defun))
   `(eval-and-compile
      (put ',name 'rx-definition ',(rx--make-binding name definition))
      ',name))
diff --git a/test/lisp/battery-tests.el b/test/lisp/battery-tests.el
index 052ae49a80..e5706807cc 100644
--- a/test/lisp/battery-tests.el
+++ b/test/lisp/battery-tests.el
@@ -22,9 +22,9 @@
 (require 'battery)
 
 (ert-deftest battery-linux-proc-apm-regexp ()
-  "Test `battery-linux-proc-apm-regexp'."
+  "Test `rx' definition `battery--linux-proc-apm'."
   (let ((str "1.16 1.2 0x07 0x01 0xff 0x80 -1% -1 ?"))
-    (should (string-match battery-linux-proc-apm-regexp str))
+    (should (string-match (rx battery--linux-proc-apm) str))
     (should (equal (match-string 0 str) str))
     (should (equal (match-string 1 str) "1.16"))
     (should (equal (match-string 2 str) "1.2"))
@@ -36,7 +36,7 @@ battery-linux-proc-apm-regexp
     (should (equal (match-string 8 str) "-1"))
     (should (equal (match-string 9 str) "?")))
   (let ((str "1.16 1.2 0x03 0x00 0x00 0x01 99% 1792 min"))
-    (should (string-match battery-linux-proc-apm-regexp str))
+    (should (string-match (rx battery--linux-proc-apm) str))
     (should (equal (match-string 0 str) str))
     (should (equal (match-string 1 str) "1.16"))
     (should (equal (match-string 2 str) "1.2"))
@@ -48,6 +48,39 @@ battery-linux-proc-apm-regexp
     (should (equal (match-string 8 str) "1792"))
     (should (equal (match-string 9 str) "min"))))
 
+(ert-deftest battery-acpi-rate-regexp ()
+  "Test `rx' definition `battery--acpi-rate'."
+  (let ((str "01 mA"))
+    (should (string-match (rx (battery--acpi-rate)) str))
+    (should (equal (match-string 0 str) str))
+    (should (equal (match-string 1 str) "01"))
+    (should (equal (match-string 2 str) "mA")))
+  (let ((str "23 mW"))
+    (should (string-match (rx (battery--acpi-rate)) str))
+    (should (equal (match-string 0 str) str))
+    (should (equal (match-string 1 str) "23"))
+    (should (equal (match-string 2 str) "mW")))
+  (let ((str "23 mWh"))
+    (should (string-match (rx (battery--acpi-rate)) str))
+    (should (equal (match-string 0 str) "23 mW"))
+    (should (equal (match-string 1 str) "23"))
+    (should (equal (match-string 2 str) "mW")))
+  (should-not (string-match (rx (battery--acpi-rate) eos) "45 mWh")))
+
+(ert-deftest battery-acpi-capacity-regexp ()
+  "Test `rx' definition `battery--acpi-capacity'."
+  (let ((str "01 mAh"))
+    (should (string-match (rx battery--acpi-capacity) str))
+    (should (equal (match-string 0 str) str))
+    (should (equal (match-string 1 str) "01"))
+    (should (equal (match-string 2 str) "mAh")))
+  (let ((str "23 mWh"))
+    (should (string-match (rx battery--acpi-capacity) str))
+    (should (equal (match-string 0 str) str))
+    (should (equal (match-string 1 str) "23"))
+    (should (equal (match-string 2 str) "mWh")))
+  (should-not (string-match (rx battery--acpi-capacity eos) "45 mW")))
+
 (ert-deftest battery-format ()
   "Test `battery-format'."
   (should (equal (battery-format "" ()) ""))
-- 
2.26.2