merbanan / rtl_433

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

merge tfa_30_3221 and lacrosse_tx141x: collections of questions and issues #2206

Open knarrff opened 2 years ago

knarrff commented 2 years ago

Those two seem to be the same, down to the case they come in (except the logo/printing).

I do own 5 TFA 30.3221 and they consistently get picked up as LaCrosse-TX141THBv2 but also as TFA-303221. Some observations:

  1. Sometimes apparently the same packet gets picked up by both decoders, at least they show the same time down to second-precision. Two examples of (a) signal(s) captured from the same sensor within the same second, but one identified as LaCrosse-TX141THBv2 and the other as TFA-303221 are [1] and [2]. They only show differences in the last bit (as shown on triq), but that bit is not really part of the message (being the "41th bit", but the content is only 40 bits wide). lacrosse_tx141x has 'priority' set to 10, while tfa_30_3221 does not have it set at all. After double-checking not only using rtl433, but gqrx: the sensors actually do always send two "waves" of signals, about 0.25 s apart, every ~60 s.
  2. For some senders, I get pings from both protocols, while:
  3. For other senders (in other locations, identical model), I get lacrosse_tx141x still picking up the signal, while tfa_30_3221 shows up less often. In the most extreme case, the sensor furthest away is not only never picked up by tfa_30_3221, but is also the sensor that is most often picked up by lacrosse_tx141x (out of the 5 sensors).
  4. The number of rows is always correctly identified as 4.
  5. The number of bits is always identified as 41, although the protocol only contains (the first) 40.
  6. lacrosse_tx141x checks for bitbuffer_find_repeated_row(bitbuffer, 3, 32), which I understand as checking the number of repeats of full rows with at least 32 bits in them. In case it finds at least 3 repeats, it accepts the data and uses the first of the repeats for further decoding.
  7. tfa_30_3221 checks for bitbuffer_find_repeated_row(bitbuffer, 2, 40), which I understand as checking the number of repeats of full rows with at least 40 bits in them. In case it finds at least 2 repeats, it accepts the data and uses the first of the repeats for further decoding.
  8. Both decoders check the checksum.
  9. The timings (short/long/gap/sync/reset) are different. Where lacrosse_tx141x uses 208/417/625/833/1700, tfa_30_3221 uses 235/480/NA/836/850. From the 5 sensors I own, I get the following ranges of readings for short/long/sync: 220-252/464-504/816-860.
  10. lacrosse_tx141x uses channel numbering as in the protocol (0-2), while tfa_30_3221 uses channel numbering as written on the case (1-3) (incrementing the number in the protocol by 1). Due to this, those two are not completely "compatible".

Some thoughts of mine on these past points:

  1. Should the specification of 'priority' on one of the matching protocols not prevent the other from showing up? Also: I never see duplicate messages from rtl433 for the same protocol: is there a mechanism in place that removes duplicates within a short time from each other? If so, this could explain the "duplicate" messages: one for pulse 1 and protocol A and one for pulse 2 and protocol B.
  2. Observations 2 and 3 seem to indicate that tfa_30_3221 is less forgiving concerning noise than lacrosse_tx141x, however observations 6 and 7 seem to indicate the opposite. I have no idea why: more testing would be required (also: should use the same sender, not different ones as now). However: might also not be that important given that they should be merged anyway.
  3. Both protocols call bitbuffer_find_repeated_row(), and that function does compare the whole row (max_bits in the call to bitbuffer_count_repeats() is set to 0). However, the signal is actually always received as rows of 41 bits, where the last bit is 'bogus' and should not be compared. As you can also see in [1] and [2], this is also the 'bit' that is most often seen as different: it is the 'bit' between the end of the last row and the 'sync' part of the next row. Shouldn't both protocols actually call bitbuffer_find_repeated_prefix() instead of bitbuffer_find_repeated_row()? The different lengths of 32 vs 40 should not be an issue in case of this sensor, as the difference are the checksum bits, which is checked separately anyway. More thought would have to go into that change in lacrosse_tx141x due to the different models it supports. If this should be changed, I would be happy to open an extra ticket for that and test.
  4. The timings (see 9. above) would have to be merged as well. Since lacrosse_tx141x seems to support a lot more sensors than tfa_30_3221, I lean towards using those. On the other hand, my observed range of timings for short/long seem to be a lot closer to the ones from tfa_30_3221. Yet on the other hand, the tolerance and also the observations suggest that both sets of values work. I am unsure about the difference in gap/reset though and have too little knowledge of the decoder to judge that impact and would appreciate help there.

