Nut upsdev - May 2015 - nutdrv_qx interface change proposal item_t::preprocess

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,
I would like to propose an interface change/extension, in order to be
able to clearly differ from a PRE_SEND and a POST_RECEIVE
item_t->preprocess-ing calls.

IMHO there is no option to differ from item->preprocess(..), called from
[1] and called from [2], at the moment. My idea is to extend the
item_t->preprocess(..) with an additional argument about the calling
point of the method like this [3].

The changes in the main nutdrv_qx module would be small [4], but every
calling point would have to be extended by the additional argument.
Pleas let me know what you'r thinking.

Best, Nick

[1]
https://github.com/networkupstools/nut/blob/master/drivers/nutdrv_qx.c#L1393
[2]
https://github.com/networkupstools/nut/blob/master/drivers/nutdrv_qx.c#L2674
[3]
https://github.com/nickma82/nut/blob/nutdrv_qx_interface_change/drivers/nutdrv_qx.h#L103
[4]
https://github.com/nickma82/nut/commit/ce330a3873fd0a9243a61258db243a6eee327e65
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJVQ38GAAoJEDMnKdeJttQVxagP/1lOfPd0qVol9nTvLAptFr8I
GKcfAg+1LHNXZ43qAxo8ZXGEHIxmRHuKxuMBAEWKXA76zlYozOIP85GGy63rONCL
D5z4y/J8B2dgW4Q0Jtj4PsAmTvsW7yXbKvGWboAhnoTeRf5BupPB4335GGByl1HM
jQZrHhG+TvAJa/86zzL0OBNggV0SMiqyI7YEyMmR+5jz5p7g0hj+0IJQ1s90aWCM
l2S5qr+joitqWS///Ta1L4X8Slbog+/wMinX0+zzRFwpcRDTfAGARJQMVQhLU5Ue
3XPBNIMMLxpB+qga3rlzk3hVxB2+t6OEuZPkDML9KouvB4fYyXsGVMc99rkQQdCD
rk10BTDErpQ+5dReQmnKMXzGW5zqikfJ+RUEoyVp94Gq5P4umA7pzcM0dO/TICpL
Jc/pt+nj9RE2VmGdHCzsvXdJ5ErnheGmGMz1jDFq7PtMeHTlXq1aLvCbMIbksvi+
WbjdpGH/ozh9SJBytOVAV7tvKXTI0J2zIjvtk9x3SlookIxhWe63WjG5mB/wvEGl
Z89RyM3h3qNC7dUWaTnIzXQ1qmxIiPnVI0uwLpe4PGZwRwQUqtaXCcd+uVV0L06/
XlzRV5Y4oXQeNAfCDABUfpvJzS0R09oK0ADyTtTdjg+7XSRw8+occtYndzh7rjrN
1eBv4tFB5jqGULDoA/PF
=8dmy
-----END PGP SIGNATURE-----

mmh..

At the moment there are 3 distinct preprocess functions that operate
on 2 different types:
- info_rw_t - preprocess();
- item_t - preprocess();
- item_t - preprocess_answer().

For info_rw_t: range/enum values are preprocessed just to see if that
specific value is to be used or not depending on the conditions the
driver meets at runtime (e.g.: low voltage devices -> low voltage
ranges). So, in this case, directions don't seem to make sense to me.

For item_t:
- preprocess_answer() is always (and only) executed everytime the
driver reads a new 'answer' (be it as a consequence of a query or of
an instant command/setvar, no difference) from the device. So, in this
case, too, I can't see the ambiguity.
- preprocess(): this function is executed
  * for queries: to translate a value from device-form to NUT-form (so
direction is DEVICE -> NUT, i.e.: read from device the 'answer' to
that query);
  * for instant commands/setvars (QX_FLAG_CMD/QX_FLAG_SETVAR): to fill
the command to be sent to device with data from NUT-form to
device-form (so direction is NUT -> DEVICE, i.e.: write to device;
answer is expected to be either 'command accepted' or 'command
rejected').
  So direction already depends on item_t type (i.e. qxflags).

At the time the driver was written, that was enough to handle all situations.

Now, I know that there can be queries that need to be preprocessed
(QED<date> and the like, right?), but there's no such a thing in NUT
(i.e.: you cannot pass a value to a query directly from the command
line just like you would do with instant commands/setvars, plus the
driver preferred way of communication is through NUT variables): the
closest thing you can do is to:
1. set a var (let's say "zig.zag") to a value (so there should be
both
a bare "zig.zag" item_t with info_flags ST_FLAG_RW and a
QX_FLAG_SETVAR "zig.zag" one in nutdrv_qx) or use an internal var that
gets its value from something other;
2. use that var (either the user-provided one, "zig.zag" , or the
internal one) to fill the query (preprocess from NUT-form to
device-form and write to device) and then proceed as per other queries
(read from device the 'answer' and get the values, say
"ding.dong.1",
"ding.dong.2", ..., which may also need to be preprocessed from
device-form to NUT-form).
Thus, in the end, you may need both the NUT->device and device->NUT
preprocess functions and I don't feel enthusiastic about having big
functions that switch on direction - I already see myself lost trying
to get out of it.

What I'd do instead is to add a 3rd item_t preprocess function (be it
'preprocess_command()') that works like the preprocess_answer() one
(but in the opposite direction): this function would be called just
before the command (any command, both queries and instant
commands/setvars, no difference) would be sent to the device so that:
- queries that need to be preprocessed, can be;
- devices that require a CRC or any other weird transliteration, can
be satisfied (at this time instant commands/setvars are already filled
with user-provided data, and the same would be true for those queries
that need to be preprocessed: you'd only have to call the CRC/whatever
function before quitting the query-preprocess one).

