[Nut-upsdev] Re: [Nut-upsuser] Ablerex 625L USB version

[Alexander I. Gordeev]

Hi Peter,

I think, you've done everything just fine. Thank you!

Peter wrote:

> Alexander,

> don't worry, I am trying to sort out these patches. Thanks for all of
> your work so far. I have created a branch at:

>   svn://svn.debian.org/svn/nut/branches/megatec-usb

> this currently contains:

> revision 793: Andrey's patch (updated)
> revision 794: Alexander's patch (updated)
> revision 796: Jon's changes merged into Alexander's

> Notes: in revision 794, I have edited your patch slightly to remove
> some unnecessary whitespace changes, to show more clearly the
> differences and similarities to Andrey's patch. For the same reason, I
> have retained the name megatec_usb.c for the source file,
> rather than serial_usb_megatec.c.

> In revision 796, I have taken some of Jon's changes and applied them
> on top of your changes. I did not apply the multitude of extra
> debugging statements that Jon had added. I also did not add some of
> Jon's changes:

> Near line 125:

> -        param_arg.get_data = get_data_agiler;
> -        param_arg.set_data = set_data_agiler;
> +        param_arg.get_data = usb_ups_device->get_data;
> +        param_arg.set_data = usb_ups_device->set_data;

> This didn't make sense to me, because usb_ups_device will be
> uninitialized at this point. 

> Near line 198:

>          if ( (c==endchar) || (c==0) ) {
>              break;
>          }

> +        if (ignset == NULL) break;
>          if (NULL!=strchr(ignset,c)) continue;

> I wasn't sure if this makes sense. Can ignset ever be NULL? And if
> yes, why should there be a "break", rather than just skipping the
> following test?

I guess, it should be something like this:

    if ((ignset == NULL) || (NULL!=strchr(ignset,c))) continue;

because it is safer. If ignset is NULL, strchr causes segmentation
fault. However, if we deal only with megatec.c ignset cannot be NULL.

> It looks indeed like the get/set_data_ablerex and get/set_data_krauler
> functions have some similarities, although they are not quite the
> same. It's probably good if you experiment with each other's code,
> which should be convenient now that it is in SVN.

Sure, it won't hurt.

> Another note: I don't like the matching code (ser_open,
> comm_usb_match). The current logic makes little sense, for example any
> device for which vid:pid is specified as the port will be assumed to
> be an AGILER device.

> There should be a more general mechanism, as in usbhid-ups, using
> options such as -x vendorid=<regex>, -x product=<regex>, -x
> serial=<regex>, etc. The work has already been done in usbhid-ups (see
> upsdrv_makevartable(), upsdrv_initups()), and should be easy to
> duplicate here.

Yes, it should be fixed. Thanks for the solution!

-- 
 Alexander                          mailto:lasaine at lvk.cs.msu.su