Thanks for reading. In order to merge these two (which clearly should be merged: they are the same sensor just with a different label), we need a plan, but for that I nee some input on the thoughts above.

Other tickets that relate to this one are

[1] https://triq.org/pdv/#AAB00B0404034401E400F427148055+AAB0340401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1A18055+AAB00B0403034401E400F427148055+AAB0340401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1A18055+AAB00B0403034401E400F427148055+AAB0340401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1A18055+AAB00B0403034401E400F427148055+AAB0330401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A19355 [2] https://triq.org/pdv/#AAB00B0404034401E400F427148055+AAB0340401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1928055+AAB00B0403034401E400F427148055+AAB0340401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1A18055+AAB00B0403034401E400F427148055+AAB0340401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1928055+AAB00B0403034401E400F427148055+AAB0330401034401E400F4271492A19292A1A1A19292A1A1A1A1A192A19292A1A1A19292A1A1A1929292A19292A1A1929292A1A1A1A355

knarrff commented 2 years ago

Another thought concerning my point 3: wouldn't it be even better to not require repeated rows? As far as I understand, those rows are sent multiple times such that at least one is received intact. There is a checksum on the message, which should take care of noise/collisions with other protocols. Would it not be better if we (only) check somewhat like this:

zuckschwerdt commented 2 years ago

Yes, the "LaCrosse-TX141THBv2" should always be preferred over the "TFA-303221". The "priority" takes care of that. But the mentioned edge cases on non-perfect reception and the different latitude in accepting a signal make this difficult.

The timings for (OOK) PWM are not critical, just a discriminator in the middle between short/long is actually used. The channel numbering was likely a mistake when implementing the "LaCrosse-TX141THBv2", we usually want the first channel to be "1". We can't change that now without breaking things sadly.

The bitbuffer_find_repeated_prefix() is new and perhaps better suited here, but we would need to guess the prefix length first or require a 32 bit prefix, then additionally check for the longer codes later.

Or, as you write, the checksum (CRC/Digest) could be strong enough to forgo the repeated row check, we would need to scan through rows and end at the first good one.

The TX141 / TX141BV3 don't seem to have a checksum though, or at least it's not implemented, that needs to be verified and otherwise would indeed require the repeated row check. Maybe this could be split in 2-3 decoders using "priority" to restore some sanity to the code and allow individual verification schemes.

PR to test some ideas and improve this very welcome.

DigiH commented 2 years ago

As my issue with rtl_433_ESP was linked to above I just want to chip in with my observations with the updated version of rtl_433_ESP, maybe this can be verified and reproduced with rtl_433 by someone.

I used to have deleted the TFA-303221 decoder and ran the previous rtl_433_ESP like that, with it nicely and regularly only ever picking up the LaCrosse-TX141THBv2.

Now with the updated rtl_433_ESP, even after I delete the TFA-303221 decoder (with its .priority = 10), the recognition and decoding of LaCrosse-TX141THBv2 is greatly reduced, reduced to the same extend as if the TFA-303221 decoder was still active:

30 minutes tests - number of recognitions

_rtl_433ESP 0.0.4-Remove-TFA-303221 (pre priority implementation) LaCrosse-TX141THBv2 Fridge - 44 Freezer - 30

_rtl_433ESP 0.1.3-Remove-TFA-303221 LaCrosse-TX141THBv2 Fridge - 24 Freezer - 8

_rtl_433ESP 0.1.3 no decoder removed LaCrosse-TX141THBv2 Fridge - 23 Freezer - 12

TFA-303221 Fridge - 8 Freezer - 16

Did I miss some adjustments for the "priority" when removing TFA-303221 for the new test, or is this an unwanted effect with the priority implementation when removing a decoder, as I would have expected a regular consistent LaCrosse-TX141THBv2 only recognition.

Thanks

zuckschwerdt commented 2 years ago

@DigiH must be something about the fragile tests which are the question here. The last changes (5c17e4e, ade6355) actually made the tests more forgiving though.

The priority is strictly a "if some other decoder already decoded, then skip this", i.e. lower priority (bigger number) means later fallback.

DigiH commented 2 years ago

Thanks @zuckschwerdt, that is how I understood it for the priority. What still surprises me though, is that when removing the TFA-303221 completely, as in the second test, the reduced results very much match the reduced results when the TFA-303221 decoder is present.

Looking to me as if without the TFA-303221 decoder the priority somehow does still not recognise the LaCrosse-TX141THBv2 all the time, if and when it originally would have selected the TFA-303221.

Therefore a branch with TFA-303221 removed does not look like a possible solution for my situation any longer.

Will have to play around and test some more.

Thanks

zuckschwerdt commented 2 years ago

Removing the TFA-303221 will not change anything for the TX141THBv2 decoder. The TFA-303221 (priority 10) is only a fallback and used when the TX141THBv2 (priority 0) can not decode a bitbuffer. It would be the other way around, disabling the TX141THBv2 will invoke the TFA-303221 for all bitbuffers (no higher priority decoder to take bitbuffers away).

knarrff commented 2 years ago

Concerning recognition: I am currently working on something that should increase this for TX141THBv2, I think without increasing the false positive rate. For a first glance, you could take a look at https://github.com/merbanan/rtl_433/commit/081de0ba9af69f198924994deb208376c92b062f. This was just my first test if this idea works at all, so this is far from production-ready, but produces nice results for my limited tests. Apart from more testing, ideally this would be better integrated in the existing code, but this is quite a bit of work do get right due to the different supported models. At the moment, this follows the approach 'change as little as possible in the existing code' (smallest diff), which isn't necessarily producing the nicest code in the end, as you can see.

DigiH commented 2 years ago

I've just tested it the other way round, as suggested above, removing TX141THBv2 and leaving only TFA-303221. This is resulting in a much better recognition, giving me the chance to nicely recognise the broadcast interval of the two thermometers again - one 50 sec, the other and 53 sec. - still with the occasional drop outs every now and then.

My assumption though, that reversing this by giving the previously only remaining TX141THBv2 a priority 10 would also result in the same good recognition didn't work out - must still be some misunderstanding on my side about the priority assignments.

@knarrff I'll give your test above a go to see how it is working in my setup. Thanks.

DigiH commented 2 years ago

Another 30 minutes test, both decoders present, solely with the changes suggested by @knarrff

TX141THBv2 Fridge - 27 Freezer - 9

TFA-303221 Fridge - 3 Freezer - 20

Combined the recognitions of both thermometers are pretty much spot on with their broadcast interval within the 30 minutes.

With the fridge thermometer almost exclusively being recognised by the TX141THBv2 decoder, while the freezer thermometer, possibly due to lower temperature and higher humidity, is mainly recognised as TFA-303221. The fact that the batteries have been in them for one and a half years might also not help - still always "battery_ok":1 though.

As my previous solution with the deletion of one decoder doesn't seem to fully work any longer I'm now thinking I'll leave both decoders in, but rename the TFA-303221 "model" to "LaCrosse-TX141THBv2" as well. While the IDs are identical, the channel differ, which could be accounted for in my backend controller. All a bit fiddly and awkward for a work around :) especially when the main properties, temperature_C and humidity are totally identical from both decoders.

Trying out a bit more here and testing …

knarrff commented 2 years ago

It is not good to have one of those sensors not being recognized by TX141THBv2, even if TFA-303221 is disabled. Could you post example signals for both, possibly the triq-URLs that rtl_433 can spit out when using -A ?

DigiH commented 2 years ago

I hope it was clear that I am using rtl_433_ESP. The debug output options are different with this implementation, and your suggestion above is not possible, but I will hook up a separate ESP32/CC1101 combo tomorrow for further debugging with @NorthernMan54 of rtl_433_ESP.

Once I get my RTL-SDR back I will also test it with rtl_433 directly, as this still seems to be an issue (more or less) on both implementations.

Thanks

DigiH commented 2 years ago

Great news, @NorthernMan54 also look into this from the rtl_433_ESP specific side and found a ESP32 only relevant change which when reverted for my setup remedied the false TFA-303221 recognitions.

That, together with @knarrff's suggested decoder specific changes for better constant recognitions, now gives me a very reliable LaCrosse-TX141THBv2 recognition, to the point that the 50 sec and 53 sec intervals are perfectly recognised and decoded with every single interval.

Very happy bunny here! :)

Thanks for all the support and pointers everybody!!

knarrff commented 2 years ago

One question to the maintainers, but first the backstory: The TFA 30.3221 I have (all five) do send 4 rows per "wave", but in two "waves" with a gap in between of about 15ms and these "couples" about 1 minute apart. This is also one of the reasons I see duplicates here: both waves are recognized by rtl_433 as separate events. Even with better recognition on the side of TX141TutHBv2 I see "duplicates": just not by two different decoders, but now by the same. This is not really the fault of the decoder, since all it does is to correctly recognize all "events" the pwm-decoder sends it, and it looks like events with more than 10 ms of gap in between are treated as separate events. Those 10 ms are probably a good value. However, now my questions:

zuckschwerdt commented 2 years ago

There is a maximum allowed gap to wait for more packets in a transmission to ensure we report events timely (PD_MAX_GAP_RATIO and PD_MAX_GAP_MS). There have been some tries to add a mode to supress duplicates, but in the end we always concluded that rtl_433 should be "low level": report all decoded data seen. Then let consumers use that for redundancy, error checking, signal strengh estimation, and so on. Searching "duplicates" should uncover those older discussions and test code, if you'd like to look into that.

randyoo commented 1 year ago

It is not good to have one of those sensors not being recognized by TX141THBv2, even if TFA-303221 is disabled. Could you post example signals for both, possibly the triq-URLs that rtl_433 can spit out when using -A ?

@knarrff -- I think I have the same issue going on. triq URLs found at the end.

I've got a total of 5 sensors that speak LaCrosse TX141. 1 came with a LaCrosse weather station, 2 pairs came with a fridge/freezer monitor set purchased off Amazon about 18 months apart.

The 2nd pair (newest), has been very finicky about decoding in rtl_433. This pair of sensors, despite appearing identical, must be speaking some slightly different protocol. The receiver/display unit picks them up fine, and they do show up sporadically in rtl_433. I've managed to capture an example of a transmission that, for whatever reason (checksum?), isn't decoded.

bresser_3ch_decode: checksum error
gt_wt_03_decode: Invalid checksum { 5} e7 : 11100
norgo_decode: wrong size of bit per row 1
markisol_decode: bits_per_row[0] = 180
Analyzing pulses...
Total count:  180,  width: 142.75 ms            (35687 S)
Pulse width distribution:
 [ 0] count:   92,  width:  264 us [212;276]    (  66 S)
 [ 1] count:   15,  width:  744 us [740;752]    ( 186 S)
 [ 2] count:   73,  width:  496 us [492;504]    ( 124 S)
Gap width distribution:
 [ 0] count:   16,  width:  716 us [708;728]    ( 179 S)
 [ 1] count:   72,  width:  236 us [228;248]    (  59 S)
 [ 2] count:   90,  width:  464 us [456;528]    ( 116 S)
 [ 3] count:    1,  width:  296 us [296;296]    (  74 S)
Pulse period distribution:
 [ 0] count:  164,  width:  732 us [720;920]    ( 183 S)
 [ 1] count:   15,  width: 1464 us [1452;1472]  ( 366 S)
Pulse timing distribution:
 [ 0] count:  165,  width:  252 us [212;296]    (  63 S)
 [ 1] count:   31,  width:  728 us [708;752]    ( 182 S)
 [ 2] count:  163,  width:  480 us [456;528]    ( 120 S)
 [ 3] count:    1,  width: 10004 us [10004;10004]       (2501 S)
