Changes to add support for AMETEK Powervar UPM as usb-hid UPS. by andrewmccartney · Pull Request #733 · networkupstools/nut

andrewmccartney · 2019-09-30T17:26:24Z

Includes adding a report index to libusb.c in order to handle the composite device in the UPM.

The original code has the report index hard coded to zero. By using a variable for the report index it can be set to the desired report index based on detecting connected devices that use multiple report indexes.

All other changes are the standard items for introducing a new HID device.

…ng a report index to libusb.c in order to handle the composite device in the UPM.

lgtm-com · 2019-09-30T17:37:49Z

This pull request introduces 1 alert when merging adb504d into 1b217f4 - view on LGTM.com

new alerts:

1 for FIXME comment

andrewmccartney · 2019-10-01T15:23:18Z

The BUILD_TYPE=default failed for a missing dependency in "make install". I don't think any of my changes could have caused this, any ideas why the build failed like this?

clepple · 2019-10-03T12:22:31Z

The BUILD_TYPE=default failed for a missing dependency in "make install". I don't think any of my changes could have caused this, any ideas why the build failed like this?

@jimklimov do you have a way to retry that build on Travis?

aquette · 2019-10-03T13:17:37Z

@clepple theoretically you can access by clicking the link and restart build (which I just did)

andrewmccartney · 2019-10-07T19:47:16Z

Thank you to @aquette and @clepple for getting the build issue sorted out. Now that it builds without conflicts is there anything else I need to do in order to get the changes merged into the master?

jimklimov

Can't comment on actual driver content (mostly it is HID mapping... and whether correct flags, string-names and other fields are used), but structurally the change looks good :)

Note this will slightly conflict (at least in Makefile and likely in subdriver list) with changes from PR #807

aquette · 2020-10-08T11:41:29Z

