tripplite_usb driver bugs

[sapireli]

The man page for the tripplite usb driver states the driver supports extra arguments which it doesn't. Specifically:

1. device, the driver accepts a bus argument but not a device, which becomes a problem when you have two of the same devices
2. serial, the serial isn't available to the driver instead what is passed is "ups.id" which is a user settable identifier with a range of 1 - 65535 using 'upsrw' utility, by default it is set to 65535. i propose this should be utilized instead of serial so that one can monitor multiple units of the same type and model.
3. allow_duplicates, doesn't work, the driver rejects this argument stating incompatibility.
   --
   @jimklimov @seanm @sshimko @mbastiaan

[jimklimov]

What version of code are you running? 1 and 3 are master branch (will be 2.8.1 soonish). Seems you run older packaged code - it should have the older man pages accompanying (or on web site - see historic release sub-site).

[jimklimov]

not sure about 2 right now

[jimklimov]

not at a computer now to double-check the master branch - it might be that the driver was missed when updating usb args handling across the board...

[sapireli]

I feel stupid now, I’m running Debian bullseye and the latest package available is 2.7.4-13.

I still think #2 is a far more elegant solution. Now I gotta figure out how to compile the latest version and install it over

[jimklimov]

Well, I don't think an `ups.id` is something every device supports or can set... does it also persist across UPS power cycles for you (saved to ppwer-independent flash)? For devices that have it, matching can be an option. Maybe even in standard shared code. PRs welcome, see the recent one for busport.

…

On Sat, Sep 23, 2023, 02:53 maximilanna ***@***.***> wrote: I feel stupid now, I’m running Debian bullseye and the latest package available is 2.7.4-13. I still think #2 <#2> is a far more elegant solution. Now I gotta figure out how to compile the latest version and install it over — Reply to this email directly, view it on GitHub <#2075 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMPTFESYDLA3IR3LN7TMPDX3YXG7ANCNFSM6AAAAAA5DREIOI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jimklimov]

I think that the bigger problem would be that asking for an id would require actual (usb hid? qx? other?) protocol interaction, while the data for initial match comes from libusb basics. So it may happen thatsolutiobs need to be in drivets' claim methods, not shared once and for all. Give it a shot, though, to see how low the friut hangs...

[sapireli]

The ups.id persists across reboots there is also no easy way to reset it, the only thing you can do is change it again.

It seems the driver is still rejecting the device argument. Yesterday I installed 2.8.0 and now I get the error:

:; /lib/nut/tripplite_usb -s ups -x bus="001" -x port=auto -x device="004"

Network UPS Tools - Tripp Lite OMNIVS / SMARTPRO driver 0.33 (2.8.0)
Warning: This is an experimental driver.
Some features may not function correctly.

libusb1: Could not open any HID devices: insufficient permissions on everything
No matching USB/HID UPS found

if I remove the “device” argument it works well.

[sapireli]

Also interestingly enough when I omit the device argument and it connects, it actually spits out the ups.id which in my case I modified it to 101.

:; /lib/nut/tripplite_usb -s ups -x bus="001" -x port=auto
Network UPS Tools - Tripp Lite OMNIVS / SMARTPRO driver 0.33 (2.8.0)
Warning: This is an experimental driver.
Some features may not function correctly.

Detected a UPS: TRIPP LITE/TRIPP LITE SMART1000RM2U    
Using SMARTPRO protocol (3003)
Unit ID: 101
Attached to Tripp Lite SMART1000RM2U

[sapireli]

Also allow duplicates isn’t working on 2.8.0

Fatal error: 'allow_duplicates' is not a valid flag for this driver.

[jimklimov]

As noted above, "#1" and "#3" (proper handling of "device" as well as relaxation to "allow_duplicates") appeared on master branch recently -- so are not part of 2.8.0 release which appeared before (PRs #1770 and #1763).

As for 101 showing up in Unit ID - that is a helpful bit -- at least the driver-specific recognizer might harness it after all.

Care to get hands dirty and dive under the hood? ;)

[sapireli]

i'll try ;)
fair to assume all the code that needs to be modified is simply in the single driver file?

[jimklimov]

Not necessarily. The recent PR #2054 to add a busport matcher for USB connections in general touches on all files involved in that, I think - several USB drivers (some are "too legacy" to touch with this sort of changes easily, as I found in earlier PRs), shared code in libusb{0,1}.c or usb-common.c, docs, nut-scanner, etc.

But the driver itself is a fair starting point to begin unraveling from (look at code, add debug messages, maybe run it with an IDE like NetBeans to step through the code as it learns about the device, etc.).

[jimklimov]

Skimmed the code going from that logged message, and the ups.id seems to be device-dependent:

• nut/drivers/tripplite_usb.c

  Lines 993 to 996 in d00e5e0

  	void upsdrv_initinfo(void)
  	{
  	const unsigned char proto_msg[] = "\0", f_msg[] = "F", p_msg[] = "P",
  	s_msg[] = "S", u_msg[] = "U", v_msg[] = "V", w_msg[] = "W\0";

  defines certain messages that can be posted to the device

• nut/drivers/tripplite_usb.c

  Lines 1133 to 1153 in d00e5e0

  	if(tl_model != TRIPP_LITE_OMNIVS && tl_model != TRIPP_LITE_SMART_0004) {
  	/* Unit ID might not be supported by all models: */
  	ret = send_cmd(u_msg, sizeof(u_msg), u_value, sizeof(u_value)-1);
  	if(ret <= 0) {
  	upslogx(LOG_INFO, "Unit ID not retrieved (not available on all models)");
  	} else {
  	unit_id = (long)((unsigned)(u_value[1]) << 8)
  	| (unsigned)(u_value[2]);
  	}
  	if(tl_model == TRIPP_LITE_SMART_0004) {
  	debug_message("U", 2);
  	}
  	}
  	if(unit_id >= 0) {
  	dstate_setinfo("ups.id", "%ld", unit_id);
  	dstate_setflags("ups.id", ST_FLAG_RW | ST_FLAG_STRING);
  	dstate_setaux("ups.id", 5);
  	upslogx(LOG_DEBUG,"Unit ID: %ld", unit_id);
  	}

  tries to post the query and interpret the result as an Unit ID
• with a send_cmd() implementation relying on having ownership of the device, to send commands and await the reports:

  nut/drivers/tripplite_usb.c

  Lines 582 to 664 in d00e5e0

  	/*!@brief Send a command to the UPS, and wait for a reply.
  	*
  	* All of the UPS commands are challenge-response. If a command does not have
  	* anything to return, it simply returns the command character.
  	*
  	* @param[in] msg Command string, minus the ':' or CR
  	* @param[in] msg_len Be sure to use sizeof(msg) instead of strlen(msg),
  	* since some commands have embedded NUL characters
  	* @param[out] reply Reply (but check return code for validity)
  	* @param[out] reply_len (currently unused)
  	*
  	* @return number of chars in reply, excluding terminating NUL
  	* @return 0 if command was not accepted
  	*/
  	static int send_cmd(const unsigned char *msg, size_t msg_len, unsigned char *reply, size_t reply_len)
  	{
  	unsigned char buffer_out[8];
  	unsigned char csum = 0;
  	int ret = 0, send_try, recv_try=0, done = 0;
  	size_t i = 0;
  	NUT_UNUSED_VARIABLE(reply_len);
  	upsdebugx(3, "send_cmd(msg_len=%u, type='%c')", (unsigned)msg_len, msg[0]);
  	if(msg_len > 5) {
  	fatalx(EXIT_FAILURE, "send_cmd(): Trying to pass too many characters to UPS (%u)", (unsigned)msg_len);
  	}
  	buffer_out[0] = ':';
  	for(i=1; i<8; i++) buffer_out[i] = '\0';
  	for(i=0; i<msg_len; i++) {
  	buffer_out[i+1] = msg[i];
  	csum += msg[i];
  	}
  	buffer_out[i] = 255-csum;
  	buffer_out[i+1] = ENDCHAR;
  	upsdebugx(5, "send_cmd: sending %s", hexascdump(buffer_out, sizeof(buffer_out)));
  	for(send_try=0; !done && send_try < MAX_SEND_TRIES; send_try++) {
  	upsdebugx(6, "send_cmd send_try %d", send_try+1);
  	ret = comm_driver->set_report(udev, 0,
  	(usb_ctrl_charbuf)buffer_out,
  	(usb_ctrl_charbufsize)sizeof(buffer_out));
  	if(ret != sizeof(buffer_out)) {
  	upslogx(1, "libusb_set_report() returned %d instead of %" PRIuSIZE,
  	ret, sizeof(buffer_out));
  	return ret;
  	}
  	#if ! defined(__FreeBSD__)
  	usleep(1000*100); /* TODO: nanosleep */
  	#endif
  	for(recv_try=0; !done && recv_try < MAX_RECV_TRIES; recv_try++) {
  	upsdebugx(7, "send_cmd recv_try %d", recv_try+1);
  	ret = comm_driver->get_interrupt(udev,
  	(usb_ctrl_charbuf)reply,
  	(usb_ctrl_charbufsize)sizeof(buffer_out),
  	RECV_WAIT_MSEC);
  	if(ret != sizeof(buffer_out)) {
  	upslogx(1, "libusb_get_interrupt() returned %d instead of %u while sending %s",
  	ret, (unsigned)(sizeof(buffer_out)),
  	hexascdump(buffer_out, sizeof(buffer_out)));
  	}
  	done = (ret == sizeof(buffer_out)) && (buffer_out[1] == reply[0]);
  	}
  	}
  	if(ret == sizeof(buffer_out)) {
  	upsdebugx(5, "send_cmd: received %s (%s)", hexascdump(reply, sizeof(buffer_out)),
  	done ? "OK" : "bad");
  	}
  	upsdebugx(((send_try > 2) || (recv_try > 2)) ? 3 : 6,
  	"send_cmd: send_try = %d, recv_try = %d\n", send_try, recv_try);
  	return done ? sizeof(buffer_out) : 0;
  	}

The nut_libusb_open() (namely in

nut/drivers/libusb1.c

Lines 690 to 694 in d00e5e0

	res = callback(udev, curDevice, rdbuf, (usb_ctrl_charbufsize)rdlen);
	if (res < 1) {
	upsdebugx(2, "Caller doesn't like this device");
	goto next_device;
	}

and

nut/drivers/libusb0.c

Lines 610 to 614 in d00e5e0

	res = callback(udev, curDevice, rdbuf, rdlen);
	if (res < 1) {
	upsdebugx(2, "Caller doesn't like this device");
	goto next_device;
	}

lines) actually establishes an ability to use a *callback() method to filter if "caller" likes the device after it has been grabbed by the driver (per shared code in this method) and collected initial HID report descriptors - find more per open_dev function pointer in

nut/drivers/nut_libusb.h

Lines 55 to 59 in d00e5e0

	int (*open_dev)(usb_dev_handle **sdevp, /* try to open the next available */
	USBDevice_t *curDevice, /* device matching USBDeviceMatcher_t */
	USBDeviceMatcher_t *matcher,
	int (*callback)(usb_dev_handle *udev, USBDevice_t *hd,
	usb_ctrl_charbuf rdbuf, usb_ctrl_charbufsize rdlen));

This happens after

nut/drivers/libusb0.c

Lines 463 to 465 in d00e5e0

	if (!callback) {
	return 1;
	}

or

nut/drivers/libusb1.c

Lines 523 to 527 in d00e5e0

	if (!callback) {
	libusb_free_config_descriptor(conf_desc);
	libusb_free_device_list(devlist, 1);
	return 1;
	}

consider if callback is NULL (in which case they skip evaluation of report descriptors).

Either way, the initial chain of matchers defined in upsdrv_initups()-- including specific data detected earlier just for reopen_matcher, and generally regexes (from ups.conf) to filter by basic information that libusb has, and the subdriver_match_func() to filter by IDs

nut/drivers/tripplite_usb.c

Lines 153 to 196 in d00e5e0

	/* TrippLite */
	#define TRIPPLITE_VENDORID 0x09ae
	/* USB IDs device table */
	static usb_device_id_t tripplite_usb_device_table[] = {
	/* e.g. OMNIVS1000, SMART550USB, ... */
	{ USB_DEVICE(TRIPPLITE_VENDORID, 0x0001), NULL },
	/* Terminating entry */
	{ 0, 0, NULL }
	};
	static int subdriver_match_func(USBDevice_t *arghd, void *privdata)
	{
	NUT_UNUSED_VARIABLE(privdata);
	/* FIXME? Should we save "arghd" into global "hd" variable?
	* This was previously shadowed by function argument named "hd"...
	*/
	/* hd = arghd; */
	switch (is_usb_device_supported(tripplite_usb_device_table, arghd))
	{
	case SUPPORTED:
	return 1;
	case POSSIBLY_SUPPORTED:
	/* by default, reject, unless the productid option is given */
	if (getval("productid")) {
	return 1;
	}
	return 0;
	case NOT_SUPPORTED:
	default:
	return 0;
	}
	}
	static USBDeviceMatcher_t subdriver_matcher = {
	&subdriver_match_func,
	NULL,
	NULL
	};

all happen before that callback is considered.

At

nut/drivers/tripplite_usb.c

Line 1601 in d00e5e0

r = comm_driver->open_dev(&udev, &curDevice, regex_matcher, NULL);

and

nut/drivers/tripplite_usb.c

Line 334 in d00e5e0

ret = comm_driver->open_dev(&udev, &curDevice, reopen_matcher, NULL);

we pass NULL for the callback argument - so this seems like the place you could extend to add matching by ups.id.

Hope this helps ;)
Jim Klimov

[sapireli]

Thank you @jimklimov, that was very helpful, i'm took a stab at it. #2093
Would love your thoughts