WIP: Support for Voltronic Axpert P30 protocol inverters #1406
Conversation
Add support for (some) solar devices manufactured by Voltronic Power. Also, add CRC routines common to various Voltronic Power devices.
Since some characters are 'escaped', the remainder is affected too: if one of the 'escapes' happens, the remainder won't be 0 even in case of a valid CRC. Therefore, simply calculate the CRC and compare it to the (expected) CRC-bytes of input.
Apparently also '2' is a valid value for inverter direction (QPIGS's reply @ 130), so move it to a standalone preprocess function which accepts also that (still unmapped) value.
Add initial support/mockups for commands: - QEBGP; - ABGP<n>; - LBF<n>; - QACCHC.
Since other variables depend on the actual (updated) value of device date and time, remove the QX_FLAG_SEMI_STATIC flag from device.date and device.time so that the driver updates them every time a QX_WALKMODE_FULL_UPDATE is executed.
Adds support for the Voltronic Axpert inverters based on the P30 protocol. Updated the nutdrv_qx driver to support the optional sending of commands protected by CRC. Initial support for query commands. Based on work done in the Voltronic Sunny driver.
There was a problem hiding this comment.
At least for changes in existing codebase, there are mismatches that gotta fail compilations ;)
Try to iterate with BUILD_TYPE=fightwarn-clang ./ci_build.sh and then just make/rinse/repeat to iterate small fixes until no warnings remain, then polish with BUILD_TYPE=fightwarn-gcc ./ci_build.sh
| @@ -928,7 +935,7 @@ static int krauler_command(const char *cmd, char *buf, size_t buflen) | |||
|
|
|||
| int retry; | |||
|
|
|||
| if (strcmp(cmd, command[i].str)) { | |||
| if (strncmp(cmd, command[i].str, cmdlen)) { | |||
There was a problem hiding this comment.
Sanity-checking this change here and in many other routines: should it be strncmp to only compare the first N characters of the cmd? By the new argument, seems so, but still...
Avoiding an unintended strncmp("shutdown", "shut up!", 4) == true sort of mishap ;)
| @@ -3915,8 +3920,8 @@ static int qx_process_answer(item_t *item, const size_t len) | |||
|
|
|||
| /* Short reply */ | |||
| if (item->answer_len && len < item->answer_len) { | |||
| upsdebugx(2, "%s: short reply (%s)", | |||
| __func__, item->info_type); | |||
| upsdebugx(2, "%s: short reply (%s) %d<%d", | |||
There was a problem hiding this comment.
for size_t len formatting symbol is %zu not %d
| @@ -3966,12 +3972,12 @@ int qx_process(item_t *item, const char *command) | |||
|
|
|||
| /* Prepare the command to be used */ | |||
| memset(cmd, 0, cmdsz); | |||
| snprintf(cmd, cmdsz, "%s", command ? command : item->command); | |||
| snprintf(cmd, cmdsz, "%s%n", command ? command : item->command, &cmd_len); | |||
There was a problem hiding this comment.
Cute trick with %n, but why not just check the return value of snprintf (error if negative, number of chars without the ending null-byte otherwise)?
There was a problem hiding this comment.
Overflow, mainly.
s(n)printf from what I can see returns the theoretical length assuming an infinite buffer, rather than the actual length constrained by the buffer.
Ideally the snprintfs should be memcpys, as the buffer could be either a zero terminated string, or a non zero terminated buffer.
The non-zero terminated buffer part is because of the voltronic's CRC implementation, which is two binary possibly-zero digits before the \r.
This is why we need all the changes to what was until now just a simple string of non-zero characters.
| @@ -91,7 +91,7 @@ typedef struct item_t { | |||
| int (*preprocess_command)(struct item_t *item, char *command, const size_t commandlen); | |||
There was a problem hiding this comment.
With the change to return lengths, this should generally be not an arbitrary sized int anymore, but guaranteed size_t or less (e.g. ssize_t for signed values including errors)
| @@ -154,7 +156,7 @@ static struct { | |||
|
|
|||
| /* == Support functions == */ | |||
| static int subdriver_matcher(void); | |||
| static ssize_t qx_command(const char *cmd, char *buf, size_t buflen); | |||
| static int qx_command(const char *cmd, size_t cmdlen, char *buf, size_t buflen); | |||
There was a problem hiding this comment.
Why the change from ssize_t to int -- typo? Especially since the implementation remains with ssize_t...
There was a problem hiding this comment.
Very old branch, tested against 2.7.4. I'm trying to keep the commit attributions of the CRC implementation code, not sure if git is going to figght with me on that.
I've updated the patch to master, going to commit it after some food :)
jimklimov
added
Qx protocol driver
|
This is a Work-In-Progress based on a very old branch containing code for a sister driver, on the case getting this to compile clean for master, and then 2.7.x. |
|
Replaced by #1407 |
Adds support for the Voltronic Axpert inverters based on the P30 protocol.
Updated the nutdrv_qx driver to support the optional sending of commands
protected by CRC.
Initial support for query commands. Based on work done in the Voltronic
Sunny driver.
Still TODO: