apcmicrolink: async USB reads, session stability, page 0 fix

NEWS.adoc

@@ -89,6 +89,67 @@ https://github.com/networkupstools/nut/milestone/13
       (`outlet.group.N.load.*`, `outlet.group.N.shutdown.*`), and the
       `outlet.group.N.switchable` variable to distinguish switchable
       groups from an unswitched main bank.
+    * USB mode: reworked the interrupt-IN read path to use a permanently
+      outstanding asynchronous transfer serviced by a dedicated thread, so
+      device replies are no longer missed while the driver is between
+      polls. Several long-run stability fixes go with it: a USB bus reset
+      is now triggered only on a genuine disconnect rather than on a retry
+      count, the cached page 0 contents are no longer wiped on every
+      session (re)start, the read queue is no longer flushed on every
+      session retry, and the authentication challenge is randomized.
+      Also warns once when built against the untested libusb-0.1 backend,
+      and backs off instead of spinning when the kernel's `usbhid` driver
+      has reclaimed the interface.
+    * Mapped two further descriptor usages:
+      `experimental.battery.serial` (the battery pack's own serial number,
+      distinct from `ups.serial`) and `microlink.diag.slave_password_echo`
+      (auth troubleshooting visibility only, not proof that
+      authentication was accepted).
+    * A page 0 whose announced frame width disagrees with the width of
+      the frame that carried it is now rejected instead of cached. Such
+      a page redefined the length the frame parser demanded, after which
+      nothing checksum-validated again and the session could not
+      recover. Affects both transports. [issue #3587]
+
+ - Contributed `apcmicrolink` driver updates [PR #3591]. The driver
+   remains experimental, but USB sessions should hold up better, and
+   fall back more readily to standard HID PDC data when the Microlink
+   tunnel is unstable or unavailable:
+    * USB mode: the standard-HID-PDC fallback now engages whenever the
+      Microlink tunnel stops producing usable data, not only when it
+      fails to answer during startup. A device that answers every poll
+      while reporting an all-zero state previously left `ups.status`
+      empty indefinitely. The driver hands back to Microlink data once
+      that source looks plausible again, with hysteresis so the two do
+      not swap on alternate polls.
+    * The Microlink tunnel is now polled for a full page pass per update
+      cycle, instead of one record per `pollinterval`. The device does
+      not acknowledge its authentication handshake until roughly 20
+      exchanges after the response is sent, and serves no measurement
+      page until it does, so the slower cadence could leave every
+      measured value reading 0 on a device that was answering normally.
+      Publishes `experimental.microlink.diag.auth_status` and
+      `experimental.microlink.diag.auth_refused`, and warns when the
+      handshake goes unacknowledged.
+    * The driver now closes the Microlink session with a STOP when it
+      exits. Without it the device could answer a later client's session
+      request mid-walk rather than at page 0, and would not re-synchronize
+      until re-enumerated.
+    * `2:4.7.28` and `2:4.7.49` are percentages of the nominal ratings
+      rather than absolute readings. `ups.load` is now published from the
+      former, with `ups.realpower` and `ups.power` derived against the
+      nominal ratings, instead of both reading two orders of magnitude
+      low. `ups.test.result` now comes from the battery-scope usage that
+      a self test actually updates, and `ups.test.interval` is derived
+      from the self-test schedule enumeration.
+    * Newly mapped: `input.sensitivity`, `ups.beeper.status`,
+      `experimental.battery.firmware` and
+      `experimental.statistics.battery.transfers`.
+    * Driver-specific diagnostic variables moved from `microlink.*` to
+      `experimental.microlink.*`.
+    * Documented the Microlink descriptor format that the driver parses,
+      for anyone extending its variable maps. See the NUT Developer
+      Guide.
 
  - `apc_modbus` driver updates:
     * Fixed string join not doing zero termination. [PR #3413]

docs/Makefile.am

@@ -111,7 +111,8 @@ USER_MANUAL_DEPS = acknowledgements.txt cables.txt config-notes.txt	\
 
 # See also conversions included via FULL_DEVELOPER_GUIDE_DEPS
 DEVELOPER_GUIDE_DEPS = contact-closure.txt design.txt developers.txt	\
- developer-guide.txt hid-subdrivers.txt macros.txt new-clients.txt	\
+ apcmicrolink-descriptors.txt developer-guide.txt hid-subdrivers.txt	\
+ macros.txt new-clients.txt	\
  new-drivers.txt net-protocol.txt nutdrv_qx-subdrivers.txt	\
  nut-versioning.adoc snmp-subdrivers.txt sock-protocol.txt
 

docs/apcmicrolink-descriptors.txt

@@ -0,0 +1,138 @@
+Reading APC Microlink descriptors
+---------------------------------
+
+//////////////////////////////////////////////////////////////////////////////
+// This chapter documents the on-the-wire structure the "apcmicrolink" driver
+// parses, for anyone extending its variable maps to cover more of a device.
+//////////////////////////////////////////////////////////////////////////////
+
+Overall concept
+~~~~~~~~~~~~~~~
+
+A Microlink device publishes a machine-readable *descriptor* alongside its
+data, so the driver learns where each value lives instead of hard-coding
+offsets. `drivers/apcmicrolink-maps.c` therefore maps *descriptor paths* to NUT
+variable names, and the driver resolves those paths to byte offsets at run
+time.
+
+Everything below was derived from a live APC SCL500RMI1UC. Other models share
+the structure but not necessarily the same set of usages.
+
+Frames, pages and the blob
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+A record is `[page id][width data bytes][2 byte checksum]`. The `width` comes
+from page 0, along with the page count, so a device with `width = 32` and
+96 pages has 35-byte records and a 3072-byte address space.
+
+The driver assembles those pages into one flat blob by laying page N at byte
+offset `N * width`. Page 0 also carries a *descriptor table offset* (byte 12 on
+the device above) and a pointer to where the *data region* begins. Everything
+before that pointer is the descriptor program; everything after it is live
+data. Do not assume the descriptor comes second - on the SCL500RMI1UC the
+descriptor occupies pages `0x00`-`0x3F` and data starts at page `0x40`.
+
+Path syntax
+~~~~~~~~~~~
+
+Paths look like SNMP OIDs and behave like them:
+
+    <object> : <collection> . <sub-collection> . <attribute>
+
+for example `2:4.5.9.40` - object `2`, collection `4.5`, sub-collection `9`,
+attribute `40`. Components are hexadecimal without a prefix.
+
+Attribute IDs are scope-relative
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This is the single most useful property when identifying an unknown usage:
+*within one top-level object*, a trailing attribute ID means the same thing on
+whichever collection carries it.
+
+[options="header"]
+|===============================================================================
+| Attribute | Seen at                                              | Meaning
+| `.F.69`   | `2:4.5`, `2:4.6`, `2:4.7`, `2:4`                     | total time
+| `.40`     | `2:4.9`, `2:4.5.9`                                   | serial number
+| `.42`     | `2:4.9`, `2:4.5`                                     | part number
+| `.4A`     | `2:4.5.9`                                            | firmware revision
+| `.19`     | `2:4.9`, `2:4.5`                                     | a date
+| `.82`     | `2:4`, `2:4.3E`                                      | name
+| `.31`     | `2:4.5`, `2:4.3E`                                    | low runtime warning
+| `.25`     | `2:4.6`, `2:4.7`                                     | voltage
+| `.27`     | `2:4.6`, `2:4.7`                                     | frequency
+| `.11`     | `2`, `2:4.5`                                         | test result
+| `.13`     | `2`, `2:4.5`                                         | calibration result
+|===============================================================================
+
+Known collections under object `2` (the UPS itself):
+
+    2:4.5    battery          2:4.9    inventory
+    2:4.6    input            2:4.B    user interface
+    2:4.7    output / power   2:4.3E   outlet group
+    2:4.8    authentication   2:4.F    statistics
+
+`9` (inventory) and `F` (statistics) nest inside other collections, which is
+why `2:4.5.9.40` is the battery serial and `2:4.6.F.69` the input total time.
+
+WARNING: Top-level objects have *independent* attribute namespaces. Do not
+carry a meaning from object `2` into object `3`: on the same device `3:22` is a
+temperature threshold rather than a temperature, `3:25` is input sensitivity
+rather than a voltage, and `3:26` is accumulated energy rather than a current.
+Object `2` is the UPS device tree and `3` appears to be a system or controller
+object; `4`, `5` and `A` are not yet identified.
+
+Two scopes, one NUT name
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+Because the same attribute exists at several scopes, it is easy to map two
+usages onto one NUT variable by accident. When that happens the winner depends
+on the order usages appear in the descriptor, not on the order of entries in
+the map - so the result varies by device and is not reproducible.
+
+Where a device may populate either scope, map the specific one and promote the
+general one only when the specific is absent. `microlink_publish_test_result()`
+does this for `ups.test.result`: `2:4.5.11` is mapped directly, and `2:11` is
+published under an experimental name and copied over only if the battery-scope
+usage is missing entirely.
+
+Identifying an unknown usage
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+In rough order of cost:
+
+1. *Infer from the attribute ID.* If the trailing component is already known at
+   another scope in the same object, that is very likely its meaning. Costs
+   nothing.
+
+2. *Diff a reference client's writes.* Change one setting in the vendor's own
+   software while capturing the bus, then look for host-to-device frames that
+   are not the plain control bytes. A write frame names its page and offset
+   directly, so it identifies the usage with no guesswork - and proves the
+   field is writable at the same time. This is far more reliable than matching
+   values by coincidence.
+
+3. *Cross-reference an SNMP agent.* Vendor management software often ships an
+   SNMP agent serving a documented MIB. Walking it gives named, described
+   values that can be matched against the data pages, which is useful for
+   read-only fields that step 2 cannot reach.
+
+Mapping pitfalls
+~~~~~~~~~~~~~~~~
+
+*Check the units `docs/nut-names.txt` states, not just the name.* Several
+usages are enumerations whose NUT counterpart is numeric. `ups.test.interval`
+is defined as seconds, but the device reports a schedule enum - two of whose
+members ("never", "on start-up only") have no interval at all. Publishing the
+enum string there would break any client doing arithmetic on it.
+
+*Beware percentages reported as if they were absolute.* A fixed-point field
+that reads roughly a hundred times too small is more likely a percentage of a
+nominal rating than a wrong binary point - a wrong binary point is off by a
+power of two, never by 5. Cross-multiplying independent readings catches this
+quickly: output current times output voltage should agree with apparent power.
+
+*Descriptor usages can be marked skipped.* Skipped usages are published neither
+under a name nor in the `unmapped` listing, so a value visible in a page dump
+may have no corresponding variable at all. Check the skip flag before assuming
+a mapping is missing.

docs/man/apcmicrolink.txt

@@ -202,9 +202,28 @@ starts up anyway using those for `ups.status`/`battery.charge`/
 `battery.runtime` (see the `hid_fallback` option above) instead of
 refusing to start. Outlet-group data and instant commands, which depend
 on the full Microlink descriptor, become available automatically once the
-tunnel connects -- no driver restart is needed. If the tunnel stays
-unresponsive, the driver periodically attempts a USB device reset to try
-to recover it.
+tunnel connects, with no driver restart needed. If the device disappears
+from the bus (unplug, power cycle), the driver resets and reopens it. It
+does not reset a device that is still present but not answering the
+tunnel, because that does not help.
+
+libusb backend
+~~~~~~~~~~~~~~
+
+USB mode has only been tested against libusb-1.0. Builds against
+libusb-0.1 log a warning once at startup and then carry on, but that
+path is untested for this driver. On at least one system the installed
+libusb-0.1 library's interrupt IN read ignored its configured timeout,
+so the driver hung waiting for a reply instead of timing out and
+retrying. If USB mode stops responding, check that NUT was built with
+`--with-usb=libusb-1.0` before looking further.
+
+With libusb-1.0 and pthreads the driver keeps one read request
+outstanding on a dedicated thread, so it picks up a reply whenever the
+device sends one. Without both, it reads only while already waiting for
+a reply. The tested device can take several seconds to answer and pushes
+unrelated reports on the same pipe meanwhile, so replies that arrive
+outside that window are missed.
 
 Kernel usbhid conflict
 ~~~~~~~~~~~~~~~~~~~~~~
@@ -216,7 +235,9 @@ cannot claim it and will fail to start; and if `usbhid` reclaims the
 interface after a USB reset or re-enumeration event *while this driver is
 already running* (observed on the tested hardware), every following
 interrupt transfer this driver attempts will be rejected by the kernel
-until the driver process is restarted.
+until the driver process is restarted. The driver detects this, logs a
+warning, and backs off between attempts instead of retrying at full
+speed. It cannot recover on its own.
 
 Neither this driver nor NUT's shared USB layer (`nut_libusb.c`) attempts
 to detach `usbhid` on its own. A udev rule that unbinds `usbhid` from the

docs/new-drivers.txt

@@ -981,3 +981,7 @@ include::snmp-subdrivers.txt[]
 [[nutdrv_qx-subdrivers]]
 
 include::nutdrv_qx-subdrivers.txt[]
+
+[[apcmicrolink-descriptors]]
+
+include::apcmicrolink-descriptors.txt[]