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
Add support for Hunnox HNX-850 #638
Conversation
d8eaf12
to
88ad313
Compare
Thanks for the patch, @marianojan. So just one thing for now... basically the working sequence is:
right? |
Hello @zykh! You are right, the working sequence is as you expressed it. I don't have a great knowledge about the project, so if something is misplaced or has to be done in another way, please tell me. Thank you for taking a look at it! |
drivers/nutdrv_qx.c
Outdated
@@ -790,7 +841,7 @@ static int krauler_command(const char *cmd, char *buf, size_t buflen) | |||
} | |||
|
|||
/* Fabula communication subdriver */ | |||
static int fabula_command(const char *cmd, char *buf, size_t buflen) | |||
static int _fabula_command(const char *cmd, char *buf, size_t buflen, char hunnox_patch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not worth adding a hunnox_patch
parameter which is only for Hunox 850. A function should be use for other protocols. Do you have any other options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that duplicating the entire function was a bad idea. But maybe it's the safest way to avoid problems for other UPSs? What do you recommend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My stance on this wavered over time, but in the end I guess making an explicit "hunnox_command" that happens to repeat some of the other code can be more readable and maintainable, and would clearly "protect" support of different devices from these and later changes in not-their subdriver.
It may be or not be possible and practical that shared code can be de-duplicated into new routines called from various places, as a separate effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a ARES AR265i. It may also use this subdriver. I hope this pull request be merged.
Thanks to this PR I was able to get data from my newly bought UPS! I'm not sure which model it really is but it's sold as "Ups Energit Iron Guardian 800 Plcd Dispay Usb Monofasica" (with spelling errors and all) and comes with software branded as "UPSmart v1.5" for redhat and I 1) don't own a CD-R and 2) use Ubuntu. |
This worked perfectly for my "Powercool Smart UPS 1200VA". |
This comment has been minimized.
This comment has been minimized.
Was this work done by/for/on-behalf of a vendor (e.g. Hunnox)? If so, please update the docs/acknowledgements.txt file as well. |
drivers/nutdrv_qx.c: rectify indentations
drivers/nutdrv_qx_hunnox.h
Outdated
@@ -0,0 +1,29 @@ | |||
/* nutdrv_qx_zinto.h - Subdriver for Zinto protocol based UPSes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marianojan : if this was adapted from a subdriver for another UPS model, worth mentioning taht but the title here should point to Hunnox ;)
drivers/nutdrv_qx_hunnox.c
Outdated
@@ -0,0 +1,137 @@ | |||
/* nutdrv_qx_zinto.c - Subdriver for Zinto protocol based UPSes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marianojan : if this was adapted from a subdriver for another UPS model, worth mentioning taht but the title here should point to Hunnox ;)
Is this going to get merged? |
Updated file heading
Updated file heading
Err on the safer side of changing behavior for non-hunnox device support, limit the applicability of langid_fix (per PR networkupstools#638 review cautionary comments). Maybe the proposed original change did apply to other devices that support the fix-up, but that should be evaluated separately and this new constraint reversed if needed.
Basically mention updates from Hunnox subdriver and protocol addition (PR networkupstools#638).
Some more keywords to the dictionary from PR networkupstools#638
This comment has been minimized.
This comment has been minimized.
Thanks @marianojan for the contribution and @panoti for review and highlighting possible sore points. |
…mand() without using a mixed routine, to ensure no impact on currently supported devices
… choice of usb_get_string{,_simple}() based on langid_fix alone
…d to support more than Hunnox branded devices
…r (and use of) "hunnox_patch" leaving the markers in comments for refactoring
…g the buflen to 102 bytes
This comment has been minimized.
This comment has been minimized.
…la-hunnox" to "hunnox"
… PR networkupstools#638 for "hunnox" support
…(USB protocol subdriver vs. "hunnox_subdriver" for device data mapping)
This comment was marked as duplicate.
This comment was marked as duplicate.
|
||
{ "device.mfr", 0, NULL, "FW?\r", "", 39, '#', "", 1, 15, "%s", QX_FLAG_STATIC | QX_FLAG_TRIM, NULL, NULL, NULL }, | ||
{ "device.model", 0, NULL, "FW?\r", "", 39, '#', "", 17, 26, "%s", QX_FLAG_STATIC | QX_FLAG_TRIM, NULL, NULL, NULL }, | ||
{ "ups.firmware", 0, NULL, "FW?\r", "", 39, '#', "", 28, 37, "%s", QX_FLAG_STATIC | QX_FLAG_TRIM, NULL, NULL, NULL }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aquette: going over recent merges, and given your earlier comments about mfr/model/serial in #807 (comment) - should these fields here be removed also (does something else handle them by default)? And/or apply some same namespace (ups vs device) here?
Sometimes it won't start
And works again only after re-attaching usb cable |
This PR adds support for the Hunnox HNX-850 UPS.
It's a special case for the
fabula
subdriver, with a required lockstep for the commands. For this UPS, if the commands are not sent in a specific order, the output status remains unchanged.To make it work, my
ups.conf
file, has the following configuration:Also I had to add a new flag,
noscanlangid
because this scan hangs the USB endpoint of the UPS.I hope it can help someone else who has this brand of UPS.