Skip to content

win32: fix short reads on serial timeout - #3560

Open
Marcelo-PX wants to merge 1 commit into
networkupstools:masterfrom
Marcelo-PX:win32-serial-timeout-posix
Open

win32: fix short reads on serial timeout#3560
Marcelo-PX wants to merge 1 commit into
networkupstools:masterfrom
Marcelo-PX:win32-serial-timeout-posix

Conversation

@Marcelo-PX

@Marcelo-PX Marcelo-PX commented Aug 5, 2026

Copy link
Copy Markdown

Summary

This is a revised follow-up to #3559.

Fix the Win32 serial read wrapper so that bytes already transferred before a
timeout are returned as a successful short read instead of being discarded as
an I/O error.

When w32_serial_read() times out while waiting for additional serial data,
valid bytes may already have been copied into the caller's buffer. The previous
Win32 code path could discard those bytes by falling through to the generic
error handling.

The revised implementation:

  • requests cancellation of the pending WaitCommEvent() operation;
  • waits for the overlapped operation to complete before reusing its state;
  • accepts ERROR_OPERATION_ABORTED as the expected cancellation result;
  • reports unexpected overlapped completion failures as -1 with
    errno = EIO;
  • preserves the existing zero-byte timeout behavior;
  • returns the accumulated byte count when one or more bytes were already
    transferred, following POSIX-style short-read semantics.

The comments for CreateEvent(..., TRUE, ...) were also corrected to identify
the events as manual-reset events.

Fixes #3452.

Rationale

A read operation which has already transferred one or more bytes should return
the byte count as a successful short read.

Returning a positive byte count together with an error in errno would not
provide useful portable semantics, since callers normally inspect errno only
when the primary return value indicates failure.

The caller remains responsible for validating protocol framing, terminators and
expected response lengths.

The cancellation path also waits for the pending overlapped operation to
complete before resetting and reusing the associated OVERLAPPED structure and
event.

Testing

Tested on Windows x86_64 with:

  • NUT development branch based on 2.8.5;
  • MSYS2/MinGW64;
  • nutdrv_qx 0.53;
  • Q1 protocol 0.08;
  • UPSBrasil 3 kVA UPS;
  • USB-to-serial adapter on COM4.

The revised driver and shared libraries were built locally and executed through
libtool --mode=execute to ensure that the newly compiled libraries were used.

Observed result:

w32_serial_read : timeout after receiving 47 characters, returning a short read
read: '(127.1 504.9 120.0 009 60.0 2.30 25.0 00000001'
subdriver_matcher: Trying protocol Q1 0.08: claim succeeded
Using protocol: Q1 0.08

A subsequent poll produced another successful 47-byte short read, confirming
that the behavior was repeatable.

The affected driver and shared libraries compiled successfully.

The source change also passed:

git diff --check
git diff --cached --check

AI disclosure

ChatGPT was used to assist with diagnosing the Win32 timeout behavior,
reviewing the overlapped cancellation and short-read semantics.

The code was manually reviewed, compiled and tested against real hardware by
the contributor.

General points

  • Described the changes in the PR submission or a separate issue, e.g.
    known published or discovered protocols, applicable hardware (expected
    compatible and actually tested/developed against), limitations, etc.

  • There may be multiple commits in the PR, aligned and commented with
    a functional change. Notably, coding style changes better belong in a
    separate PR, but certainly in a dedicated commit to simplify reviews
    of "real" changes in the other commits. Similarly for typo fixes in
    comments or text documents.

  • Use of coding helper tools and AI should be disclosed in the commit
    or PR comments (it is interesting to know which ones do a decent job).
    As with other contributions, a human is responsible and thanked for the
    quality and content of the change, and is presumed to have the right to
    post that code to be published further under the project's license terms.

  • Especially with involvement of AI, including modern IDE coding aid,
    please be sure to revise that proposed code and documentation changes
    follow NUT code style guide -- this helps portability across the decades
    worth of supported systems. Notably, avoid Unicode characters where ASCII
    text is expected (C sources and headers, manual pages and other acsiidoc
    inputs). Particularly AI is keen on adding mdash characters instead of
    plain ASCII double-dash (which renders into the long dash where applicable).

  • Please star NUT on GitHub, this helps with sponsorships! ;)

Frequent "underwater rocks" for driver addition/update PRs

  • Revised existing driver families and added a sub-driver if applicable
    (nutdrv_qx, usbhid-ups...) or added a brand new driver in the other
    case.

  • Did not extend obsoleted drivers with new hardware support features
    (notably blazer and other single-device family drivers for Qx protocols,
    except the new nutdrv_qx which should cover them all).

  • For updated existing device drivers, bumped the DRIVER_VERSION macro
    or its equivalent.

  • For USB devices (HID or not), revised that the driver uses unique
    VID/PID combinations, or raised discussions when this is not the case
    (several vendors do use same interface chips for unrelated protocols).

  • For new USB devices, built and committed the changes for the
    scripts/upower/95-upower-hid.hwdb file

  • Proposed NUT data mapping is aligned with existing docs/nut-names.txt
    file. If the device exposes useful data points not listed in the file, the
    experimental.* namespace can be used as documented there, and discussion
    should be raised on the NUT Developers mailing list to standardize the new
    concept.

  • Updated data/driver.list.in if applicable (new tested device info)

Frequent "underwater rocks" for general C code PRs

  • Did not "blindly assume" default integer type sizes and value ranges,
    structure layout and alignment in memory, endianness (layout of bytes and
    bits in memory for multi-byte numeric types), or use of generic int where
    language or libraries dictate the use of size_t (or ssize_t sometimes).

  • Progress and errors are handled with upsdebugx(), upslogx(),
    fatalx() and related methods, not with direct printf() or exit().
    Similarly, NUT helpers are used for error-checked memory allocation and
    string operations (except where customized error handling is needed,
    such as unlocking device ports, etc.)

  • Coding style (including whitespace for indentations) follows precedent
    in the code of the file, and examples/guide in docs/developers.txt file.

  • For newly added files, the Makefile.am recipes were updated and the
    make distcheck target passes.

General documentation updates

  • Added a bullet point into NEWS.adoc, possibly also UPGRADING.adoc
    if there is something packagers or custom-build users should take into
    account (new driver categories, configuration options, dependencies...)

  • Updated docs/acknowledgements.txt (for vendor-backed device support)

  • Added or updated manual page information in docs/man/*.txt files
    and corresponding recipe lists in docs/man/Makefile.am for new pages

  • Passed make spellcheck, updated spell-checking dictionary in the
    docs/nut.dict file if needed (did not remove any words -- the make
    rule printout in case of changes suggests how to maintain it).

Additional work may be needed after posting this PR

  • Propose a PR for NUT DDL with detailed device data dumps from tests
    against real hardware (the more models, the better).

  • Address NUT CI farm build failures for the PR: testing on numerous
    platforms and toolkits can expose issues not seen on just one system.

  • Revise suggestions from LGTM.COM analysis about "new issues" with
    the changed codebase.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

A ZIP file with standard source tarball and another tarball with pre-built docs for commit d730b34 is temporarily available: NUT-tarballs-PR-3560.zip.

@jimklimov jimklimov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, can you look at errno for tot==0 case, and add an entry in NEWS.adoc please?

Comment thread common/wincompat.c Outdated
@jimklimov jimklimov added this to the 2.8.6 milestone Aug 5, 2026
@jimklimov jimklimov added serial port 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 Aug 5, 2026
Signed-off-by: Marcelo Pacheco <marcello.mpacheco@gmail.com>
@Marcelo-PX
Marcelo-PX force-pushed the win32-serial-timeout-posix branch from e8a79ed to d730b34 Compare August 5, 2026 18:13
@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5030-master completed (commit e928ac69db by @Marcelo-PX)

@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5031-master completed (commit dd57052335 by @Marcelo-PX)

@Marcelo-PX Marcelo-PX closed this Aug 5, 2026
@Marcelo-PX
Marcelo-PX deleted the win32-serial-timeout-posix branch August 5, 2026 19:18
@Marcelo-PX
Marcelo-PX restored the win32-serial-timeout-posix branch August 5, 2026 19:19
@Marcelo-PX Marcelo-PX reopened this Aug 5, 2026
@jimklimov jimklimov added the AI For good or bad, machine tools are upon us. Humans are still the responsible ones. label Aug 5, 2026
@AppVeyorBot

Copy link
Copy Markdown

Build nut 2.8.5.5032-master completed (commit fd145a835c by @Marcelo-PX)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI For good or bad, machine tools are upon us. Humans are still the responsible ones. serial port 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

Status: Done

Development

Successfully merging this pull request may close these issues.

nutdrv_qx (Megatec) errors out after reading UPS data on Windows over USB CDC/COM

3 participants