Add support for Gridstream RF protocol from Landis & Gyr meters #2616

krvmk · 2023-09-02T05:29:07Z

This PR is to add initial support for the Gridstream RF protocol as discussed in #2531.

merbanan · 2023-09-02T16:30:13Z

Can you add relevant information from here:
https://usermanual.wiki/Landis-Gyr-Technology/IWRS4/html

src/devices/gridstream.c

merbanan · 2023-09-02T17:00:06Z

src/devices/gridstream.c

+        NULL,
+};
+
+r_device const gridstream = {


Duplicate this for the other rates and change the name to match the rate.

Added the other rates, and they appear to work, at least with my saved samples. However, I cannot determine a way to output the bitrate in the data table.

merbanan · 2023-09-02T17:01:42Z

Looks good, make sure to run the maintainer update script also.

src/devices/gridstream.c

gdt · 2023-10-05T23:00:44Z

There are changes to README and man page that seem like they have crept in vs being intended.

gdt · 2023-10-05T23:01:56Z

include/rtl_433_devices.h

@@ -254,7 +254,9 @@
    DECL(fineoffset_ws90) \
    DECL(thermopro_tx2c) \
    DECL(tfa_303151) \
-


Blank line should be left. While it doesn't matter if it's there or not in the grand scheme of things, commits should not make unintended changes.

gdt · 2023-10-05T23:05:12Z

src/devices/gridstream.c

+        {0x142A, "Washington", "Puget Sound Energy"},
+        {0x47F7, "Pennsylvania", "PPL Electric"},
+        {0x22c6, "Long Island, NY", "PSEG Long Island"}};
+


Would be good to have a comment explaining if you have to search, or what the grand plan is. Looks like search, but there is wisdom in somebody's head not in comments right now.

The decoder attempts all of the known CRC init values until it finds one that works. If they all fail, either the data is actually bad, or the CRC init is not known. In the case of the latter, using the reveng program with several fixed length bytestreams with CRC terminated values can output a previously unknown CRC init value, which is how the current values in the struct were derived. This struct would continue to be expanded (through PRs) as new ones are identified.

That's irregular, but makes sense. What I was asking for was for the PR to be amended so that this wisdom would be in the code, so that people reading it, long after the pull request is closed, would see it. (My review is that when we merge, there should be no reason for anyone to look at the PR history.)

gdt · 2023-10-05T23:06:24Z

src/devices/gridstream.c

+            0x7F,
+            0xF8,
+    };
+    uint8_t *b = malloc(bitbuffer->bits_per_row[0] / 8);


Maybe this is normal, but I'm nervous about not storing b's length for bounds checking.

As the maximum size of the gridstream data stream is not known (to my knowledge), and the bitbuffer length can be arbitrarily sized, there was no way to set a maximum size of b without the possibility of overflow issues.

OK, but needs to be explained in comments in the code to be merged.

The last offset you are reading seems b[37]? Then clamp the input length from bitbuffer->bits_per_row[0] - offset - 37 to 38*8 and use a fixed 38 byte buffer. Or any sane upper length really.

The CRC location is based on an offset value that is stored in the datastream and there have been some discovered streams that were much longer than 38 bytes, but are still unknown at this time on how to interpret. Arbitrarily limiting the size would reduce the streams that would captured and/or potentially cause memory exception.

Assume a sane upper bound, say 50(?) bytes, and abort with a warning if a longer message is encountered. That will additionally gain insight into the protocol. Potentially accepting multiple kB of message does not sound proper. If length is a byte then it's bounded anyway ;)

We don't know for certain, but it is believed length is a double byte, as the byte before has always been 0x00. But then again, if it is always 0x00, that is effectively a single byte.

gdt · 2023-10-05T23:07:46Z

src/devices/gridstream.c

+
+                decoder_output_data(decoder, data);
+                break;
+            case 0xD2:


spacing is odd here, and that makes it a tiny bit harder to read.

gdt · 2023-10-05T23:09:42Z

src/devices/gridstream.c

+                free(b);
+                return DECODE_FAIL_MIC;
+            }
+            sprintf(found_crc, "%04x", known_crc_init[crcidx].value);


I get what's happening here, but given that location/provider are not precomputed it made me think there was a bug. I would prefer to see all three handled the same way so that the reader is led to the right conclusion the first time.

found_crc is the only one that needs a transform to string from the struct in order to be displayed properly in the output data. The location/provider is already in a string.

It would still be nice to regularize and/or comment, to make a reader who is competent but not expert quickly come to the right conclusion.

gdt · 2023-10-05T23:13:18Z

src/devices/gridstream.c

