Skip to content

Update nutdrv_qx_megatec.c #2980

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

Merged
merged 6 commits into from
Jun 12, 2025
Merged

Update nutdrv_qx_megatec.c #2980

merged 6 commits into from
Jun 12, 2025

Conversation

gksmsk
Copy link
Contributor

@gksmsk gksmsk commented Jun 4, 2025

Added a parameter battery.runtime

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.

  • 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

  • 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.

Added a parameter battery.runtime
@jimklimov jimklimov mentioned this pull request Jun 4, 2025
20 tasks
@jimklimov jimklimov added enhancement Qx protocol driver Driver based on Megatec Q<number> such as new nutdrv_qx, or obsoleted blazer and some others labels Jun 4, 2025
@jimklimov jimklimov added this to the 2.8.4 milestone Jun 4, 2025
@jimklimov
Copy link
Member

jimklimov commented Jun 4, 2025

@gksmsk : thanks for the contribution! I've updated the code a bit (regarding comments and a NEWS announcement), and found that the reply in your example seems shorter than documented in other protocol dialects (38 vs 39 bytes, including the finishing '\r'). Can you please double-check which number is correct for your device (e.g. there is no space padding after the zeroes in the version)?

If it is really 38, we need to check the parsing code to see if all 5 of these table lines need to be updated to expect the shorter reply as valid (and the last line to pick one byte less into firmware version).

@AppVeyorBot
Copy link

@gksmsk
Copy link
Contributor Author

gksmsk commented Jun 5, 2025

what protocol dialect, Q1 I or others?

@gksmsk
Copy link
Contributor Author

gksmsk commented Jun 5, 2025

we already tested it. That's no problem.

@jimklimov
Copy link
Member

what protocol dialect, Q1 I or others?

The I query is seen in many nutdrv_qx_*.c mappings, probably copy-pasted from some original file as people created new protocol support - all those entries have same comments and a 39-byte expected transfer size.

I can't rule out that this response may in fact differ per dialect/vendor/model though, and that maybe the copy-pasted table entries are wrong for some devices and don't in fact work for somebody, but I did not quickly find any complaints about these fields being missing/broken, so...

…o "I" in nutdrv_qx::megatec subdriver [networkupstools#2980]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to gksmsk/nut that referenced this pull request Jun 8, 2025
…er for "I" query, and any length after byte 28 till end of reply is ups.firmware [networkupstools#2980]

Signed-off-by: Jim Klimov <[email protected]>
…er for "I" query, and any length after byte 28 till end of reply is ups.firmware [networkupstools#2980]

Also finally bump the subdriver version for this PR

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
Copy link
Member

jimklimov commented Jun 8, 2025

I've quickly reviewed the structure spec at drivers/nutdrv_qx.h: it seems the number in table is "min length" of the reply, so I put 38 here (and any 39-byte or longer replies should also be ok). Also I set 0 as end of the last expected field, so the ups.firmware version would be as long as the end of device response is.

@gksmsk : can you please re-test updated code with your device (which model is it BTW)? Also, can you please post a data dump from upsc or tools/nut-ddl-dump.sh for the NUT DDL library? Maybe even two dumps, with a recent NUT release or master-branch code, and with this change in place, so we can see the difference?

Thanks for the PR!

@AppVeyorBot
Copy link

@jimklimov
Copy link
Member

@gksmsk : have you tested this recent update over the past days? Does the driver behave better (or not-worse) than before it?

Did you have any chance to test with different devices talking a Qx dialect (especially regarding non-regression compared to older NUT builds)?

@jimklimov jimklimov merged commit e8c6124 into networkupstools:master Jun 12, 2025
30 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Qx protocol driver Driver based on Megatec Q<number> such as new nutdrv_qx, or obsoleted blazer and some others
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants