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.

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

Use libusb-1.0 report of pollfd's in NUT main loop #2747

Open
jimklimov opened this issue Jan 3, 2025 · 1 comment
Open

Use libusb-1.0 report of pollfd's in NUT main loop #2747

jimklimov opened this issue Jan 3, 2025 · 1 comment
Labels
enhancement USB Windows Windows-not-on-par-with-POSIX Aspect of Windows builds known to be dysfunctional compared to POSIX builds; fix needed to be on par
Milestone

Comments

@jimklimov
Copy link
Member

As discussed in #2742 (comment) a NUT driver (any one of them) does some work in upsdrv_updateinfo(), effectively sleeps for the remainder of timeout in dstate_poll_fds() (if no extrafd like those for a serial port or a raw network socket gets bytes incoming, or a sockfd internal to dstate.c), and only then gets to run upsdrv_updateinfo() again and perhaps notice the absence of the device as reported by libusb.

NOTE: Currently extrafd is ignored in WIN32 builds, but we do handle sockfd so this might be extensible eventually. See also https://learn.microsoft.com/en-us/windows/win32/winsock/select-and-fd---2

Currently this shortcut is not really used by the majority of drivers, so they normally all "suffer" a standard delay between loop cycles, with the few hits being:

:; git grep extrafd
drivers/apcsmart-old.c: extrafd = upsfd;
drivers/apcsmart.c:     upsfd = extrafd = ser_open(device_path);
drivers/clone-outlet.c:         extrafd = upsfd = sstate_connect();
drivers/clone-outlet.c: extrafd = upsfd = sstate_connect();
drivers/clone.c:                extrafd = upsfd = sstate_connect();
drivers/clone.c:        extrafd = upsfd = sstate_connect();
drivers/dstate.c:int dstate_poll_fds(struct timeval timeout, TYPE_FD arg_extrafd)
drivers/dstate.c:       if (VALID_FD(arg_extrafd)) {
drivers/dstate.c:               FD_SET(arg_extrafd, &rfds);
drivers/dstate.c:               if (arg_extrafd > maxfd) {
drivers/dstate.c:                       maxfd = arg_extrafd;
drivers/dstate.c:       if (VALID_FD(arg_extrafd) && (FD_ISSET(arg_extrafd, &rfds))) {
drivers/dstate.c:       NUT_UNUSED_VARIABLE(arg_extrafd);
drivers/dstate.c:       if (VALID_FD(arg_extrafd)) {
drivers/dstate.c:               rfds[maxfd] = arg_extrafd;
drivers/dstate.c:       if (VALID_FD(arg_extrafd) && (ret == arg_extrafd)) {
drivers/dstate.h:int dstate_poll_fds(struct timeval timeout, TYPE_FD extrafd);
drivers/main.c:TYPE_FD  extrafd = ERROR_FD;
drivers/main.c:                 while (!dstate_poll_fds(timeout, extrafd) && !exit_flag) {
drivers/main.c:                         /* repeat until time is up or extrafd has data */
drivers/main.h:extern TYPE_FD           upsfd, extrafd;
drivers/netxml-ups.c:/* TODO: port extrafd to Windows */
drivers/netxml-ups.c:                   extrafd = ne_sock_fd(sock);
drivers/netxml-ups.c:/* TODO: port extrafd to Windows */
drivers/netxml-ups.c:                           extrafd = ne_sock_fd(sock);
drivers/apcsmart-old.c: extrafd = upsfd;
drivers/apcsmart.c:     upsfd = extrafd = ser_open(device_path);
drivers/clone-outlet.c:         extrafd = upsfd = sstate_connect();
drivers/clone-outlet.c: extrafd = upsfd = sstate_connect();
drivers/clone.c:                extrafd = upsfd = sstate_connect();
drivers/clone.c:        extrafd = upsfd = sstate_connect();
drivers/dstate.c:int dstate_poll_fds(struct timeval timeout, TYPE_FD arg_extrafd)
drivers/dstate.c:       if (VALID_FD(arg_extrafd)) {
drivers/dstate.c:               FD_SET(arg_extrafd, &rfds);
drivers/dstate.c:               if (arg_extrafd > maxfd) {
drivers/dstate.c:                       maxfd = arg_extrafd;
drivers/dstate.c:       if (VALID_FD(arg_extrafd) && (FD_ISSET(arg_extrafd, &rfds))) {
drivers/dstate.c:       NUT_UNUSED_VARIABLE(arg_extrafd);
drivers/dstate.c:       if (VALID_FD(arg_extrafd)) {
drivers/dstate.c:               rfds[maxfd] = arg_extrafd;
drivers/dstate.c:       if (VALID_FD(arg_extrafd) && (ret == arg_extrafd)) {
drivers/dstate.h:int dstate_poll_fds(struct timeval timeout, TYPE_FD extrafd);
drivers/main.c:TYPE_FD  extrafd = ERROR_FD;
drivers/main.c:                 while (!dstate_poll_fds(timeout, extrafd) && !exit_flag) {
drivers/main.c:                         /* repeat until time is up or extrafd has data */
drivers/main.h:extern TYPE_FD           upsfd, extrafd;
drivers/netxml-ups.c:/* TODO: port extrafd to Windows */
drivers/netxml-ups.c:                   extrafd = ne_sock_fd(sock);
drivers/netxml-ups.c:/* TODO: port extrafd to Windows */
drivers/netxml-ups.c:                           extrafd = ne_sock_fd(sock);

As documented in https://libusb.sourceforge.io/api-1.0/group__libusb__poll.html#ga54c27dcf8a95d2a3a03cfb7dd37eae63 and https://libusb.sourceforge.io/api-1.0/structlibusb__pollfd.html the LibUSB-1.0 API actually allows programs to see the collection of file descriptors (and their event flags from poll.h) associated with a context, so we have a chance to redesign driver loops (not sure OTOH there's one or more extrafd equivalents in our case) in such a way that we can react more quickly to USB device events. Note there does not seem to be any similar facility in older LibUSB-0.1 headers.

This idea may also help with #2609 as well.

@jimklimov jimklimov added enhancement Windows USB Windows-not-on-par-with-POSIX Aspect of Windows builds known to be dysfunctional compared to POSIX builds; fix needed to be on par labels Jan 3, 2025
@jimklimov jimklimov added this to the 2.8.4 milestone Jan 3, 2025
@jimklimov
Copy link
Member Author

Per networkupstools/libmodbus#1 (comment):

NOTE: The test (made with libmodbus use of this API) Got a list of 3 libusb file descriptors to poll and so did not assign any to ctx->s; this may need more exploration - what those FDs are and if each should be polled. Maybe change the single ctx->s to an array (or to keep code of other RTU's largely unmodified, add the array to backend or even common ctx structure, and use it for select() if e.g. ctx->s==-2)?

The same considerations impact the use of single extrafd in NUT drivers. In the few that actually use it, so relatively easy to refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement USB Windows Windows-not-on-par-with-POSIX Aspect of Windows builds known to be dysfunctional compared to POSIX builds; fix needed to be on par
Projects
None yet
Development

No branches or pull requests

1 participant