[Nut-upsdev] Patching CyberPower UPS drivers

[Charles Lepple]

On Jan 19, 2016, at 9:37 AM, Ben Kamen <ben at benkamen.net> wrote:
> 
> Hey there,
> 
>  I got a new UPS (CyberPower PR1500LCDRTXL2Ua) and found the serial drivers don't quite work...
[...]
> Essentially, I rewrote powpan_status so it better tolerates strings of different lengths (although I only have the 1 UPS right now, so I can't test against another and different string lengths)
> 
> I had to make the serial read much longer so it grabs 55bytes instead of the original short list.
> 
>>  9.843588     read: (55 bytes) => 23 49 31 31 38 2e 30 4f 31 31 38 2e 30 4c 30 31 37 42
>>   9.843663      31 30 30 54 30 32 35 48 30 36 30 2e 30 46 30 36 30 2e 30 52 30 35 31 51 30
>>   9.843695      30 32 53 90 84 c0 88 80 57 00 81 0d
> 
> compared to the original string support in the file, there's now 2 fields of binary data (in ASCII Protocol? seriously? smh)

One thing that you can do to try and cover all the bases with the string lengths is to have "#define TESTING" or something, and have a test function that cycles through the various reply strings, rather than reading from serial. That way, someone without any of these units can test on their own system.

For instance:

https://github.com/networkupstools/nut/blob/master/drivers/nutdrv_qx.c#L2124

reads from an array defined in the sub-driver:

https://github.com/networkupstools/nut/blob/master/drivers/nutdrv_qx_q1.c#L87

> and the S field on this UPS has 4 bytes instead of just 2.
> 
> (not that it matters I guess... but now it makes me wonder what all the extra information is)
> 
> (I also guess someone has tried reaching out to CyberPower for docs and been turned down?)

Not always the case, but usually we ask for permission to publish the protocols on the NUT website (to assist future developers), and that is often met with silence.

> Anyway -- in rewriting this, the only part I have left to do to where I'm happy is setting the timeout.
> 
> Instead of asking for N number of chars and then timing out after a long time, I'd like to ask it for a string (up to the 0x0D).

There are also ser_get_line() and ser_get_line_alert():

https://github.com/networkupstools/nut/blob/master/drivers/serial.c#L369

Not sure if there was a specific reason why those weren't used originally, or if this driver predates them.

> I haven't written that in yet, but will if someone says there's another way -- or I have to do it that way....
> 
> so that an incomplete string after time causes a timeout but a string that comes in reasonably soon will be collected -- but not using a fixed number (since it seems these UPSs now have variability in string length between models) and then just parse the string.
> 
> The parsing part works. I just need to alter the collection part so the timeouts work differently.

I definitely like your approach. Thanks for moving some of the "magic numbers" to named constants, as well.

A few items:

* Since v2.6.5, some of the string functions were renamed. rtrim() -> str_rtrim(), for instance.
* If you are taking out a block of code, "#if 0" is better than comment characters (which don't nest), and removing it completely is even better (fewer false positives when searching). If nothing else, it's in version control.

Diff attached.

Thanks,

-- 
Charles Lepple
clepple at gmail


-------------- next part --------------
A non-text attachment was scrubbed...
Name: powerp-txt.c.diff
Type: application/octet-stream
Size: 9129 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20160128/a3f287bf/attachment.obj>