Level estimates [high, low]:  15939,    743
RSSI: -0.1 dB SNR: 13.3 dB Noise: -13.4 dB
Frequency offsets [F1, F2]:     881,      0     (+3.4 kHz, +0.0 kHz)
Guessing modulation: Pulse Width Modulation with sync/delimiter
view at https://triq.org/pdv/#AAB00B040100FC02D801E027148155+AAB00B040300FC02D801E027149155+AAB034040100FC02D801E02714A0A0A08282A0A0A0828282A0828282A08282A082A0A0828282828282A082A082A08282A0A0A0A082829155+AAB00B040300FC02D801E027149155+AAB034040100FC02D801E02714A0A0A08282A0A0A0828282A0828282A08282A082A0A0828282828282A082A082A08282A0A0A0A082A09155+AAB00B040300FC02D801E027149155+AAB034040100FC02D801E02714A0A0A08282A0A0A0828282A0828282A08282A082A0A0828282828282A082A082A08282A0A0A0A082829155+AAB00B040300FC02D801E027149155+AAB033040100FC02D801E02714A0A0A08282A0A0A0828282A0828282A08282A082A0A0828282828282A082A082A08282A0A0A0A0828355
Attempting demodulation... short_width: 264, long_width: 496, reset_limit: 732, sync_width: 744
Use a flex decoder with -X 'n=name,m=OOK_PWM,s=264,l=496,r=732,g=0,t=0,y=744'
pulse_demod_pwm(): Analyzer Device
bitbuffer:: Number of rows: 5
[00] { 1} 80                : 1
[01] {41} 18 ee d3 f5 61 80 : 00011000 11101110 11010011 11110101 01100001 1
[02] {41} 18 ee d3 f5 61 00 : 00011000 11101110 11010011 11110101 01100001 0
[03] {41} 18 ee d3 f5 61 80 : 00011000 11101110 11010011 11110101 01100001 1
[04] {41} 18 ee d3 f5 61 80 : 00011000 11101110 11010011 11110101 01100001 1

As you can see--what I find very strange--by manually feeding test data consisting of the very same data I captured, it seems to be decoded without any issue.

~/rtl_433$ rtl_433 -vvvv -R 73 -y '{41}18eed3f56180 {41}18eed3f56100 {41}18eed3f56180 {41}18eed3f56180'
rtl_433 version 21.12 (2021-12-14) inputs file rtl_tcp RTL-SDR SoapySDR
Use -h for usage help and see https://triq.org/ for documentation.
Registering protocol [73] "LaCrosse TX141-Bv2, TX141TH-Bv2, TX141-Bv3, TX141W, TX145wsdth sensor"
Registered 1 out of 207 device decoding protocols
Verifying test data with device LaCrosse TX141-Bv2, TX141TH-Bv2, TX141-Bv3, TX141W, TX145wsdth sensor.
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
time      : 2022-12-16 21:05:02
model     : LaCrosse-TX141THBv2                    Sensor ID : e7
Channel   : 01           Battery   : 1             Temperature: -20.00 C     Humidity  : 10 %          Test?     : No            Integrity : CRC
pulse_demod_string(): LaCrosse TX141-Bv2, TX141TH-Bv2, TX141-Bv3, TX141W, TX145wsdth sensor
bitbuffer:: Number of rows: 4
[00] {41} e7 11 2c 0a 9e 00 : 11100111 00010001 00101100 00001010 10011110 0
[01] {41} e7 11 2c 0a 9e 80 : 11100111 00010001 00101100 00001010 10011110 1
[02] {41} e7 11 2c 0a 9e 00 : 11100111 00010001 00101100 00001010 10011110 0
[03] {41} e7 11 2c 0a 9e 00 : 11100111 00010001 00101100 00001010 10011110 0

Here is an example of 1 of the 3 sensors that's detected without issue:

