Expose the `workarounds` struct as part of public API #133

tanuva · 2023-10-04T11:21:00Z

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- · 2023-10-05T13:26:53Z

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: #93 )

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

compile-time choice
environment variable

r00t- · 2023-10-05T13:27:31Z

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

examples/sml_server.c

r00t- · 2023-10-05T13:37:15Z

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 · 2023-10-06T13:29:48Z

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- · 2023-10-06T16:15:59Z

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 · 2023-10-06T19:11:54Z

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- · 2023-10-07T16:57:26Z

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.

The struct `sml_workarounds` has become a bitmap now, and it is part of the public API. I rephrased the DZG workaround explanation a bit to match the new information: The workaround can't be applied automatically because we need meter model *and* serial number, but only get the latter via SML. The serial numbers that have the fix apparently differ between DZG meter models. This is also why `sml_workarounds` needs to be public: There needs to be a way for the libsml user to configure at least this workaround for the meter at hand. Implementing it as a bitmap is memory-efficient in contrast to passing an additional struct around everywhere. Adding `sml_workarounds` to `sml_buffer` makes it possible to configure workarounds individually for multiple meters that may be handled by the user, each of which will have their own buffer.

This is necessary since we can't decide automatically if the workaround needs to be enabled. (See previous commits for details.)

tanuva · 2023-10-08T20:04:48Z

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.

Make use of libsml's workaround options to apply a workaround for DZG meters. This builds upon the changes from [libsml PR volkszaehler#133](volkszaehler/libsml#133).

r00t- · 2023-11-14T19:37:07Z

sml/include/sml/sml_shared.h

@@ -56,6 +56,43 @@ typedef int64_t i64;
 #define SML_TYPE_NUMBER_32 sizeof(u32)
 #define SML_TYPE_NUMBER_64 sizeof(u64)

+// A bitmap to configure workarounds.
+typedef int16_t sml_workarounds;


would make this uint, so we can safely use all the bits.
i wonder if it should be 32 bits, this library is probably too large for 16 bit machines, otoh. we won't need that many entries anyway...

r00t- · 2023-11-14T20:16:50Z

sml/src/sml_file.c

-sml_file *sml_file_parse(unsigned char *buffer, size_t buffer_len) {
+sml_file *sml_file_parse(unsigned char *buffer, 
+						 size_t buffer_len,
+						 const sml_workarounds workarounds) {


i'm a bit confused that our "constructor" is named _parse, but yes, this is the correct place for that code :)

r00t- · 2023-11-14T20:18:39Z

examples/sml_server.c

 			printf("-h - help\n");
 			printf("-s - process only one OBIS data stream (single)\n");
 			printf("-v - verbose\n");
 			exit(0); // exit here
 			break;
+		case 'd':


in "production" we should probably not have an extra flag for each workaround, but i guess in an example program and while there is only one, this is ok :)

jmberg · 2023-11-15T20:58:15Z

sml/include/sml/sml_shared.h

+/*
+ * Workaround for certain DZG meters that encode the consumption wrongly.
+ * Affected:
+ * - Model TODO with serial numbers starting with 6 (in decimal)


DVS74
https://www.dzg.de/fileadmin/dzg/content/downloads/produkte-zaehler/dvs74/DZG_DVS74-G2_data_sheet.pdf
the other parts of the model I mentioned are just encoding how many things it can measure, afaict

jmberg · 2023-11-15T20:59:41Z

sml/include/sml/sml_shared.h

+ * - Model TODO with serial numbers starting with 6 (in decimal)
+ *
+ * We only get the serial number, not the meter model via SML. Since model
+ * DWSB.2TH (serial starting with 4) does not exhibit this bug, we can't


is that an EWE specific model? I only found this
https://www.ewe-netz.de/-/media/ewe-netz/downloads/zaehleranleitung/dzg_dwsb20_2th_95003041.pdf

then again, there's this
https://www.dzg.de/fileadmin/dzg/content/downloads/produkte-zaehler/dvsb/DZG_DVSB_DWSB_Handbuch_230612_01.pdf and https://www.dzg.de/produkte/moderne-messeinrichtung#dvsb

which documents the whole series - so maybe this is the whole DVSB series?

tanuva · 2024-02-28T11:35:49Z

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

r00t- reviewed Oct 5, 2023

View reviewed changes

examples/sml_server.c Outdated Show resolved Hide resolved

tanuva force-pushed the pr-expose-workarounds branch from 60accef to 90e9b73 Compare October 6, 2023 13:21

tanuva added 2 commits October 8, 2023 21:34

sml_server: Add option -d to enable DZG workaround

e4201d3

This is necessary since we can't decide automatically if the workaround needs to be enabled. (See previous commits for details.)

tanuva force-pushed the pr-expose-workarounds branch from 90e9b73 to e4201d3 Compare October 8, 2023 19:58

tanuva mentioned this pull request Oct 20, 2023

Add ability to configure SML meter workarounds volkszaehler/vzlogger#606

Closed

r00t- reviewed Nov 14, 2023

View reviewed changes

jmberg reviewed Nov 15, 2023

View reviewed changes

r00t- mentioned this pull request Nov 27, 2023

make DZG workaround more specific #136

Merged

tanuva closed this Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose the `workarounds` struct as part of public API #133

Expose the `workarounds` struct as part of public API #133

tanuva commented Oct 4, 2023 •

edited

Loading

r00t- commented Oct 5, 2023

r00t- commented Oct 5, 2023 •

edited

Loading

r00t- commented Oct 5, 2023

tanuva commented Oct 6, 2023

r00t- commented Oct 6, 2023

tanuva commented Oct 6, 2023

r00t- commented Oct 7, 2023

tanuva commented Oct 8, 2023

r00t- Nov 14, 2023

r00t- Nov 14, 2023 •

edited

Loading

r00t- Nov 14, 2023

jmberg Nov 15, 2023

jmberg Nov 15, 2023

tanuva commented Feb 28, 2024

Expose the workarounds struct as part of public API #133

Expose the workarounds struct as part of public API #133

Conversation

tanuva commented Oct 4, 2023 • edited Loading

Open Questions

r00t- commented Oct 5, 2023

r00t- commented Oct 5, 2023 • edited Loading

r00t- commented Oct 5, 2023

tanuva commented Oct 6, 2023

r00t- commented Oct 6, 2023

tanuva commented Oct 6, 2023

r00t- commented Oct 7, 2023

tanuva commented Oct 8, 2023

r00t- Nov 14, 2023

Choose a reason for hiding this comment

r00t- Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

r00t- Nov 14, 2023

Choose a reason for hiding this comment

jmberg Nov 15, 2023

Choose a reason for hiding this comment

jmberg Nov 15, 2023

Choose a reason for hiding this comment

tanuva commented Feb 28, 2024

Expose the `workarounds` struct as part of public API #133

Expose the `workarounds` struct as part of public API #133

tanuva commented Oct 4, 2023 •

edited

Loading

r00t- commented Oct 5, 2023 •

edited

Loading

r00t- Nov 14, 2023 •

edited

Loading