Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support Armac O/850E/PSW #2153 #2154

Merged
merged 9 commits into from
Jan 18, 2024
+157 −32
Merged

support Armac O/850E/PSW #2153 #2154

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
11c298b
support Armac O/850E/PSW #2153
vytautassurvila Nov 7, 2023
4cca5d7
drivers/nutdrv_qx.c: load_armac_endpoint_cache(): avoid "-Wdeclaratio…
jimklimov Nov 22, 2023
7a490af
drivers/nutdrv_qx.c: armac_command(): avoid "-Wdeclaration-after-stat…
jimklimov Nov 22, 2023
fdf5471
Merge branch 'master' into armac-2023
jimklimov Nov 24, 2023
8de5371
Merge branch 'master' into armac-2023
jimklimov Dec 3, 2023
596ba3a
Merge branch 'master' into armac-2023
jimklimov Jan 13, 2024
8a8bb7d
Merge branch 'networkupstools:master' into armac-2023
vytautassurvila Jan 15, 2024
dc3299e
drivers/nutdrv_qx.c: return actual number of bytes we copied to buffer
vytautassurvila Jan 15, 2024
66ff781
drivers/nutdrv_qx.c: use correct read_size for 'too much data read' m…
vytautassurvila Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 155 additions & 30 deletions drivers/nutdrv_qx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1836,20 +1836,119 @@ static void *ablerex_subdriver_fun(USBDevice_t *device)
return NULL;
}

static struct {
bool_t initialized;
bool_t ok;
uint8_t in_endpoint_address;
uint8_t in_bmAttributes;
uint16_t in_wMaxPacketSize;
uint8_t out_endpoint_address;
uint8_t out_bmAttributes;
uint16_t out_wMaxPacketSize;
} armac_endpoint_cache = { .initialized = FALSE, .ok = FALSE };

static void load_armac_endpoint_cache(void)
{
#if WITH_LIBUSB_1_0
int ret;
struct libusb_device *dev;
struct libusb_config_descriptor *config_descriptor;
bool_t found_in = FALSE;
bool_t found_out = FALSE;
#endif /* WITH_LIBUSB_1_0 */

if (armac_endpoint_cache.initialized) {
return;
}

armac_endpoint_cache.initialized = TRUE;
armac_endpoint_cache.ok = FALSE;

#if WITH_LIBUSB_1_0
dev = libusb_get_device(udev);
if (!dev) {
upsdebugx(4, "load_armac_endpoint_cache: unable to libusb_get_device");
return;
}

ret = libusb_get_active_config_descriptor(dev, &config_descriptor);
if (ret) {
upsdebugx(4, "load_armac_endpoint_cache: libusb_get_active_config_descriptor error=%d", ret);
libusb_free_config_descriptor(config_descriptor);
return;
}

if (config_descriptor->bNumInterfaces != 1) {
upsdebugx(4, "load_armac_endpoint_cache: unexpected config_descriptor->bNumInterfaces=%d", config_descriptor->bNumInterfaces);
libusb_free_config_descriptor(config_descriptor);
return;
} else {
/* Here and below, the "else" is for C99-satisfying new variable scoping */
const struct libusb_interface *interface = &config_descriptor->interface[0];

if (interface->num_altsetting != 1) {
upsdebugx(4, "load_armac_endpoint_cache: unexpected interface->num_altsetting=%d", interface->num_altsetting);
libusb_free_config_descriptor(config_descriptor);
return;
} else {
uint8_t i;
const struct libusb_interface_descriptor *interface_descriptor = &interface->altsetting[0];

if (interface_descriptor->bNumEndpoints != 2) {
upsdebugx(4, "load_armac_endpoint_cache: unexpected interface_descriptor->bNumEndpoints=%d", interface_descriptor->bNumEndpoints);
libusb_free_config_descriptor(config_descriptor);
return;
}

for (i = 0; i < interface_descriptor->bNumEndpoints; i++) {
const struct libusb_endpoint_descriptor *endpoint = &interface_descriptor->endpoint[i];

if (endpoint->bEndpointAddress & LIBUSB_ENDPOINT_IN) {
found_in = TRUE;
armac_endpoint_cache.in_endpoint_address = endpoint->bEndpointAddress;
armac_endpoint_cache.in_bmAttributes = endpoint->bmAttributes;
armac_endpoint_cache.in_wMaxPacketSize = endpoint->wMaxPacketSize;
} else {
found_out = TRUE;
armac_endpoint_cache.out_endpoint_address = endpoint->bEndpointAddress;
armac_endpoint_cache.out_bmAttributes = endpoint->bmAttributes;
armac_endpoint_cache.out_wMaxPacketSize = endpoint->wMaxPacketSize;
}
}
}
}

if (found_in || found_out) {
armac_endpoint_cache.ok = TRUE;

upsdebugx(4, "%s: in_endpoint_address=%02x, in_bmAttributes=%02d, out_endpoint_address=%02d, out_bmAttributes=%02d",
__func__, armac_endpoint_cache.in_endpoint_address, armac_endpoint_cache.in_bmAttributes,
armac_endpoint_cache.out_endpoint_address, armac_endpoint_cache.out_bmAttributes);
}

libusb_free_config_descriptor(config_descriptor);
#else /* WITH_LIBUSB_1_0 */
upsdebugx(4, "%s: SKIP: not implemented for libusb-0.1 or serial connections", __func__);
#endif /* !WITH_LIBUSB_1_0 */
}

/* Armac communication subdriver
*
* This reproduces a communication protocol used by an old PowerManagerII
* software, which doesn't seem to be Armac specific. The banner is: "2004
* Richcomm Technologies, Inc. Dec 27 2005 ver 1.1." Maybe other Richcomm UPSes
* would work with this - better than with the richcomm_usb driver.
*/
#define ARMAC_READ_SIZE 6
#define ARMAC_READ_SIZE_FOR_CONTROL 6
#define ARMAC_READ_SIZE_FOR_INTERRUPT 64
static int armac_command(const char *cmd, char *buf, size_t buflen)
{
char tmpbuf[ARMAC_READ_SIZE];
char tmpbuf[ARMAC_READ_SIZE_FOR_INTERRUPT];
int ret = 0;
size_t i, bufpos;
const size_t cmdlen = strlen(cmd);
bool_t use_interrupt = FALSE;
int read_size = ARMAC_READ_SIZE_FOR_CONTROL;

/* UPS ignores (doesn't echo back) unsupported commands which makes
* the initialization long. List commands tested to be unsupported:
Expand All @@ -1863,6 +1962,10 @@ static int armac_command(const char *cmd, char *buf, size_t buflen)
NULL
};

if (!armac_endpoint_cache.initialized) {
load_armac_endpoint_cache();
}

for (i = 0; unsupported[i] != NULL; i++) {
if (strcmp(cmd, unsupported[i]) == 0) {
upsdebugx(2,
Expand All @@ -1873,29 +1976,51 @@ static int armac_command(const char *cmd, char *buf, size_t buflen)
}
upsdebugx(4, "armac command %.*s", (int)strcspn(cmd, "\r"), cmd);

/* Cleanup buffer before sending a new command */
for (i = 0; i < 10; i++) {
ret = usb_interrupt_read(udev, 0x81,
(usb_ctrl_charbuf)tmpbuf, ARMAC_READ_SIZE, 100);
if (ret != ARMAC_READ_SIZE) {
// Timeout - buffer is clean.
break;
#if WITH_LIBUSB_1_0
/* Be conservative and do not break old Armac UPSes */
use_interrupt = armac_endpoint_cache.ok
&& armac_endpoint_cache.in_endpoint_address == 0x82
&& armac_endpoint_cache.in_bmAttributes & LIBUSB_TRANSFER_TYPE_INTERRUPT
&& armac_endpoint_cache.out_endpoint_address == 0x02
&& armac_endpoint_cache.out_bmAttributes & LIBUSB_TRANSFER_TYPE_INTERRUPT
&& armac_endpoint_cache.in_wMaxPacketSize == 64;
#endif /* WITH_LIBUSB_1_0 */

if (use_interrupt && cmdlen + 1 < armac_endpoint_cache.in_wMaxPacketSize) {
memset(tmpbuf, 0, sizeof(tmpbuf));
tmpbuf[0] = 0xa0 + cmdlen;
memcpy(tmpbuf + 1, cmd, cmdlen);

ret = usb_interrupt_write(udev,
vytautassurvila marked this conversation as resolved.
Show resolved Hide resolved
armac_endpoint_cache.out_endpoint_address,
(usb_ctrl_charbuf)tmpbuf, cmdlen + 1, 5000);

read_size = ARMAC_READ_SIZE_FOR_INTERRUPT;
} else {
/* Cleanup buffer before sending a new command */
for (i = 0; i < 10; i++) {
ret = usb_interrupt_read(udev, 0x81,
(usb_ctrl_charbuf)tmpbuf, ARMAC_READ_SIZE_FOR_CONTROL, 100);
if (ret != ARMAC_READ_SIZE_FOR_CONTROL) {
// Timeout - buffer is clean.
break;
}
upsdebugx(4, "armac cleanup ret i=%" PRIuSIZE " ret=%d ctrl=%02hhx", i, ret, tmpbuf[0]);
}
upsdebugx(4, "armac cleanup ret i=%" PRIuSIZE " ret=%d ctrl=%02hhx", i, ret, tmpbuf[0]);
}

/* Send command to the UPS in 3-byte chunks. Most fit 1 chunk, except for eg.
* parameterized tests. */
for (i = 0; i < cmdlen;) {
const size_t bytes_to_send = (cmdlen <= (i + 3)) ? (cmdlen - i) : 3;
memset(tmpbuf, 0, sizeof(tmpbuf));
tmpbuf[0] = 0xa0 + bytes_to_send;
memcpy(tmpbuf + 1, cmd + i, bytes_to_send);
ret = usb_control_msg(udev,
USB_ENDPOINT_OUT + USB_TYPE_CLASS + USB_RECIP_INTERFACE,
0x09, 0x200, 0,
(usb_ctrl_charbuf)tmpbuf, 4, 5000);
i += bytes_to_send;
/* Send command to the UPS in 3-byte chunks. Most fit 1 chunk, except for eg.
* parameterized tests. */
for (i = 0; i < cmdlen;) {
const size_t bytes_to_send = (cmdlen <= (i + 3)) ? (cmdlen - i) : 3;
memset(tmpbuf, 0, sizeof(tmpbuf));
tmpbuf[0] = 0xa0 + bytes_to_send;
memcpy(tmpbuf + 1, cmd + i, bytes_to_send);
ret = usb_control_msg(udev,
USB_ENDPOINT_OUT + USB_TYPE_CLASS + USB_RECIP_INTERFACE,
0x09, 0x200, 0,
(usb_ctrl_charbuf)tmpbuf, 4, 5000);
i += bytes_to_send;
}
}

if (ret <= 0) {
Expand All @@ -1911,17 +2036,17 @@ static int armac_command(const char *cmd, char *buf, size_t buflen)
memset(buf, 0, buflen);

bufpos = 0;
while (bufpos + ARMAC_READ_SIZE < buflen) {
while (bufpos + read_size + 1 < buflen) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This +1 along 0x3f instead of 0x0f is pretty much the only change I'm not certain won't affect all other versions.

We'd need to just test it on hardware I guess. Or create a database of recorded communications and do a regression testing on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the +1 is for ensuring a zero-byte in the end (of buffer to be provided big enough by the caller).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a bit more about this, it may be prudent to check that buflen > read_size in advance: if the caller's buffer is too small to fit even the first read eventually (if usb_interrupt_read() would return the full requested read_size and not fewer bytes for some reason), this is more of a programmer error to upsdebugx() as such than a device/protocol error. I think it can help long-term troubleshooting as this code evolves.

Then there's a chance that we expect to read say 64 bytes (due to if use_interrupt ... above) but only get say 6 in reality (guessing here, no idea if this reality is possible - WDYT?), and the caller's buffer is big enough for 6 but not for 64. With current while-clause, we would not even bother trying a query. Is this a correct thing to do?

Just trying to think of things that could go wrong, so might brainstorm up some garbage ideas here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be prudent to check that buflen > read_size in advance

If I understand it right we already have this check - we won't enter loop at all in that case while (bufpos + read_size + 1 < buflen). And bellow the loop we emit the error for this specific case upsdebugx(2, "Protocol error, too much data read.");. It still had hardcoded read_size that was fixed via 66ff781

Then there's a chance that we expect to read say 64 bytes (due to if use_interrupt ... above) but only get say 6 in reality

At least in my case I do always get 64 bytes from usb_interrupt_read. From them we have only 47 meaningful bytes that will be copied to buf. This is handled via bytes_available = (unsigned char)tmpbuf[0] & 0x3f;. In theory this function could be changed to work when caller's buffer would be less than 64 bytes. But then we are risking that all data we read from interrupt won't fit to buf. I'm not sure which way is better?

size_t bytes_available;

/* Read data in 6-byte chunks */
ret = usb_interrupt_read(udev, 0x81,
(usb_ctrl_charbuf)tmpbuf, ARMAC_READ_SIZE, 1000);
ret = usb_interrupt_read(udev, use_interrupt ? armac_endpoint_cache.in_endpoint_address : 0x81,
(usb_ctrl_charbuf)tmpbuf, read_size, 1000);

/* Any errors here mean that we are unable to read a reply
* (which will happen after successfully writing a command
* to the UPS) */
if (ret != ARMAC_READ_SIZE) {
if (ret != read_size) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we loop here (in current code and the earlier one), would it be less misleading to return not the latest ret (if positive, just too short presumably), but a count of successfully read bytes e.g. bufpos + ret? If ret is negative (for error), probably pass it through as is and disregard any earlier success? Or return bufpos anyway, if positive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Fixed via dc3299e

Copy link
Member

@jimklimov jimklimov Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vytautassurvila @blaa : any particular preference for bufpos vs. bufpos + ret in this case?

Generally I think we can fall into this situation if the device has less info to send than exactly the read_size. Like, it has 8 bytes for some message and we read in 6-byte chunks, so our buffer gets ret=6 in one cycle and ret=2 in the other quite validly - and if ret is always non-negative, we can tell the caller that we have collected 8 bytes.

But this is a general train of thought, you may be better aware about what does and does not happen in these particular interactions :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken here we just received data to tmpbuf. It will be copied to buf only at line 2107. I guess idea is to copy data to buf only when some validation is performed on data we received.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, good catch you too :")

So if we abort here, buf has only received bufpos finished bytes indeed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose, here and elsewhere we declare that correct reads always return exactly read_size and may not be shorter, to the best of our practical knowledge at this time at least?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can speak only about my UPS - usb_interrupt_read and read_size always match. Within those 64 bytes not all bytes are meaningful - that is determined by bit masking first byte.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the insights. LGTM I guess. The caller's buffer is large enough for the tested devices, I suppose?

/* NOTE: If end condition is invalid for particular UPS we might make one
* request more and get this error. If bufpos > (say) 10 this could be ignored
* and the reply correctly read. */
Expand All @@ -1946,15 +2071,15 @@ static int armac_command(const char *cmd, char *buf, size_t buflen)
* Current assumption is that this is number of bytes available on the UPS side
* with up to 5 (ret - 1) transferred.
*/
bytes_available = (unsigned char)tmpbuf[0] & 0x0f;
bytes_available = (unsigned char)tmpbuf[0] & 0x3f;
if (bytes_available == 0) {
/* End of transfer */
break;
}

if (bytes_available > ARMAC_READ_SIZE - 1) {
if (bytes_available > (unsigned)read_size - 1) {
/* Single interrupt transfer has 1 control + 5 data bytes */
bytes_available = ARMAC_READ_SIZE - 1;
bytes_available = read_size - 1;
}

/* Copy bytes into the final buffer while detecting end of line - \r */
Expand Down
Loading