Nut upsdev - Jun 2006 - RFC: allow HID subdriver's to register ups.conf settings

Hi

I'd like to know what you guys think to the following idea.

Please be aware, C/C++ are not exactly my strongpoint (couldn't be much 
further from it actually..!), so I'm not sure whether this is all legal 
code.

--

SYNOPSIS:

Allow a newhidups subdriver to register that it can handle settings in 
ups.conf (and via -x on the command line), as per the normal serial drivers.

DESCRIPTION OF CHANGES:

- In newhidups.h, add an extra line to the subdriver_s struct;

                void (*extendvartable)(void);

  so it becomes;

        struct subdriver_s {
                char *name;
                int (*claim)(HIDDevice *hd);
                usage_tables_t *utab;
                hid_info_t *hid2nut;
                int (*shutdown)(int ondelay, int offdelay);
                char *(*format_model)(HIDDevice *hd);
                char *(*format_mfr)(HIDDevice *hd);
                char *(*format_serial)(HIDDevice *hd);
                void (*extendvartable)(void);
        };

- In the subdriver source, add an extra function, 
subdriver_extendvartable(). I'm currently
  working on belkin-hid.c, so it would have;

        void belkin_extendvartable(void) {
                addvar(VAR_FLAG, "wait", "Wait for AC power
          ");
                addvar(VAR_VALUE,"wait", "Wait for AC power and
battery
level ");
                /* ... */
        }

  The other HID drivers, I guess, could simply have empty functions at the 
moment.

  Next, modify the subdriver_t chunk (again, this is shown for the 
belkin-hid) to be;

        subdriver_t belkin_subdriver = {
                BELKIN_HID_VERSION,
                belkin_claim,
                belkin_utab,
                belkin_hid2nut,
                belkin_shutdown,
                belkin_format_model,
                belkin_format_mfr,
                belkin_format_serial,
                belkin_extendvartable,
        };

- In newhidups.c, at the end of upsdrv_makevartable(), add a single line 
that does:

                subdriver->extendvartable();

  to actually extend the table of configuration options for the specific 
subdriver.

--

Assuming that this sounds like a good idea, I also have a query;

With respect to the subdriver_s struct, would it be better to have the
"void
(*extendvartable) (void);" line at the end of the struct (as shown above), 
or would some other position within the struct be more appropriate???

Thanks

Jo Turner
  -)O(-

Hi Jo,

this sounds reasonable, but I would change it as follows:

- call the function "init" instead of "extendvartable" -
  this could be used by subdrivers to do any necessary
  initializations. 

- it doesn't matter where in the struct it goes; the end is fine.

- allow the value to be NULL if a subdriver does not require
  initialization.

- change the call to:

  if (subdriver->init != NULL) {
     subdriver->init();
  }

-- Peter

j T wrote:
> 
> Hi
> 
> I'd like to know what you guys think to the following idea.
> 
> Please be aware, C/C++ are not exactly my strongpoint (couldn't be much
> further from it actually..!), so I'm not sure whether this is all legal
> code.
> 
> --
> 
> SYNOPSIS:
> 
> Allow a newhidups subdriver to register that it can handle settings in 
> ups.conf (and via -x on the command line), as per the normal serial
drivers.
> 
> DESCRIPTION OF CHANGES:
> 
> - In newhidups.h, add an extra line to the subdriver_s struct;
> 
>                 void (*extendvartable)(void);
> 
>   so it becomes;
> 
>         struct subdriver_s {
>                 char *name;
>                 int (*claim)(HIDDevice *hd);
>                 usage_tables_t *utab;
>                 hid_info_t *hid2nut;
>                 int (*shutdown)(int ondelay, int offdelay);
>                 char *(*format_model)(HIDDevice *hd);
>                 char *(*format_mfr)(HIDDevice *hd);
>                 char *(*format_serial)(HIDDevice *hd);
>                 void (*extendvartable)(void);
>         };
> 
> - In the subdriver source, add an extra function, 
> subdriver_extendvartable(). I'm currently
>   working on belkin-hid.c, so it would have;
> 
>         void belkin_extendvartable(void) {
>                 addvar(VAR_FLAG, "wait", "Wait for AC power
>           ");
>                 addvar(VAR_VALUE,"wait", "Wait for AC power
and battery
> level ");
>                 /* ... */
>         }
> 
>   The other HID drivers, I guess, could simply have empty functions at the 
> moment.
> 
>   Next, modify the subdriver_t chunk (again, this is shown for the 
> belkin-hid) to be;
> 
>         subdriver_t belkin_subdriver = {
>                 BELKIN_HID_VERSION,
>                 belkin_claim,
>                 belkin_utab,
>                 belkin_hid2nut,
>                 belkin_shutdown,
>                 belkin_format_model,
>                 belkin_format_mfr,
>                 belkin_format_serial,
>                 belkin_extendvartable,
>         };
> 
> - In newhidups.c, at the end of upsdrv_makevartable(), add a single line 
> that does:
> 
>                 subdriver->extendvartable();
> 
>   to actually extend the table of configuration options for the specific 
> subdriver.
> 
> --
> 
> Assuming that this sounds like a good idea, I also have a query;
> 
> With respect to the subdriver_s struct, would it be better to have the
"void
> (*extendvartable) (void);" line at the end of the struct (as shown
above),
> or would some other position within the struct be more appropriate???
> 
> Thanks
> 
> Jo Turner
>   -)O(-
> 
> 
> 
> _______________________________________________
> Nut-upsdev mailing list
> Nut-upsdev@lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
>