[Nut-upsdev] Re: [nut-commits] svn commit r801 - in

[Alexander I. Gordeev]

Sorry, I was extremely busy last 2 weeks...

On Thu, 22 Feb 2007 07:08:46 +0300, Peter Selinger <selinger at mathstat.dal.ca> wrote:

>
> In usbhid-ups, device matching serves several different purposes.
>
> (1) To see which device the user wants to connect to, in case a single
>     machine is connected to several different UPSs. This is important,
>     because developers often have more than one UPS. It is also
>     important for server-type installations with multiple UPS units.
>
> (2) To decide which subdriver to use.
>
> (3) To ensure that we are not connecting to any devices that are not
>     UPSs (hubs, mice, toaster oven), unless specifically requested
>     otherwise by the user.
>
> As far as I see, all three concerns are also valid for megatec_usb.
>
> (A) The default behavior should be to connect to the first device
> found that is known to be supported (i.e., whose productid/vendorid is
> in the list of subdrivers).
>
> (B) However, *if* the user specifies either productid or vendorid,
> then we should connect to the first device that matches the user
> choice (even if it is not in the list of supported devices). This will
> allow users of new devices to experiment without having to change the
> source code. (Very important for daily support on the mailing list).
>
> Behaviors (A) and (B) take care of concerns (1) and (3).
>
> Finally, the choice of (2) is easy in case (A), because we already
> have a list mapping devices to subdrivers. In case of (B), if we match
> a device that is in the list, then we should use that subdriver by
> default, but the user must be able to override this. Finally, if we
> match a device that is not in the list, we must choose a reasonable
> default, and the user must definitely be able to override it.
>

Thanks for the complete definition!

> I thought about adding three options
> -x vendorid
> -x productid
> -x subdriver
>to implement this behavior. (One could also match by vendor name,
> product name, or serial number as in usbhid-ups, or allow regular
> expressions, but that is perhaps not a priority).
>
> The problem with this approach is that the variables need to be
> declared in upsdrv_makevartable(), which is shared between the megatec
> (serial) and megatec_usb drivers. So this part of the code would need
> to be specialized somehow (move it to megatec_usb.c, and make a new
> file megatec_serial.c, containing only serial-specific code).
>
> Another option is to use the existing "port" variable, which is unused
> in USB, so that the user would specify something like ffff:0001:ablerex,
> which each ":" being optional, i.e., valid ports are:
>
> 1234                (vendorid only)
> 1234:5678           (vendorid and productid)
> 1234:5678:subdriver (vendorid, productid, and subdriver name)
>
> The advantage is that it needs no special new options. The
> disadvantage is limited flexibility for future extensions (match by
> serial number etc).
>

I think, the first way is the best. It is:
  - flexible
  - the same way is in the other usb drivers
  - more adequate to the things that go internally (automatic matching
    with some constraints)

But I don't think adding megatec_serial.c is necessary. We avoided such
a file from the beginning.
All variables declared in megatec.c are used only in megatec.c. So we
need them in both megatec and megatec_usb drivers. Also serial.c doesn't
use any variables.
Well, I suggest add a function to both serial.c and megatec_usb.c that
can expand variable table with the variables that the corresponding
driver needs. I.e. I suggest to declare variables used in each file in
this particular file only. The only thing to care about is variable
naming.
Please, look at the diff listing below.

I'd rather use the "port" variable like that:
  - "auto" for usual automatic matching
  - "bus/device" for manual selection
I think the latter would be useful in some cases (debugging; if user has
2 identical UPSes). Just an opinion.



Index: drivers/megatec.c
===================================================================
--- drivers/megatec.c   (revision 839)
+++ drivers/megatec.c   (working copy)
@@ -757,6 +757,9 @@
         addvar(VAR_VALUE, "ondelay", "Delay before UPS startup (minutes)");
         addvar(VAR_VALUE, "offdelay", "Delay before UPS shutdown (minutes)");
         addvar(VAR_VALUE, "battvolts", "Battery voltages (empty:full)");
+
+       /* serial protocol may need more variables */
+       ser_makevartable();
  }


Index: drivers/megatec_usb.c
===================================================================
--- drivers/megatec_usb.c       (revision 839)
+++ drivers/megatec_usb.c       (working copy)
@@ -160,6 +160,16 @@
         return 0;
  }

+void ser_makevartable(void)
+{
+       addvar(VAR_VALUE, "vendor", "Regular expression to match UPS Manufacturer string");
+       addvar(VAR_VALUE, "product", "Regular expression to match UPS Product string");
+       /* addvar(VAR_VALUE, "serial", "Regular expression to match UPS Serial number"); */
+       addvar(VAR_VALUE, "vendorid", "Regular expression to match UPS Manufacturer numerical ID (4 digits hexadecimal)");
+       addvar(VAR_VALUE, "productid", "Regular expression to match UPS Product numerical ID (4 digits hexadecimal)");
+       addvar(VAR_VALUE, "bus", "Regular expression to match USB bus name");
+}
+
  int ser_close(int fd, const char *port)
  {
         usb->close(udev);
Index: drivers/serial.c
===================================================================
--- drivers/serial.c    (revision 839)
+++ drivers/serial.c    (working copy)
@@ -170,6 +170,10 @@
         return 0;
  }

+void ser_makevartable(void)
+{
+}
+
  int ser_close(int fd, const char *port)
  {
         if (fd < 0)
Index: drivers/serial.h
===================================================================
--- drivers/serial.h    (revision 839)
+++ drivers/serial.h    (working copy)
@@ -19,6 +19,8 @@

  int ser_set_speed(int fd, const char *port, speed_t speed);

+void ser_makevartable(void);
+
  int ser_close(int fd, const char *port);

  int ser_send_char(int fd, unsigned char ch);

-- 
   Alexander