+
+static hid_info_t powervar_hid2nut[] = {
+
+  { "ups.powersummary.iproduct", 0, 0, "UPS.PowerSummary.iProduct", NULL, "%.0f", 0, NULL },


same comments as #807 on indexed string (mfr, model, serial)

@aquette Are you saying that we don't need to have the iProduct, iSerialNumber & iManufacturer entries?

Understood. I have removed them and diid a build and an install locally and all is well. I'll wait to upload until we have completed the other review items.

Ok.
Please send in an upsc output once good, to confirm everything is fine

UPSC output with all changes included.

amccartney@amccartney-OptiPlex-3010:/lib/nut$ upsc UPM@localhost
Init SSL without certificate database
battery.capacity: 100
battery.capacitymode: 2
battery.charge: 100
battery.designcapacity: 100
battery.ioeminformation: AMETEK-POWERVAR Cor
battery.runtime: 360000
battery.type: PbAc
device.mfr: AMETEK-POWERVAR Corporation
device.model: UPM UPS (v01.51, Nov 3 2018)
device.serial: 5014403R-1740135
device.type: ups
driver.flag.pollonly: enabled
driver.name: usbhid-ups
driver.parameter.pollfreq: 30
driver.parameter.pollinterval: 2
driver.parameter.port: auto
driver.parameter.synchronous: no
driver.version: 2.7.4-732-gadb504d0
driver.version.data: Powervar HID 0.19
driver.version.internal: 0.43
ups.mfr: AMETEK-POWERVAR Corporation
ups.model: UPM UPS (v01.51, Nov 3 2018)
ups.powersummary.imanufacturer: 2
ups.powersummary.iproduct: 2
ups.powersummary.iserialnumber: 4
ups.productid: 0002
ups.serial: 5014403R-1740135
ups.status: OL
ups.vendorid: 4234

thx @andrewmccartney

few more comments in light of this (@jimklimov @clepple and others may complete):

battery.capacitymode: 2
** capacitymode doesn't exist (as per https://github.com/networkupstools/nut/blob/master/docs/nut-names.txt#L350)
** battery.capacity.mode would be more appropriate at first sight (new var, need RFC), but depends on what the values meaning
** for the value, there should be something more friendly to provide (2 == "some explicit string")

battery.designcapacity
** battery.charge.design would be more appropriate (new var, need RFC), considering the value

battery.ioeminformation
** non existing variable. battery.mfr or device.mfr.oem or ?

device.model: UPM UPS (v01.51, Nov 3 2018)
** the part between parenthesis should be split between device.firmware and ups.mfr.date

ups.powersummary.imanufacturer & ups.powersummary.iproduct & ups.powersummary.iserialnumber
** to be removed, as per my previous comment. these are just the indexes of the strings, already published as device.{mfr, model, serial}

side comment: I see no load, realpower nor power data, that would give visibility on the power consumption on the UPS output. Nothing avail?

cheers,
Arno

@aquette @jimklimov with regards to these changes, if I am understanding you correctly I would need to change the Powervar UPS HID behavior to accomplish this. This would be a UPS firmware change and at this time that is not a possibility. Please advise me if I am not understanding what you are asking here.

@andrewmccartney no need to modify your FW for the above (possibly apart from the realpower, power and load Usage.
The other comments need just the NUT subdriver modification.
device.firmware could also be in its own "UPS.PowerSummary.iVersion", but can also be split from iModel using the callback function:

modify powervar_format_model() so that it strips the parenthesis and just return the model

leave a mapping on iProduct, but for device.firmware and set a callback function to extract the firmware info
{ "device.firmware", 0, 0, "UPS.PowerSummary.iProduct", NULL, "%.0f", 0, stringid_to_firmware},

same for ups.mfr.date...

lgtm-com · 2020-10-09T09:57:54Z

This pull request introduces 1 alert when merging b25f8b5 into 6eceb3f - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2020-10-09T13:10:16Z

This pull request introduces 1 alert when merging 1c75f81 into ab955fd - view on LGTM.com

new alerts:

1 for FIXME comment

jimklimov · 2020-10-12T09:57:13Z

There were some good comments in discussion about ensuring an UPS poweroff with some clever scenarios, which currently should be set up outside the USB HID connection. I think the manpage needs to mention that, perhaps referencing the vendor documentation (if there are some stable URLs possible) or spelling out the interesting parts in NUT docs/ codebase.

This also brought to my attention that the PR does not update docs/man/usbhid-ups.txt to mention the subdriver support being added.

Update the docs about new subdriver

jimklimov · 2020-10-12T12:46:48Z

One more point came up, it is worth mentioning the vendors' efforts at https://github.com/networkupstools/nut/blob/master/docs/acknowledgements.txt

…ded company/contributor info. Updated docs/man/usbhid-ups.txt

lgtm-com · 2020-10-12T14:55:03Z

This pull request introduces 1 alert when merging e266362 into f83ca0b - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2020-10-12T15:12:19Z

This pull request introduces 1 alert when merging 29f78b5 into f83ca0b - view on LGTM.com

new alerts:

1 for FIXME comment

jimklimov · 2020-10-12T16:59:41Z

Thanks for getting to the docs too! Dictionary wants an update per https://travis-ci.org/github/networkupstools/nut/jobs/735038633#L870 :

  ASPELL   Spell checking on usbhid-ups.txt
FAILED : Aspell reported errors here:
----- vvv
342:& AMETEK 23 8: ATEK, AMMETER, AMTRAK, EMETIC, ELTEK, AMMETERS, AMT, AMOK, AMATEUR, UNITEK, AMATI, AMIDE, AMITY, EMOTE, AZTEC, AMMETER'S, GAMETIC, ACETIC, AMIDES, EMOTED, EMOTES, AMIDE'S, AMITY'S
537:& Powervar 14 15: Power var, Power-var, PowerVS, Powervalue, Powers, Power, Power's, However, Powerdev, PowerOff, Powerful, Poweroff, Perv, Weaver
684:& UPM 12 24: UMP, PM, UM, UP, APM, UPI, PPM, WPM, CPM, UPC, UPS, RPM
----- ^^^

Add new product name and vendor keywords

Rearranged lines to keep "all" block together, and finish the list item with a comma without much context change ;)

lgtm-com · 2020-10-12T17:17:01Z

This pull request introduces 1 alert when merging d4172dc into f83ca0b - view on LGTM.com

new alerts:

1 for FIXME comment

andrewmccartney · 2020-10-13T17:58:04Z

@jimklimov I'm new to the process here for merging changes. At this stage the changes are approved - correct? is there anything else I need to do to have them merged?

lgtm-com · 2020-11-06T21:37:33Z

This pull request introduces 1 alert when merging a567c93 into fe1ad77 - view on LGTM.com

new alerts:

1 for FIXME comment

Add vendor contribution from AMETEK Powervar for UPM series

jimklimov · 2020-11-07T21:43:07Z

Looks good to me, if other NUT maintainers do not voice objections I plan to merge this PR in a week or so.

Thank you very much for the contribution, and for patience with us :) Much appreciated!

jimklimov · 2020-11-07T21:47:14Z

+* link:https://www.ametek.com/products/businessunits/powersystemsandinstruments/powervar[AMETEK
+Powervar], through Andrew McCartney, has added support for all AMETEK Powervar
+UPM models as usb-hid UPS.
+


Note @andrewmccartney : I took the liberty to stub an acknowledgements entry here. Feel free to modify if I got some names or spellings wrong.

lgtm-com · 2020-11-07T21:54:21Z

This pull request introduces 1 alert when merging 55778c9 into 3f3a959 - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2020-11-09T20:31:56Z

This pull request introduces 1 alert when merging 501ae3a into 26c8994 - view on LGTM.com

new alerts:

1 for FIXME comment

lgtm-com · 2020-11-29T13:39:03Z

This pull request introduces 1 alert when merging 4fd8a45 into 3a4e88d - view on LGTM.com

new alerts:

1 for FIXME comment

jimklimov · 2021-01-28T09:15:29Z

Since other NUT maintainers did not voice objections, I plan to merge this after the build completes which re-syncs the PR code to current master branch state.

lgtm-com · 2021-01-28T09:28:29Z

This pull request introduces 1 alert when merging ecce15e into 9defa7a - view on LGTM.com

new alerts:

1 for FIXME comment

Changes to add support for Powervar UPM as usb-hid UPS. Includes addi…

adb504d

…ng a report index to libusb.c in order to handle the composite device in the UPM.

andrewmccartney changed the title ~~Changes to add support for AMETEK Powervar UPM as usb-hid UPS. Includes addi…~~ Changes to add support for AMETEK Powervar UPM as usb-hid UPS. Sep 30, 2019

jimklimov approved these changes Oct 7, 2020

View reviewed changes

Comment thread drivers/powervar-hid.c

aquette reviewed Oct 8, 2020

View reviewed changes

Comment thread drivers/powervar-hid.c

Merge branch 'master' into master

b25f8b5

Merge branch 'master' into master

1c75f81

andrewmccartney added 2 commits October 12, 2020 09:39

Merge branch 'master' into master

e266362

Changes per NUT review comments. Removed info fields from polling. Ad…

e4cb867

…ded company/contributor info. Updated docs/man/usbhid-ups.txt

Merge branch 'master' of https://github.com/andrewmccartney/nut

29f78b5

jimklimov added 2 commits October 12, 2020 19:02

Update nut.dict for PR networkupstools#733

c311835

Add new product name and vendor keywords

Update usbhid-ups.txt

d4172dc

Rearranged lines to keep "all" block together, and finish the list item with a comma without much context change ;)

andrewmccartney added 3 commits November 6, 2020 14:55

Final clean up and edits for Powervar UPM support

679eee3

Merge branch 'master' of https://github.com/andrewmccartney/nut

48f9c34

Merge branch 'master' into master

a567c93

jimklimov and others added 2 commits November 7, 2020 22:41

Update acknowledgements.txt

05f91b6

Add vendor contribution from AMETEK Powervar for UPM series

Merge branch 'master' into master

55778c9

jimklimov reviewed Nov 7, 2020

View reviewed changes

Merge branch 'master' into master

501ae3a

Merge branch 'master' into master

4fd8a45

Merge branch 'master' into master

ecce15e

jimklimov added the ready / gonna merge The PR is in final cycles leading to merge unless someone logs an objection before we hit the button label Mar 2, 2021

jimklimov merged commit 8ba451e into networkupstools:master Mar 2, 2021

clepple mentioned this pull request Jul 1, 2021

Fix GetValue() to correctly extract values from a report when compiled in LP64 mode (usbhid-ups) #1040

Merged


		static hid_info_t powervar_hid2nut[] = {

		{ "ups.powersummary.iproduct", 0, 0, "UPS.PowerSummary.iProduct", NULL, "%.0f", 0, NULL },

Uh oh!

Conversation

andrewmccartney commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com Bot commented Sep 30, 2019

Uh oh!

andrewmccartney commented Oct 1, 2019

Uh oh!

clepple commented Oct 3, 2019

Uh oh!

aquette commented Oct 3, 2019

Uh oh!

andrewmccartney commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimklimov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aquette Oct 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgtm-com Bot commented Oct 9, 2020

Uh oh!

lgtm-com Bot commented Oct 9, 2020

Uh oh!

jimklimov commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimklimov commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com Bot commented Oct 12, 2020

Uh oh!

lgtm-com Bot commented Oct 12, 2020

Uh oh!

jimklimov commented Oct 12, 2020

Uh oh!

lgtm-com Bot commented Oct 12, 2020

Uh oh!

andrewmccartney commented Oct 13, 2020

Uh oh!

lgtm-com Bot commented Nov 6, 2020

Uh oh!

jimklimov commented Nov 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com Bot commented Nov 7, 2020

Uh oh!

lgtm-com Bot commented Nov 9, 2020

Uh oh!

lgtm-com Bot commented Nov 29, 2020

Uh oh!

jimklimov commented Jan 28, 2021

Uh oh!

lgtm-com Bot commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

andrewmccartney commented Sep 30, 2019 •

edited

Loading

andrewmccartney commented Oct 7, 2019 •

edited

Loading

aquette Oct 10, 2020 •

edited

Loading

jimklimov commented Oct 12, 2020 •

edited

Loading

jimklimov commented Oct 12, 2020 •

edited

Loading