RSSI: -0.1 dB SNR: 13.3 dB Noise: -13.4 dB
Frequency offsets [F1, F2]:   -7770,      0     (-29.6 kHz, +0.0 kHz)
Guessing modulation: Pulse Width Modulation with sync/delimiter
view at https://triq.org/pdv/#AAB00B0403034401E400F427148055+AAB0340401034401E400F427149292A19292929292A1A1A192A1A1A192A1A192A192929292A192A1A1929292A1929292A1A192A1A1928055+AAB00B0403034401E400F427148055+AAB0340401034401E400F427149292A19292929292A1A1A192A1A1A192A1A192A192929292A192A1A1929292A1929292A1A192A1A1A18055+AAB00B0403034401E400F427148055+AAB0340401034401E400F427149292A19292929292A1A1A192A1A1A192A1A192A192929292A192A1A1929292A1929292A1A192A1A1928055+AAB00B0403034401E400F427148055+AAB0330401034401E400F427149292A19292929292A1A1A192A1A1A192A1A192A192929292A192A1A1929292A1929292A1A192A1A1A355
Attempting demodulation... short_width: 236, long_width: 480, reset_limit: 856, sync_width: 836
Use a flex decoder with -X 'n=name,m=OOK_PWM,s=236,l=480,r=856,g=0,t=0,y=836'
pulse_demod_pwm(): Analyzer Device
bitbuffer:: Number of rows: 4
[00] {41} 20 ee d0 b1 1b 00 : 00100000 11101110 11010000 10110001 00011011 0
[01] {41} 20 ee d0 b1 1b 80 : 00100000 11101110 11010000 10110001 00011011 1
[02] {41} 20 ee d0 b1 1b 00 : 00100000 11101110 11010000 10110001 00011011 0
[03] {41} 20 ee d0 b1 1b 80 : 00100000 11101110 11010000 10110001 00011011 1

And for comparison, a capture from the other sensor that only works occasionally:

https://triq.org/pdv/#AAB00B0401010002D801D827148155+AAB00B0403010002D801D827149155+AAB0340401010002D801D82714A0A082A08282A082828282828282A082A082A082A082A0A082828282A082A08282A0828282A08282829155+AAB00B0403010002D801D827149155+AAB0340401010002D801D82714A0A082A08282A082828282828282A082A082A082A082A0A082828282A082A08282A0828282A08282829155+AAB00B0403010002D801D827149155+AAB0340401010002D801D82714A0A082A08282A082828282828282A082A082A082A082A0A082828282A082A08282A0828282A08282829155+AAB00B0403010002D801D827149155+AAB0330401010002D801D82714A0A082A08282A082828282828282A082A082A082A082A0A082828282A082A08282A0828282A08282A355

I'd be happy to provide further info or captures if it's helpful!

randyoo commented 1 year ago

Concerning recognition: I am currently working on something that should increase this for TX141THBv2, I think without increasing the false positive rate. For a first glance, you could take a look at 081de0b. This was just my first test if this idea works at all, so this is far from production-ready, but produces nice results for my limited tests.

@knarrff -- Just got around to implementing the code you referenced in that commit, and it seems to be working well for me, too! The two "new" sensors (that were never recognized previously) are now coming through regularly! The other sensors are all being picked up, as well. Danke sehr!

If there's any specific testing I can do to help out, let me know.

randyoo commented 1 year ago

Just to follow up, I've since added a few more sensors, and they all seem to be picked up reliably. I don't know what kind of further testing is needed, but if it's within my capabilities, I'd be happy to lend a hand.

zuckschwerdt commented 1 year ago

@knarrff can you PR that change, it should do for now? (and someday we need to refactor LaCrosse code…)

knarrff commented 1 year ago

The patch as it is right now is not really ready for a PR just yet I think. I wanted to use it to get some discussion going (as I did, thanks) and wanted to also test is myself for some time (which I also now did).

One thing that, besides time, hinders me at preparing PR is that I still see occasional events by tfa_30_3221 even though lacrosse_tx141x is active, has priority and should catch everything now. I have a few ideas why that is and did not find time to look into those.

Also, I do have an idea of how to make the detection even more robust especially when you have more than one sender. The problems here are the times when their submissions partially overlap. The problem here is that those senders all send at roughly 50 s intervals (and not randomly which would be better). Those 50s are just a bit different between senders, so when they drift, the time they need to drift in and then out again of each others "transmission window" can be several hours to a day in some cases. This could go into a separate request in theory, but it is touching the same code that changed here. I would rather change it once than twice.

beauwest commented 1 year ago

I just recently picked up a LaCrosse temperature sensor, and it is occasionally being seen as the TFA sensor. Not a huge deal, but caused me some confusion, and extra sensors getting created automatically in Home Assistant.

gdt commented 1 year ago

Separately from valid cleanup, it is always going to be the case that decoders will occasionally receive an errored packet that was not from an intended device. So really there should be some list of devices to create, with a way for users to basically accept discovered ones. Perhaps automatic based on reliable reception over time. But all of this is a separate issue.

