[Nut-upsdev] Re: Tr : NUT patches

[Peter Selinger]

From: arnaud.quette at mgeups.com
> 
> Side notes:
> -  there are few new variables that need to be added to the
> naming scheme. These are consistant with the current naming,
> so no problem. But we need to document these a bit:
> Name / Description / Typical value

Here are the variables that I have added, that were not already in
docs/new-names.txt:

* battery.charge.warning / Battery level when UPS switches to "Warning" / 50

  This is similar to battery.charge.low. I don't actually know what happens
  when this warning level is reached; perhaps it is just informational.

* battery.mfr.date / Date when the battery was manufactured / 2005/04/02

  This is similar to ups.mfr.date.

* ups.beeper.status / State of the beeper: enabled, disabled, or muted / enabled

  The belkinunv driver uses ups.beeper.enable, with values "yes" and
  "no", but the USB-HID Usage table for power devices actually
  specifies three states for a generic UPS beeper (see usage
  0084005a, AudibleAlarmControl):

    "Read or Write value:
      1: Disabled (Never sound)
      2: Enabled (Sound when an alarm is present)
      3: Muted (Temporarily silence the alarm)
    This is the requested state (Write value) or the present state (Read
    value) of the audible alarm. The Muted state (3) persists until the
    alarm would normally stop sounding. At the end of this period the
    value reverts to Enabled (2). Writing the value Muted (3) when the
    audible alarm is not sounding is accepted but otherwise has no effect."

  I therefore recommend that this variable be standardized. In fact,
  it should be writable, but I don't know how to do this.

* ups.test.panel / Panel test / 0

  My APC UPS has a "panel test" variable, which consists of a single
  R/W bit. In write mode: setting this to 1 causes the UPS to omit one
  long beep. Setting this to 0 has no audible or visible effect. In
  read mode: simply returns the value of the last "write". I surmise
  that this variable indicates that a test is "in progess", but I am
  not sure of the test ever stops or does anything. Since NUT has
  instant commands "test.panel.start" and "test.panel.stop", I figured
  it might be worth having an associated variable as well. But it
  probably cannot be given an interesting meaning until we find other
  APC models with more interesting panels (mine has no LED's, so there
  is not much to test except the beeper). In particular, a panel test
  does not seem to have any interesting "result" that is reported.

> - I'll forward the PARSER patch, if accepted, to the libhid project
> (the project I've launched for the long run hid UPS support,
> and more generally for the opensource hid support) as the
> code base is the same (MGE HID Parser + libusb + wrapping functions)

Good. This bug is quite serious. For example, look at ALfred Ganz's
post at
http://lists.alioth.debian.org/pipermail/nut-upsdev/2005-August/000015.html
- it shows the same mangled Usage tree that I got. 

> - the APC shutdown method should be matched against
> the apcupsd method as they have already dealed with this:
> http://cvs.sourceforge.net/viewcvs.py/apcupsd/apcupsd/src/drivers/usb/usb.c?rev=1.17&view=markup
> => look for usb_ups_kill_power()

My change in this function is just a matter of taste. When called with
the "-k" flag, I prefer the UPS to shut down immediately, as this will
typically happen at the end of my shutdown script when the computer is
ready to power off. The method that was already implemented also
works, but has a 60 second delay. The file you reference above also
seems to prefer a delay.

Thanks, -- Peter