+            case 0xD5:
+                sprintf(destaddress_str, "%02x%02x%02x%02x", b[5], b[6], b[7], b[8]);
+                sprintf(srcaddress_str, "%02x%02x%02x%02x", b[9], b[10], b[11], b[12]);
+                if (stream_len == 0x47) {


There are conditional "include this item if this condition is true" constructs. That would perhaps make this easier to understand, if that fits the situation.

How would conditional items be handled with the data output? I've not found any example of conditional constructs with data make.

See fineoffset_ws80.c and look for DATA_COND. There is a way to have what is basically an if statement and if true, the item is added, and if not, it isn't. At least that's my understanding.

gdt · 2023-10-05T23:17:36Z

conf/rtl_433.example.conf

-# protocol 245 # ThermoPro TX-2C Thermometer
+# protocol 245 # ThermoPro TX-2C Thermometer and Humidity sensor
+  protocol 246 # TFA 30.3151 Weather Station
+  protocol 247 # Gridstream decoder


"decoder" has no content . Perhaps

protocol 247 # Gridstream (power meters)

if that's fair.

doctorkb · 2024-02-02T16:09:40Z

I think this might solve my need -- my water company just swapped out the previous meter for a Landis + Gyr GridLinx Interpreter which supposedly connects to the GridStream network.

Is there anything I can do to help get this out of development and into production? It looks like it maybe just failed a single test on code style...

zuckschwerdt · 2024-02-02T16:52:22Z

The CRLF problems need to be fixed, indentation fixed, some comments added, style updated (as per the review) and then it should be in good shape to merge. Maybe we need to manually merge with those fixes added?

krvmk · 2024-02-02T18:25:22Z

The CRLF problems need to be fixed, indentation fixed, some comments added, style updated (as per the review) and then it should be in good shape to merge. Maybe we need to manually merge with those fixes added?

I had made most of the recommendations to a separate branch here, but I have not had time to move forward with finishing it up.
https://github.com/krvmk/rtl_433/blob/merge/src/devices/gridstream.c

krvmk · 2024-02-03T06:03:35Z

Reset and repushed, but the maintainer_update changed a bunch of other stuff in the README. I'm not sure why.

krvmk · 2024-02-07T04:05:27Z

Should be good to go now.

gdt · 2024-06-04T12:32:54Z

What's the status of this? I'm looking at #2531 and it sort of seems like if this were merged we could close the issue and be down two items.

merbanan · 2024-06-04T12:39:47Z

LGTM, we need some samples though and a rebase.

krvmk · 2024-06-05T02:53:00Z

LGTM, we need some samples though and a rebase.

What do you need for samples? There are some in the issue discussion #2531.

zuckschwerdt · 2024-06-05T08:30:10Z

Trouble is that this decoder uses extra headers of stdbool.h, stdlib.h, time.h. The one single bool should be an int.
The usage of strftime() and localtime() is understandable but not what we usually have in decoders.
What is stdlib used for?

gdt · 2024-06-05T10:38:35Z

stdlib.h is described at https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdlib.h.html

It is surprising to me that use of it in rtl_433 is novel. But

./output_mqtt.c:#include <stdlib.h>
./output_influx.c:#include <stdlib.h>
./bitbuffer.c:#include <stdlib.h>
./pulse_data.c:#include <stdlib.h>
./output_file.c:#include <stdlib.h>
./baseband.c:#include <stdlib.h>
./data_tag.c:#include <stdlib.h>
./output_trigger.c:#include <stdlib.h>
./sdr.c:#include <stdlib.h>
./pulse_slicer.c:#include <stdlib.h>
./optparse.c:#include <stdlib.h>
./write_sigrok.c:#include <stdlib.h>
./pulse_detect.c:#include <stdlib.h>
./decoder_util.c:#include <stdlib.h>
./r_util.c:#include <stdlib.h>
./pulse_analyzer.c:#include <stdlib.h>
./rtl_433.c:#include <stdlib.h>
./r_api.c:#include <stdlib.h>
./term_ctl.c:#include <stdlib.h>
./samp_grab.c:#include <stdlib.h>
./mongoose.c:#include <stdlib.h>
./mongoose.c:#include <stdlib.h>
./am_analyze.c:#include <stdlib.h>
./bit_util.c:#include <stdlib.h>
./data.c:#include <stdlib.h>
./confparse.c:#include <stdlib.h>
./getopt/getopt.c:/* Don't include stdlib.h for non-GNU C libraries because some of them
./getopt/getopt.c:# include <stdlib.h>
./getopt/getopt.c:#include <stdlib.h>
./compat_paths.c:#include <stdlib.h>
./output_rtltcp.c:#include <stdlib.h>
./list.c:#include <stdlib.h>
./output_log.c:#include <stdlib.h>
./output_udp.c:#include <stdlib.h>
./pulse_detect_fsk.c:#include <stdlib.h>
./devices/flex.c:#include <stdlib.h>
./devices/blueline.c:#include <stdlib.h>
./fileformat.c:#include <stdlib.h>

zuckschwerdt · 2024-06-05T10:51:46Z

It is surprising to me that use of it in rtl_433 is novel. But

Decoders (src/devices) should not use stdlib because alloc() should not be used. It appears you don't alloc, why do you need stdlib?

gdt · 2024-06-05T12:35:09Z

I guess it's easy enough to remove it and see if there is a warning.

krvmk · 2024-06-05T14:38:41Z

It is surprising to me that use of it in rtl_433 is novel. But

Decoders (src/devices) should not use stdlib because alloc() should not be used. It appears you don't alloc, why do you need stdlib?

Good catch. It was leftover from the initial attempt using a dynamic bitbuffer size.