zuckschwerdt commented 1 year ago

@gdt I always wanted that kind of automatic learning utility. Also it should report if a known and reliable sensor suddenly stops. We need to implement that some day.

gdt commented 1 year ago

It seems like there is good stuff in here that we should do something about. But I'm not sure what, and if anything is better in the meantime.

The key question is if there are two kinds of transmitters, or if we have two decoders for the same transmitter. Reading the above, it feels like 90% argument that we have two decoders for the same.

quaqo commented 1 year ago

It seems like there is good stuff in here that we should do something about. But I'm not sure what, and if anything is better in the meantime.

The key question is if there are two kinds of transmitters, or if we have two decoders for the same transmitter. Reading the above, it feels like 90% argument that we have two decoders for the same.

Just my 2 cents/experience, my rtl_433 installation has been running for now almost a year continuously with @knarrff's patch on my Home Assistant-based RPi and pretty much solved my issues. The only "hack/workaround" I added to the patch is that I gave the same model name "TFA-PATCH" in both lacrosse_tx141x.c and tfa_30_3221.c so that any duplicate decoding is read by Home Assistant as the same sensor.

For sure it's a hack, and not a very elegant one, but is good enough for me and as I said I never lost a reading anymore and has been running since October 31st 2022 (patched on the rtl_433 git master at that date).

Hope my experience helps.

gdt commented 5 months ago

Any update from the submitter and commenters? What is the path to closing this issue?

knarrff commented 5 months ago

I also have been running the patch since I made it. I haven't seen issues, but I also did not have the time (or interest - since it works in my case) so far for any improvements. Reading my past self: "The patch as it is right now is not really ready for a PR just yet" I think I trust him (my old me) and would stick to: it would need improvements code-wise before it should go in. At this point it would mean for me getting familiar with the code again. I would do that if there is the chance to resolve some of the open questions of how to proceed and getting the patch in.

gdt commented 5 months ago

Generally when someone is trying to make a PR good enough there are responses and progress. Not sure what you mean by "if there is the chance".

randyoo commented 5 months ago

Just my 2 cents/experience, my rtl_433 installation has been running for now almost a year continuously with @knarrff's patch on my Home Assistant-based RPi and pretty much solved my issues.

For what it's worth, my experience echos that of @quaqo. Grateful for the patch, have been running it without any obvious issue since Christmas Eve 2022.

I'm also not sure what, if anything, is better in the meantime. Might there be some sort of harm to integrate the patch as-is, and refactor later as per @zuckschwerdt at the beginning of last year?

gdt commented 5 months ago

Converting a patch to a draft PR does no harm and is real progress. I'm ok with decoding being a little rough as long as the code is clean and non-scary, but I'm not sure how others see it. Still, a draft PR is way better than a patch in comments in a lengthy issue, and easier to work on and talk about.

knarrff commented 5 months ago

Generally when someone is trying to make a PR good enough there are responses and progress. Not sure what you mean by "if there is the chance".

I am sorry if my reply sounded resentful or similar. It was not meant that way. All I wanted to say is that at least at the moment I do not have that much free time, so I am careful to spend it on projects that have a higher chance of getting somewhere. Receiving good replies from two people within essentially a few hours plus my positive experiences with the project in the past are enough in my books to "fit the bill".

As to good enough: at the time I did not know how to make it good enough, that's why there are so many questions. I must admit that I dropped the ball on this one: typical case of "works for me".

It is also likely that in the end this will result in multiple PRs. There are issues with the specific sensor/sender recognizing signals that might not be 100% clean, but there is also the issue of recognizing signals from different sensors when they are close together (in time) - which is an issue in my case. Part of the 2nd problem can be handled by improvements on the 1st, but only so much. And then yet a 3rd issue is the "repeats" that really only happen because a sensor chooses to send the same data multiple times and with a delay that is too long to let the code treat it as "one". At least in cases where it is known from the protocol that these are just repeats (usually for error-checking or simply to get the signal through), I think we should not report those twice. It unnecessarily exposes part of the underlying protocol of the sender for which rtl_433 is supposed to be the interface layer, know about and deal with.

randyoo commented 5 months ago

I may have spoken too soon earlier, when I said the patch was working "without any obvious issue". I've been collecting the data in Home Assistant, and as I review charts, I've noted there are a number of spikes that occur irregularly (<1x per day, on average). The temperature and humidity reading both differ drastically from previous readings, and subsequent readings are back to baseline.

An example of two spikes: Screenshot 2024-06-09 at 6 03 25 PM

I don't know whether it's the decoding of the transmission, or if the units are just occasionally sending actual garbage data. I have the ability to filter it, but thought it may be worth mentioning. I do have 10 units scattered around the house, so it's fairly likely that some overlapping transmission is sometimes happening.

I'm still willing to help with testing.

gdt commented 5 months ago

Reading back over this I still think we are awaiting one or more PRs. It's fine to submit a PR that fixes something while not fixing everything, as long as the PR doesn't introduce new wrongness.

As for duplicates, doctrine is that if it's sent twice and we decode twice and emit it twice, that is totally fine. There is no need to try to deal with that, and arguably it's wrong to do so. There is a ticket open about a higher-level facility for duplicate suppression, but I don't see that as being in individual drivers, it's nowhere near baked, and an improvement to this driver certainly has no duty to fix that!

knarrff commented 5 months ago

As for the duplicates: the definition of duplicate in the code boils down to, as far as I see, the constants at the top of include/pulse_data.h. To me they seem quite arbitrary. In the end, for the case here this boils down to the definition of packets of shape:

"_A_A___B_B___"

where the two As are 0.25s apart, as well as the two Bs, and A and B are ~50s apart. Also, as the letters indicate, the two As are always the same data, so are the two Bs. Does the gap between two identical letters here constitute a packet gap or are they part of the packet definition, so that a packet is "A_A" or "B_B"? I argue, that this is part of the protocol that trl_433 should abstract away: it is one packet of data that happens to have a very short time of "silence" in the middle that just happens to be longer than PD_MAX_GAP_MS (0.1s).

gdt commented 5 months ago

Your times don't make sense; 50s is forever. In your example, could another transmitter make a transmission during the gap, and might it be decoded? But I don't see changing this as linked to solving the core problem of this issue.

If you want to open an issue or PR about how big the gap should be, you certainly can (but it's going to be controversial :-).

knarrff commented 5 months ago

I think there is a misunderstanding. The "picture" in the previous comment was meant to show two "packets", with 50s in between. The problem is that one packet, e.g. "A_A" also has a gap of about 0.25s and this, at the moment, makes rtl_433 treat this as two separate packets.

As to changing the gap time: I could imagine this to be protocol dependent. Also, the comment in the code seems to suggest that the length was chosen so that we can report the signal in a timely manner, and I agree that this can be important for some applications and will be important when it comes to signals (from different senders) that are very close to each other in time and still should be picked up as separate signals. We could go into details of how this could be implemented, but I think we digress too much from the original topic here although it is related my original "thought 1".

It would help if we could first identify answers for the other issues. This PR was made with the intention of changing as little of the code as possible - so that a review is made easier. It would be better to do a larger decoding overhaul, but for that to happen I would rather have a somewhat defined goal in mind which includes a possible merge between the two if this is possible, desired timings, a specification of what is still considered "valid" and what not. For the moment, I would exclude the question of 0.25s-duplicates from this.

What it would still include, though, is the question of how to report back / find data from different sensors that happen very close in time from each other. "Sensor outages" due to overlap cannot be avoided entirely, but we could try to minimize them somehow. I am not sure how to do that within the current structure.

gdt commented 5 months ago

You say "this PR", but this is #2206 which is an issue not a PR. It would be reasonable for you to extract smaller, contained, separable changes from the discussion and create PRs. Then there will be actual code changes on the table for discussion.

knarrff commented 5 months ago

You are right. What I meant is not a PR: https://github.com/merbanan/rtl_433/commit/081de0ba9af69f198924994deb208376c92b062f

zuckschwerdt commented 5 months ago

250 ms is too much time for rtl_433 to hold back reports for some duplicate checking. Also 250 ms would span multiple I/Q data frames, the demod can't see the transmissions as releated, a sane maximum is below 10 ms. Duplicate checking should be external, a script that knows how to gauge and handle the exact pattern of repeats.