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

[Peter Selinger]

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?

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.

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.

-- Peter

Alexander I. Gordeev wrote:
> 
> Jon, Carlos,
> 
> I've tried to explain what I have achieved but I see that you haven't
> got it. Let's try again.
> (Sorry, if all this is because of my bad English, I'm not a native
> English-speaker.)
> 
> Jon wrote::
> >     Here is a level 4 dump of the output I currently get. This
> > shows that apart from the first call which is to the comm_usb_recv
> > without calling the comm_usb_send first, most of it is working. The
> > functions I have not implemented end up doing what the original code
> > did, ie set_report, get_interrupt.
> 
> Jon wrote:
> >     I am using Serial-Over-USB to drive my UPS. I have taken the
> > work that was done for the AGILER interface and added the ABLEREX
> > interface. However, I could not get the 'get_interrupt' command to
> > work. It would not return any useful data. So I had to change the
> > 'set_data code' to issue 'usb_get_string_simple' and translate the
> > character requests into index numbers. The 'get_data' just reads a
> > static variable that is set by the 'set_data'. In this way I can get
> > most of the functionality working.
> 
> 1. I've asked you to test the patch I supplied without any change,
> because
> >  Our devices seem to be mostly identical!
> That means that only one _subdriver_ is needed. So the functions
> get_data_krauler/set_data_krauler are ready to work with your UPS too.
> I see no point in writing this subdriver once again, as you did. They
> are just quite similar, really.
> 
> 2. Also I don't get why have you prefered 2 month old patch,
> which won't compile against current trunc, as the base for your work.
> I've reworked it, as I wrote about a week ago. What I did:
>  - renamed all the functions like in serial.c. So now you don't need
>  to change megatec.c in any way. If you don't understand why I did it,
>  please refer to the discussion that took place in December.
>  - fixed some bugs from the original patch. And, I believe, didn't
>  introduced new ones. I've checked everything hard. However, not all
>  the bugs (that original code has) are fixed yes. There are at least 2
>  of them yet. I'll work on them.
>  - wrote a new subdriver to support my own UPS. Again, I'm sure, it
>  _will_ support somehow your device. I thought that you could figure
>  it out.
> 
> One thing I haven't done: UPS commands. I had to stop adding features
> after I met the bug I've already written about yesterday:
> > ...
> > Trying to match device
> > Device matches
> > HID descriptor, method 1: (9 bytes) => 09 21 00 01 00 01 22 70 02
> > HID descriptor, method 2: (9 bytes) => 09 21 00 01 00 01 22 70 02
> > HID descriptor retrieved (Reportlen = 624)
> > Unable to get Report descriptor (-88): Invalid or incomplete multibyte or>  wide character
> > No appropriate HID device found
> 
> I guess, this bug is specific to my UPS. But I don't know it for sure
> :( So, please, test my code without changes and send all output to me.
> It will be really great.
> 
> Of course, I'll test your code too. Maybe, it will stop the bug...
> I'm looking forward for the patch.
> 
> Please, tell me if you have any questions...
> 
> Carlos wrote:
> > The idea is to keep megatec unmodified, and up until now a simple
> > USB communications layer with the same API as the serial layer would
> > make this possible. The only catch was the need to add "subdrivers"
> > to support different models, but otherwise this layer could even
> > prove useful for protocols other than megatec. 
> 
> > The original agiler interface by Andrey Lelikov modifies
> > "megatec.c" to add that USB layer, but I think it should be just an
> > outside file with the same API as the serial layer, and linked at
> > compile time to generate an ablerex specific driver, even if it is megate> c specific.
> 
> > Then 1) would be "serial.c", 2) "megatec-transport-usb.c", 3) "megatec-tr> ansport-ablerec.c"
> 
> > But don't worry about that just now. It is more important to see it
> > working, this refactoring can be done later. 
> 
> > When I have some free time I'll check which one of these transport
> > mechanisms my own UPS uses. Then I'll be able to help in refactoring
> > whatever code we have by that time.
> 
> Well, I've done this refactoring.
> Sorry, if you've received this message twice!
> 
> -- 
>  Alexander                          mailto:lasaine at lvk.cs.msu.su