[Nut-upsdev] [nut-commits] svn commit r1596 - in trunk: drivers man tools

[Arjen de Korte]

Citeren Charles Lepple <clepple at gmail.com>:

>> Log:
>> Added new richcomm_usb driver based on the lakeview_usb driver  
>> (using libusb).
>
> Sorry if I wasn't clear - I was advocating *renaming* the lakeview_usb
> driver to richcomm_usb.

You were very clear. The reason for adding the richcomm_usb driver  
instead of replacing/renaming the existing lakeview_usb driver, is  
that the latter seems to be tested at some point in time and the first  
is not (yet).

> Is there another reason to keep both in the tree?

Yes. Although I'm fairly confident that the richcomm_usb driver will  
work, I would like to see it tested before dropping the lakeview_usb  
driver.

The reason for rewriting the lakeview_usb driver, is that it is  
duplicating lots of stuff we already have in libusb.c and also  
contains some lines that make me wonder what is meant:

55 	MESSAGE_VALUE, INDEX_VALUE, query, sizeof(query), 1000);

Here query is an argument to the call of this function of type  
(unsigned char *). On a 32-bit system this will boil down to 4, but on  
a 64-bit system most likely 8. This only works correctly if query is  
an array, which is not the case here.

62 	ret = usb_interrupt_read(upsdev, REPLY_REQUESTTYPE, reply,  
sizeof(REPLY_PACKETSIZE), 1000);

Again wrong use of sizeof(). I don't know what the preprocessor will  
make out of 'sizeof(6)', but it most likely is not what was intended.

66 	usb_comm_fail("Receive error (Request command): COMMAND: %x\n", query);

This is a non-portable construction. The (implicit) typecasting from  
(unsigned char *) to (unsigned int) is never pretty anyway.

> I'm pretty sure that
> this device with the Lakeview Research VID is actually made by
> Richcomm, as the earlier links pointed out.

Indeed.

Best regards, Arjen
-- 
Please keep list traffic on the list