Here's a first try at it:
https://github.com/zykh/nut/tree/nutdrv_qx_preprocess_command

Am I that far from what you want to achieve?

Hi and thank you for your effort.

You are very close to what I want to achieve, but I was already aware of
the impossibility of querying direct from a command. I wanted the
following (both would be covered by your changes):
1. preprocess an item_t in the direction: NUT->DEVICE
    in order to be able to support queries like ups.generated.yesterday
("QED<date-1><Checksum>\r") and ups.generated.today
    (note: today's generated power is even more complicated because it
can't be queried due to QED<date>, but has to be queried hour by hour
QEH<date><time>)
2. requesting data from a specific day requested from higher layers
(such as the client).

On 19/05/15 02:11, hyouko at gmail.com wrote:
> mmh..
>
> At the moment there are 3 distinct preprocess functions that operate
> on 2 different types:
> - info_rw_t - preprocess();
> - item_t - preprocess();
> - item_t - preprocess_answer().
>
> For info_rw_t: range/enum values are preprocessed just to see if that
> specific value is to be used or not depending on the conditions the
> driver meets at runtime (e.g.: low voltage devices -> low voltage
> ranges). So, in this case, directions don't seem to make sense to me.
>
> For item_t:
> - preprocess_answer() is always (and only) executed everytime the
> driver reads a new 'answer' (be it as a consequence of a query or
of
> an instant command/setvar, no difference) from the device. So, in this
> case, too, I can't see the ambiguity.
> - preprocess(): this function is executed
>   * for queries: to translate a value from device-form to NUT-form (so
> direction is DEVICE -> NUT, i.e.: read from device the 'answer'
to
> that query);
>   * for instant commands/setvars (QX_FLAG_CMD/QX_FLAG_SETVAR): to fill
> the command to be sent to device with data from NUT-form to
> device-form (so direction is NUT -> DEVICE, i.e.: write to device;
> answer is expected to be either 'command accepted' or 'command
> rejected').
>   So direction already depends on item_t type (i.e. qxflags).
Allright, so I had a misunderstanding there. What goes in the
preprocess(..) method and what is supposed to go out grinded my gears a
couple of time, even after going through the code and whose
documentations.  More verbose documentation of the preprocess(..) [1]
would be nice though.
> Now, I know that there can be queries that need to be preprocessed
> (QED<date> and the like, right?), 
Correct.
> the closest thing you can do is to:
> 1. set a var (let's say "zig.zag") to a value (so there
should be both
> a bare "zig.zag" item_t with info_flags ST_FLAG_RW and a
> QX_FLAG_SETVAR "zig.zag" one in nutdrv_qx) or use an internal var
that
> gets its value from something other;
> 2. use that var (either the user-provided one, "zig.zag" , or the
> internal one) to fill the query (preprocess from NUT-form to
> device-form and write to device) and then proceed as per other queries
> (read from device the 'answer' and get the values, say
"ding.dong.1",
> "ding.dong.2", ..., which may also need to be preprocessed from
> device-form to NUT-form).
Thank you, thats probably the way I'm going to take
here.
> Thus, in the end, you may need both the NUT->device and device->NUT
> preprocess functions and I don't feel enthusiastic about having big
> functions that switch on direction - I already see myself lost trying
> to get out of it.
I would prefer one single preprocess(..) method which handles all the
preprocessing (similar to what proposed [2]), rather than expanding the
item_t structure any further. But thats just me.
There's a lot of potential beautifying in the item_t structure (e.g.
moving all the dynamic data a sub structure leaving a single dynamic
field, building preprocessor wrappers).
> What I'd do instead is to add a 3rd item_t preprocess function (be it
> 'preprocess_command()') that works like the preprocess_answer() one
> (but in the opposite direction): this function would be called just
> before the command (any command, both queries and instant
> commands/setvars, no difference) would be sent to the device so that:
> - queries that need to be preprocessed, can be;
> - devices that require a CRC or any other weird transliteration, can
> be satisfied (at this time instant commands/setvars are already filled
> with user-provided data, and the same would be true for those queries
> that need to be preprocessed: you'd only have to call the CRC/whatever
> function before quitting the query-preprocess one).
>
> Here's a first try at it:
> https://github.com/zykh/nut/tree/nutdrv_qx_preprocess_command
That would work for me too. I'm looking forward to the merge.

[1]
https://github.com/networkupstools/nut/blob/master/drivers/nutdrv_qx.h#L97-L101
[2]
https://github.com/nickma82/nut/commit/ce330a3873fd0a9243a61258db243a6eee=327e65

Hi,
it has been a while now since the last changes to the Voltronic Sunny
code, because it's working just fine.

I saw that there are a lot of changes in the master tree and my question
is what I should do with the Voltronic Sunny part? Does it still make
sense to try to make a pull-request out of it?

Best, Nick