[abratchik]

Fixes issue #985

This pull request enables support of USB HID-compliant UPS, which are composite devices. Example - Powervar UPS, which is a composite device utilizing interface greater than 0.

General idea of proposed changes - additional fields added to the usb_communication_subdriver_t structure as follows:

• hid_rep_index - interface number. Default is 0.
• hid_desc_index - descriptor number. Default is 0
• hid_ep_in - input interrupt endpoint number. Default is 1
• hid_ep_out - output interrup endpoint number. Default is 1.

These parameters can later be overridden in a usbhid-ups subdriver in order to set the correct values and remove the necessity to hardcode VID/PID in the libusb.c module. The most convenient place to override these settings in the subdriver is the claim method as it is called at the time of subdriver matching before any HID requests:

/* this function allows the subdriver to "claim" a device: return 1 if
 * the device is supported by this subdriver, else 0. */
static int arduino_claim(HIDDevice_t *hd)
{
	int status = is_usb_device_supported(arduino_usb_device_table, hd);

	switch (status)
	{
	case POSSIBLY_SUPPORTED:
		/* by default, reject, unless the productid option is given */
		if (getval("productid")) {
                        usb->hid_ep_in=4;          /* <== setting output interrupt end point to 4 */
                        usb->hid_ep_out=5;        /* <== setting output interrupt end point to 5 */
                        usb->hid_rep_index = 2;  /* <== setting interface to 2 */
			return 1;
		}
		possibly_supported("Arduino", hd);
		return 0;

	case SUPPORTED:
                usb->hid_ep_in=4;
                usb->hid_ep_out=5;
                usb->hid_rep_index = 2;
		return 1;

	case NOT_SUPPORTED:
	default:
		return 0;
	}
}

Additional usbhid-ups subdriver added to support Arduino-based HID-compliant UPS project. The subdriver uses the interface 2 and interrupt endpoints 4 and 5 thus utilizing the flexibility introduced by this PR. The idea of introducing this driver is to enable creation and prototyping of smart UPS based on Arduino, which is inexpensive versatile platform very much suitable for custom fully open-source UPS development.

Powerwar UPS HID subdriver is also updated - update of hid_rep_index based on hardcoded VID/PID removed from libusb.c. The hid_rep_index for this device is now set in the subdriver itself similarly to the code snippet above.

[lgtm-com]

This pull request introduces 1 alert when merging 2d8b0e93c67761b84b9c652fd7514aa4fe45cf77 into 0a198f0 - view on LGTM.com

new alerts:

• 1 for FIXME comment

[jimklimov]

Thanks for this PR! Looks reasonable, but got a few nits to address (including consistent indentation).

Also would like some review from other community members, to make sure I did not miss an elephant in the room.

[clepple]

Overall a useful improvement to the code, thanks!

I think the short commit message should match the PR title, or even better, the commit should be split into the two separate commits (one for interface > 0, and one for Arduino support). The "request changes" feedback is for the comments in the rest of my review, not splitting the commit, but smaller and more specific commits make it easier if someone is using git bisect or manually reverting commits during regression testing.

[abratchik]

Overall a useful improvement to the code, thanks!

I think the short commit message should match the PR title, or even better, the commit should be split into the two separate commits (one for interface > 0, and one for Arduino support). The "request changes" feedback is for the comments in the rest of my review, not splitting the commit, but smaller and more specific commits make it easier if someone is using git bisect or manually reverting commits during regression testing.

@clepple thank you for review - I agree this should be 2 commits. On the other hand, the change was only to add Arduino support and open the door for custom UPS development. Enhancement to libusb is a side change, just to avoid hardcoded VID/PID.

[jimklimov]

Well, technically... they do sort of pursue different goals and might be cherry-picked separately by some NUT fork or release to bring different benefits. Non-zero interface support is a welcome feature on its own (especially if made configurable), and Arduino driver is "just" one of several consumers for such feature.

[clepple]

LGTM, thanks!

[abratchik]

@jimklimov I have decomposed the commits into two as requested, please kindly check

[jimklimov]

Sorry it took a while to rectify the NUT CI side as well to get to the PRs in queue, great thanks for your contribution and patience!

[jimklimov]

Had a few "cosmetic" nits, an important one being the copyrights in new files. Since I can't edit the source for this PR, I proposed the changes in abratchik#1 - so intend to delay merging this one for a few days until that one is accepted, or would add those commits to NUT later after merging this PR anyway :)

[abratchik]

Had a few "cosmetic" nits, an important one being the copyrights in new files. Since I can't edit the source for this PR, I proposed the changes in abratchik#1 - so intend to delay merging this one for a few days until that one is accepted, or would add those commits to NUT later after merging this PR anyway :)

hi @jimklimov I accepted your changes, thanks a lot!