> ---------------------- Envoyée par Arnaud QUETTE/FR/MGE-UPS/User/GIN le 25/08/2005 08:56 ---------------------------
> 
> 
> selinger at mathstat.dal.ca (Peter Selinger) le 24/08/2005 14:51:41
> 
> 
> Pour : arnaud.quette at mgeups.com
> cc :
> 
> Objet :     NUT patches
> 
> 
> Dear Arnaud,
> 
> first, thanks for taking over the NUT project. I used to work with
> Russell when I wrote the belkinunv driver.
> 
> My wife recently got an APC Back-UPS ES 650 with a USB connector.
> This gave me an opportunity to fine-tune the newhidups driver a bit.
> I have made several fixes / changes, which I am sending to you as a
> tarball of patches (in a separate email). For your convenience, I have
> separated the changes into a number of different patches that can be
> applied independently. Here are some details of what each patch does.
> 
> The patches named nut-cvs-patch-* apply to the most recent CVS
> development tree.
> 
> The patch named nut-2.0.2-* applies to the stable 2.0.2 tree, and
> combines all the changes of the various nut-cvs-patch-* patches.  I
> have included it for convenience (because there is some fuzz and some
> failed hunks when trying to apply the cvs patches to the stable tree).
> 
> I hope you can use these! Let me know if you have any question.
> -- Peter
> (selinger at users.sourceforge.net)
> 
> 1) nut-cvs-patch-PARSER-2005-08-24
> ----------------------------------------
> 
> This patch fixes a critical bug in the HIDParser. According to the USB
> HID specification, when parsing the report descriptor, local tags
> (such as "usage") are only supposed to apply to the next main item,
> not any subsequent items. Concretely, when parsing the report
> descriptor of my APC UPS, there were some redundant "usage" items that
> would stay on the UsageTab and would mangle the tree by shifting all
> usages by one. An example showing the effect of this bug is contained
> in the file hidparser-example.txt, also contained in the attached
> tarball.
> 
> This patch also fixes a minor bug where the first item of the report
> descriptor was accidentally thrown away.
> 
> This patch also adds the ability to deal with "long items" in report
> descriptors (as per USB HID specification). The items are not
> currently used, but at least they are safely skipped, rather than
> causing the parser to break.
> 
> 2) nut-cvs-patch-APC-2005-08-24
> ----------------------------------------
> 
> This patch fixes/adds driver variables and instant commands for APC.
> I added support for the following driver variables:
> 
>   battery.charge.warning
>   battery.runtime.low
>   ups.load
>   ups.delay.shutdown
>   ups.test.panel
>   ups.beeper.status
>   ups.mfr.date
>   battery.mfr.date
>   battery.date
>   input.voltage.nominal
>   input.transfer.low
>   input.transfer.high
> 
> I also added support for the following instant commands:
> 
>   test.panel.start
>   test.panel.stop
>   load.off
>   shutdown.return
>   shutdown.stop
>   beeper.on
>   beeper.off
> 
> I also extended the info_lkp_t mechanism. I found that for certain
> data types (in particular, dates) a simple lookup table is
> insufficient to map the driver data to a string. I have extended the
> info_lkp_t structure with a hook for a custom function, and extended
> hu_find_infoval() to use the hook if present. This allows dates
> (ups.mfr.date, battery.mfr.date, battery.date) from the APC device to
> be formatted.
> 
> I also made a variant of the function newhidups.c:find_nut_info(),
> called find_nut_info_valid(). This function is used when looking up an
> instant command. Rather than returning the first command in the list,
> it returns the first one that actually works for the current UPS.
> This makes it possible to have multiple alternative definitions for a
> certain instant command that will work on different UPS models.
> 
> I have also changed the behavior of the "-k" flag to try "load.off"
> before "shutdown.return", as this kills the power immediately, instead
> of after a 60 second delay. This change only affects APC models.
> 
> Finally, I corrected some mistakes in APC Usage Paths that were caused
> by the above HIDParser bug.
> 
> 3) nut-cvs-patch-REOPEN-2005-08-24
> ----------------------------------------
> 
> This patch fixes some problems associated with closing and reopening
> devices. I assume that the problems themselves are OS specific, but
> the patch gives solutions that should work correctly for all OSs.
> 
> When reopening a device, this patch will look for a device that
> matches the previous manufacturer, model, and serial
> number. Therefore, if there is more than one UPS attached, one is
> guaranteed to get the same one.
> 
> Also, when I unplug and reattach my UPS, my operating system (Linux
> 2.6.4) attaches a kernel driver to it. The original libusb_open()
> function would return too early when reopening a device, and in
> particular, it would not try to detach any kernel drivers. With this
> patch, reopening is treated in the same way as opening (and works much
> better for me).
> 
> Finally, calling usb_release_interface() often caused my driver
> process to go into uninterruptible sleep and never wake up (it could
> not even be killed with "kill -KILL"). I had to reboot each time to be
> able to attach another driver. I commented out this call, which seems
> to be useless anyway.
> 
> 4) nut-cvs-patch-NUMPATH-2005-08-24
> ----------------------------------------
> 
> This patch extends the function hid_lookup_usage() so that path names
> can also be looked up by numerical values. This is just a convenience
> - it allows one to put stuff like "UPS.PowerConverter.ff860024" in the
> apc-hid.h while testing device variables whose purpose is yet unknown.
> 
> 5) nut-cvs-patch-DEP-2005-08-24
> ----------------------------------------
> 
> This is just a convenience; I added proper dependencies to
> drivers/Makefile.in. I noticed that when I edited a *.h file, make
> would never recompile the appropriate drivers. With this patch, it
> does.
> 
> 6) nut-2.0.2-patch-ALL-2005-08-24
> ----------------------------------------
> 
> While patches 1)-5) are for the CVS development tree, patch 6) is for
> the stable 2.0.2 tree. It combines all the above changes from 1)-5).