merbanan / rtl_433

Program to decode radio transmissions from devices on the ISM bands (and other frequencies)
GNU General Public License v2.0
5.91k stars 1.3k forks source link

oregon v1 protocol crc is apparently calculated incorrectly (for at least some emitters) #1886

Open c0d3rx opened 2 years ago

c0d3rx commented 2 years ago

Hi, the crc of the oregon v1 packet seems not work well, after observing packet data for a while, I have modified the source code as follow. Now seems to works well

diff --git a/src/devices/oregon_scientific_v1.c b/src/devices/oregon_scientific_v1.c
index c2da764e..93e368c6 100644
--- a/src/devices/oregon_scientific_v1.c
+++ b/src/devices/oregon_scientific_v1.c
@@ -60,8 +60,9 @@ static int oregon_scientific_v1_callback(r_device *decoder, bitbuffer_t *bitbuff
             }
             continue; //  DECODE_FAIL_SANITY
         }
+        if (cs>0x180) cs++;        
+        cs = (cs & 0xFF);

-        cs = (cs & 0xFF) + (cs >> 8);
         checksum = nibble[6] + (nibble[7] << 4);
         /* reject 0x00 checksums to reduce false positives */
         if (!checksum || (checksum != cs))
zuckschwerdt commented 2 years ago

The sum is also calculated strangely. It's split into nibbles, then reassembled to sum:

            nibble[i * 2    ] = byte & 0x0f;
            nibble[i * 2 + 1] = byte >> 4;
            if (i < ((OSV1_BITS / 8) - 1))
                cs += nibble[i * 2] + 16 * nibble[i * 2 + 1];

which is actually

                cs += byte;

it should really be using reflect_bytes() or reflect_nibbles() and then add_bytes().

Now (cs>0x180) cs++; is the same as (cs>0x180) cs += (cs >> 8); and we currently have effectivly (cs>0x100) cs += (cs >> 8);. Not sure if the change is really the right thing? Can you capture a good number of codes and post here?

c0d3rx commented 2 years ago

For me is the right thing, before the code change I was receiving data intermittently, now like a swiss clock. I cannot tell you exactly the sensor model, i am grabbing signal from a far sensor using a yagi antenna. Consider that for this change I have observed raw data for 2 days. Some packets i observed during switch from x180 to > x180

( computed is simply the sum of the first 3 bytes )
8 a 7 5 1 8 0 8  received 80, computed 180  chk ok
....
8 a 7 5 1 8 0 8  received 80, computed 180  chk ok
8 a 7 5 1 8 0 8  received 80, computed 180  chk ok
8 a 8 5 1 8 2 8  received 82, computed 181  chk nok
8 a 8 5 1 8 2 8  received 82, computed 181  chk nok
.....
8 a 8 5 1 8 2 8  received 82, computed 181  chk nok
8 a 9 5 1 8 3 8  received 83, computed 182  chk nok
8 a 9 5 1 8 3 8  received 83, computed 182  chk nok
8 a 9 5 1 8 3 8  received 83, computed 182  chk nok
8 a 9 5 1 8 3 8  received 83, computed 182  chk nok
.....
zuckschwerdt commented 2 years ago

That looks strange. We expect checksum to fold with something like a bitshift, not with a random magic value. I guess >= 0x180 would be somewhat ok, but this is so strange. I need to investigate this more closely, maybe find old issues for this.

gdt commented 8 months ago

It seems this is a valid bug report with a fix. @c0d3rx Can you retest with git master and if appropriate submit a PR?

gdt commented 1 month ago

@c0d3rx I say again: can you retest with git master and if appropriate submit a PR?