Skip to content

Disable mge-xml.c::convert_deci() that was needed a decade ago #394

Merged
aquette merged 10 commits intonetworkupstools:masterfrom
jimklimov:mge-deci
Mar 1, 2017
Merged

Disable mge-xml.c::convert_deci() that was needed a decade ago #394
aquette merged 10 commits intonetworkupstools:masterfrom
jimklimov:mge-deci

Conversation

@jimklimov
Copy link
Member

@jimklimov jimklimov commented Feb 20, 2017

Made a debug level 5 notice for end-users, and bumped subdriver version for clarity.

Note: version bump from 0.26 to 0.28 accounts for another recent PR against this file that bumps it to 0.27.

@clepple
Copy link
Member

clepple commented Feb 20, 2017

I am not too familiar with the XML-capable devices to know how common this problem is, but I don't know if people will think to turn the debug log up to 5 to debug this.

What about a one-time log message at LOG_NOTICE?

Also, we have the GitHub issue URL set in configure.ac, so you could add that to the log message with the PACKAGE_BUGREPORT macro.

@jimklimov
Copy link
Member Author

Thanks for the suggestion, trying it out now....

Copy link
Member

@aquette aquette left a comment

Choose a reason for hiding this comment

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

seems good to me.
Few comments for the records:

  • the split was in 2007
  • 3ph units from MGE went back to Schneider ; as discussed and reported by jim, not sure if the recent units (such as Galaxy Vx) still use XML/PDC... I've asked a friend to see. I've found no DDL for XML, so jim's approach is good.
  • 1ph went with Eaton, were we still have XML/PDC, though soon to be set as legacy.

@jimklimov
Copy link
Member Author

jimklimov commented Feb 27, 2017

As a final round at this "one-line deprecation grown too big", I've made a friendly gesture towards legacy users by offering a configuration toggle to enable the decimation conversion routine at run-time without rebuilding, and tested that it works like advertised (errr, documented) :)

Copy link
Contributor

@zykh zykh left a comment

Choose a reason for hiding this comment

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

Hey, @jimklimov, what just happened (6268006)?

Apart from that, just one observation: why not a simpler VAR_FLAG for do_convert_deci? Then there would be no need to check and store the value, a simple testvar("do_convert_deci") in mge-xml.c's convert_deci() would be enough.
Everything else looks good to me (well, before 6268006).

@aquette
Copy link
Member

aquette commented Feb 28, 2017

@jimklimov : seconding @zykh in the 2 last comments:

  • do_convert-deci should use a VAR_FLAG, which exists for such cases (presence of the flag mean true/on ; absence implies false/off)
  • there was a wrong rebase on FTY branch which pulls all the DMF code and more. please revert.

@jimklimov
Copy link
Member Author

jimklimov commented Feb 28, 2017

Yuikes, seems the github web interface fracked something up :\ ETOOMANYREPOS

UPDATE: It seems that the web-based conflict resolution (after posting a PR to our project fork) rebased the whole original branch (in github's copy of my repo) over that target branch.

Regarding VAR_FLAG : do I understand correctly that it has different semantics from the toggle I've added? Meaning, present in config/CLI with (or without) any value means TRUE always?

@jimklimov
Copy link
Member Author

Force-flushed back the branch to my latest state from yesterday.

@jimklimov
Copy link
Member Author

Implemented the suggestion from @zykh so now do_convert_deci is a flag and tested that presence or lack of any value does not matter: present == set. And that it still works, e.g.:

$ sudo ./drivers/netxml-ups -a 93pm0 -u root -DDDDD -x do_convert_deci=off 2>&1 | ggrep -B5 -A5 -i 'deci'
...
   0.000000	[D5] send_to_all: SETINFO driver.parameter.do_convert_deci "off"
...
   0.719646	[D3] mge_xml_startelm_cb: name <OBJECT> (parent = 300, state = 301)
   0.719689	[D3] mge_xml_cdata_cb: cdata [8] (state = 301)
   0.719725	[D3] mge_xml_endelm_cb: name </OBJECT> (state = 301)
   0.719767	[D3] -> XML variable UPS.PowerConverter.Input[1].Phase[1].Current [8] maps to NUT variable input.L1.current
   0.719812	convert_deci() is now deprecated, so values from XML are normally not decimated. This driver instance has however configured do_convert_deci in your ups.conf, so this behavior for old MGE NetXML-capable devices is preserved.
   0.719913	[D4] -> XML variable UPS.PowerConverter.Input[1].Phase[1].Current [8] which maps to NUT variable input.L1.current was converted to value 0.8 for the NUT driver state
   0.719949	[D5] send_to_all: SETINFO input.L1.current "0.8"
$ sudo ./drivers/netxml-ups -a 93pm0 -u root -DDDDD  2>&1 | ggrep -B5 -A5 -i 'deci'
...
   0.735048	[D3] mge_xml_cdata_cb: cdata [8] (state = 301)
   0.735086	[D3] mge_xml_endelm_cb: name </OBJECT> (state = 301)
   0.735129	[D3] -> XML variable UPS.PowerConverter.Input[1].Phase[1].Current [8] maps to NUT variable input.L1.current
   0.735178	convert_deci() is now deprecated, so values from XML are not decimated. If you happen to have an old MGE NetXML-capable device that now shows measurements 10x too big, and a firmware update does not solve this, please inform NUT devs via the issue tracker at https://github.com/networkupstools/nut/issues with details about your hardware and firmware versions. Also try to enable do_convert_deci in your ups.conf
   0.735220	[D5] convert_deci() is now deprecated, so value '8' is not decimated. If this change broke your setup, please see details logged above.
   0.735253	[D4] -> XML variable UPS.PowerConverter.Input[1].Phase[1].Current [8] which maps to NUT variable input.L1.current was converted to value 8 for the NUT driver state
   0.735288	[D5] send_to_all: SETINFO input.L1.current "8"

jimklimov and others added 9 commits February 28, 2017 11:27
Left the old code in place, commented away. If devices with old NetXML pop up (those which served in XML 10x the measured value), fixes can be applied based on voltage and amps and power (order of magnitude comparison), or perhaps model names and/or firmware versions or release dates, if we get those.
…th debug level 5) so we get reports if deprecation bites anyone in reality
…it once visibly with details like issue tracker URL, and in regular debug messages refer to that line
…) behavior at run-time, with a configuration toggle (documented)
@zykh
Copy link
Contributor

zykh commented Feb 28, 2017

(if this PR gets merged before #390, we have to remember to adjust subdriver version number)

@jimklimov
Copy link
Member Author

Of course, I'd take care of that. Also, github will complain about a merge conflct ;)

@aquette aquette merged commit b09e77a into networkupstools:master Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants