Expose the `workarounds` struct as part of public API

[tanuva]

This is supposed to become a sensible fix for #132. In that issue, it turned out that the workaround for broken negative values from DZG power meters cannot be applied automatically. For one model, DZG confirmed that the bug was fixed in devices with serial numbers starting with 6 (#106) - of a certain model, apparently. I have a different model, the DZG DWSB20.2TH with a serial number starting with 4. Mine does not exhibit the bug.

Since we only get the serial number and not the meter model via SML there is no way to reliably enable the fix automatically. Therefore I tried to expose the struct holding workaround flags as part of the public libsml API.

This also implies a corresponding PR for vzlogger. I have the code already and could push it anytime, however I may spare myself a rebase or two if we discuss this one first, maybe?

Open Questions

• [ ] I'd like to mention the meter model the workaround is needed for in the explanatory code comment. Which one was that?

[r00t-]

thanks for taking this on! (and taking so much responsibility for other people's/company's bugs!)

it seems a bit scary to need so much code for this, but i guess this is the cleanest way. (there's another MR (of mine) thay would change the ABI, maybe we should do both while we're at it: https://github.com/volkszaehler/libsml/pull/93 )

"easier" (for the codebase, not the user) solutions could be:

• compile-time choice
• environment variable

[r00t-]

don't have time for a detailed code review atm, will try to have a closer look asap.

[r00t-]

Passing workarounds explicitly everywhere isn't beautiful, don't know of a better way in C.

it's the same in any language, you need some data structure(s) to manage state - object oriented programming only makes it more explicit, but the logic is the same.

(almost) every function you added the workarounds parameter to already takes an sml_buffer, which should be the right place to add the flag. (i guess just the naming does not make it very intuitive.) https://github.com/volkszaehler/libsml/blob/master/sml/include/sml/sml_shared.h#L71

[tanuva]

I made the workarounds struct a member of sml_buffer now. I did not make the flag itself a buffer member because that would force us to add a parameter for every flag to sml_file_parse. I think the main decision there is if we expect more workarounds to appear in the future. If we do, passing a struct is imho nicer. If we have faith in meter manufacturers we could also stick to just passing the one flag there, avoiding the need to copy values from the user-supplied sml_workarounds instance to the one that is part of sml_buffer. (I chose to not make that a pointer to avoid the additional memory management...)

[r00t-]

actually it's quite wasteful to use a struct of ints for a set of boolean flags. one could use just an int and assign a meaning to each bit of it. i don't think we'll need more than 31 (or even 15) workarounds.

[tanuva]

Definitely more efficient, yes. I feel like I'm leaving my comfort zone of a desktop developer when it comes to keeping things really small and portable though. :)

Just to sync before I do the rebase dance, I'd set it up like this:

typedef int16_t sml_workarounds;
const int16_t sml_workaround_dzg_negative = 0x01;

// Add it to sml_buffer:
sml_workarounds workarounds;

// Now we can simply
if (buf->workarounds & sml_workaround_dzg_negative) { /* profit */ }

...so that the magic numbers actually have names. While I don't like this (potential) heap of not-really-encapsulated numbers too much I have to admit this is more elegant than copying values into the sml_workarounds struct held by each sml_buffer. Maybe it is the better tradeoff, looking at expecting possibly sometime more workarounds.

[r00t-]

one would normally use constants (#define, in uppercase) to label the bits, no need to waste space on read-only variables :)

i'm sorry that i don't find the time for a more detailed review, it seems a waste of your time to keep rewriting most of the code for such small iterative steps.

[tanuva]

one would normally use constants (#define, in uppercase) to label the bits, no need to waste space on read-only variables :)

Aaah, the C world comes at me in big waves. I'd love to put the workaround options into an enum class for proper encapsulation, but that's not going to work here. Defines seem to be established in libsml, so I shall carry the flame.

i'm sorry that i don't find the time for a more detailed review, it seems a waste of your time to keep rewriting most of the code for such small iterative steps.

I'm used to iterative reviews, no worries. And I do appreciate very much that you are as responsive as you are, things are lingering around for much longer in other places! :) The changeset has become much smaller by now and feels less invasive, so the iteration was definitely worth it.

[tanuva]

This has become obsolete through the more specific workaround merged above.