Improve/refactor shared Acurite TXR decoder

[rct]

The acurite_txr_decode function that handles Atlas, 5n1, and 3n1 weather stations, TXR temperature/humidity, 6045 lightning, 515 refrigerator/freezer and1190/1192 water sensors, and 899 rain gauge has been refactored. Changes:

• Refactor 5n1, 3n1, 515, 899 decoders into separate decoding function
  • The shared callback function should be easier to understand and add new devices
• Fix: valid messages with checksum == 0 were being rejected
  • This was due to a bad check for an extra byte being added by demodulators
  • Test case -y 3863187dbbc6f3d87bff
• Fix: Invalid short messages could be accepted resulting in bad data if last byte of message resulted in a valid checksum
  • Test case 9 byte Atlas message that should be rejected -y 'f8 63 99 fc 7b af 3f 5f'
  • See also PR Reject short messages for Acurite-Rain899 #2105 #2116
• Improve rejection of invalid messages
  • Use message type, rather than message length as the primary identifier
  • Validate minimum message length by specific message type
  • Accept messages with one or more extra bytes (demodulator can add extra bits)
    • but only use correct number of bytes for the specific message type.
  • Add parity check to MIC for all devices except the 3n1 weather station
    • Parity was previously only being checked correctly for the tower and leak detector
    • (The 3n1's g001 sample in rtl_433_tests has odd parity on the 2nd to last byte of both message copies. It seems unlikely both repeats would have the same problem. However the g002 sample does use even parity on all bytes.
    • Note: the samples for the 899 range gauge do not have enough data to verify parity is used on the whole message. Need testing from someone with this device.
  • Reject messages if certain fields such as temperature, humidity, wind direction, and other fields have invalid values (such as humidity > 100). This helps when the MIC passes, but there is invalid data in the message.
    • Previously some sanity checks for humidity and temperature were added to a few devices like the tower sensor. Most devices now have humidity validated at a minimum.

These tests should work with both the old code and the new code:

-y '{64}3c003a7df6e8a37a {80}3c009a7df6e8a3d8782c {64}7800397dfca3bb8e {80}7800997dfca3bbd87840 {64}f400b87dfa47d747 {80}f400187dfa47d7d878f9 {64}3D6C87FF5A3AAC75ff {64}2D6C8EFF747290A2 {56}1D9C7E78177E49ff {56}1D9CBEE8177EF9 f4880ffffffffa88/ 20c79fd47209fad5/ 20c79fd4728878d2/ f4880ffffffffa88/ f4880ffffffffa88ff/ {50}3f85b7ee53c0c0/ {50}39083674a998ff'

[zuckschwerdt]

Looks really good! Thanks!

[rct]

Thanks. I'm not sure how to make the symbolizer happy or how to figure out what it is complaining about. I changed the /** style comments to /*. That didn't seem to help.

[zuckschwerdt]

Doc-comments attach to the next function definition. A block at the top with some helpers between then needs to specify the function, e.g. /** @fn int acurite_txr_callback(r_device *decoder, bitbuffer_t *bitbuffer)

[zuckschwerdt]

The process is this: for an r_device follow the callback to get the docs to attach and scan the function to find the models this produces. If this is not immediate one must use a @sa (see also) indirection. Most TPMS have this scheme, I guess it applies to this refactor too.

[rct]

Ok, I'll try to make those doc comments fit. Is there a way to see what function an error like 36 Error: models not found (acurite.c) is in reference to?

[rct]

Added @sa and@fn, still seeing some symbolizer errors.

I'm not sure why it is flagging an issue finding acurite_1190_decode

43 Error: broken link for @sa (acurite_1190_decode)
44 Error: models not found (acurite.c)
45 Error: but @sa (acurite_1190_decode)

[zuckschwerdt]

I'll have a look. The messages need to be better anyway if possible.
In the meantime: to test things you can just run local with ./tests/symbolizer.py >/dev/null

[zuckschwerdt]

Ah, in the doc comment meant for acurite_1190_decode() has a @fn and thus does not attach to acurite_1190_decode(). You only want @fn to attach a doc comment to a function which it is not directly preceeding (which likely should not be done anyway, i.e. a doc comment should be right before the function -- but the main doc is usually a header… )

[zuckschwerdt]

In other words: Have /** ... */ without @fn right before a function it applies to. If a doc comment block (the header block) is not directly followed by the function it applies to use @fn (which is the case for @fn int acurite_txr_callback(r_device *decoder, bitbuffer_t *bitbuffer)).
Don't use multiple doc comment blocks for one function.

[rct]

Ok thanks for the tip about running ./tests/symbolizer.py locally.

I'm tripping over a few things.

So if I understand, I need @sa to link acurite_txr_callback doc block to the functions with the models it generates for the doc processes. However @sa function() gives me a broken link error. I'm following the example in m_bus.c.

/**
Process messages for Acurite weather stations, tower and related sensors
@sa acurite_1190_decode()
@sa acurite_515_decode()
@sa acurite_6045_decode()
@sa acurite_899_decode()
@sa acurite_3n1_decode()
@sa acurite_5n1_decode()
@sa acurite_atlas_decode()
@sa acurite_tower_decode()

generates

::error file=acurite_txr::broken link for @sa (acurite_tower_decode)
::error file=acurite_txr::models not found (acurite.c)
::error file=acurite_txr::but @sa (acurite_tower_decode)

[rct]

EDITED/UPDATED

Don't use multiple doc comment blocks for one function.

I think I've eliminated those, please let me know if not.

There seem to be certain doc blocks that cause the no func error like the acurite_515_decode and acurite_tower_decode, but I'm not sure what it is tripping over. I can work around that by turning them back to regular comments. acurite_atlas_decode has a doc block /** but doesn't seem to cause a no func.

But I still can't get past, ::error file=acurite_txr::models not found (acurite.c). BTW Why is it file=acurite_txr?

I think you've indicated that one or more models should be attached to the callback acurite_txr_callback() which doesn't have a model in it's function definition.

I think you've indicated @sa acurite_xxx_decode() should be used to attach a model to that function, but that doesn't seem to work.

Based on what you wrote, I don't think I should need any @fn acurite_txr_callback(... because the doc blocks all directly precede the functions they are documenting.

Thank you for your help.

[zuckschwerdt]

One more quirk: we only consider function signatures of (r_device *decoder, bitbuffer_t *bitbuffer) to be a decoder that might produce models. That the reason for ::error file=acurite_txr::broken link for @sa (acurite_tower_decode)

[rct]

Ah thanks - glad to know it wasn't something I was doing wrong.

Is there a workaround I should be doing to make the tests pass?

[zuckschwerdt]

The quick fix would be to use e.g.

static int acurite_tower_decode(r_device* decoder, bitbuffer_t *bitbuffer, uint8_t* bb) {
    (void)bitbuffer;

Then each device decoder can be regcognized, has the proper doc-comment and is sa-linked, which should be enough.

[rct]

@deviantintegral - If you get a chance, can you test this PR against your 899 rain gauge?

Two things to look at: The test to reject short messages from #2116 is much more general and I've added parity checking for the 899 (actually all devices that share this decoder except the 3n1). From the samples you have provided in merbanan/rtl_433_tests#428 I couldn't verify that parity checking was used on the data bytes. However I could see that parity checking was used on the message type byte.

[rct]

One more quirk: we only consider function signatures of (r_device *decoder, bitbuffer_t *bitbuffer)

[...]

The quick fix would be to use e.g.

static int acurite_tower_decode(r_device* decoder, bitbuffer_t *bitbuffer, uint8_t* bb) {
    (void)bitbuffer;

Then each device decoder can be regcognized, has the proper doc-comment and is sa-linked, which should be enough.

So does it only match the first two arguments or does it need an exact match for three?

I converted one function (899) to the signature you suggested and @sa function() linked it. but I'm still getting ::error file=acurite_txr::models not found (acurite.c)

[zuckschwerdt]

Rebase to get the fixed symbolizer messages, then patch like this and it should work (but also fix calls and add (void)bitbuffer; to silence warnings):

diff --git a/src/devices/acurite.c b/src/devices/acurite.c
index 2de4710b..08c8f297 100644
--- a/src/devices/acurite.c
+++ b/src/devices/acurite.c
@@ -193,7 +193,7 @@ static int acurite_rain_896_decode(r_device *decoder, bitbuffer_t *bitbuffer)
     return 1;
 }
 
-/*
+/**
 Acurite 609 Temperature and Humidity Sensor.
 
 5 byte messages:
@@ -271,7 +271,7 @@ static int acurite_th_decode(r_device *decoder, bitbuffer_t *bitbuffer)
     return result;
 }
 
-/*
+/**
 Acurite 06045m Lightning Sensor decoding.
 
 Specs:
@@ -480,11 +480,11 @@ static int acurite_6045_decode(r_device *decoder, bitbuffer_t *bitbuffer, unsign
 }
 
 
-/*
+/**
 Acurite 899 Rain Gauge decoder
 
 */
-static int acurite_899_decode(r_device* decoder, uint8_t* bb)
+static int acurite_899_decode(r_device *decoder, bitbuffer_t *bitbuffer, uint8_t* bb)
 {
     // MIC (checkum, parity) validated in calling function
 
@@ -530,11 +530,11 @@ static int acurite_899_decode(r_device* decoder, uint8_t* bb)
 
 }
 
-/*
+/**
 Acurite 3n1 Weather Station decoder
 
 */
-static int acurite_3n1_decode(r_device* decoder, uint8_t* bb)
+static int acurite_3n1_decode(r_device *decoder, bitbuffer_t *bitbuffer, uint8_t* bb)
 {
     // MIC (checkum, parity) validated in calling function
 
@@ -611,11 +611,11 @@ static int acurite_3n1_decode(r_device* decoder, uint8_t* bb)
 }
 
 
-/*
+/**
 Acurite 5n1 Weather Station decoder
 
 */
-static int acurite_5n1_decode(r_device* decoder, uint8_t* bb)
+static int acurite_5n1_decode(r_device *decoder, bitbuffer_t *bitbuffer, uint8_t* bb)
 {
     // MIC (checkum, parity) validated in calling function
 
@@ -959,7 +959,7 @@ static int acurite_atlas_decode(r_device *decoder, bitbuffer_t *bitbuffer, unsig
     return 1; // one valid message decoded
 }
 
-/*
+/**
 Acurite 592TXR Temperature Humidity sensor decoder
 
 Message Type 0x04, 7 bytes
@@ -987,7 +987,7 @@ Notes:
   - @todo - check if high 3 bits ever used for anything else
 
 */
-static int acurite_tower_decode(r_device* decoder, uint8_t* bb)
+static int acurite_tower_decode(r_device *decoder, bitbuffer_t *bitbuffer, uint8_t* bb)
 {
     // MIC (checkum, parity) validated in calling function
 
@@ -1045,7 +1045,7 @@ static int acurite_tower_decode(r_device* decoder, uint8_t* bb)
     return 1;
 }
 
-/*
+/**
 Acurite 1190/1192 leak detector
 
 Note: it seems like Acurite has deleted this product and
@@ -1053,7 +1053,7 @@ related information from their website so specs, manual, etc.
 aren't easy to find
 
 */
-static int acurite_1190_decode(r_device* decoder, uint8_t* bb)
+static int acurite_1190_decode(r_device *decoder, bitbuffer_t *bitbuffer, uint8_t* bb)
 {
     // Channel is the first two bits of the 0th byte
     // but only 3 of the 4 possible values are valid
@@ -1101,7 +1101,7 @@ CCII IIII | IIII IIII | pBMM MMMM | bTTT TTTT | bTTT TTTT | KKKK KKKK
 - p: Parity bit
 
 */
-static int acurite_515_decode(r_device* decoder, uint8_t* bb)
+static int acurite_515_decode(r_device *decoder, bitbuffer_t *bitbuffer, uint8_t* bb)
 {
     // length, MIC (checkum, parity) validated in calling function
 
@@ -1172,7 +1172,7 @@ static int acurite_515_decode(r_device* decoder, uint8_t* bb)
     return 1;
 }
 
-/*
+/**
 Check Acurite TXR message integrity (length, checksum, parity)
 
 Need to pass in expected length - correct number of bytes for
@@ -1232,16 +1232,16 @@ static int acurite_txr_check(r_device *decoder, uint8_t const bb[], unsigned bro
 }
 
 
-/*
+/**
 Process messages for Acurite weather stations, tower and related sensors
- sa acurite_1190_decode()
- sa acurite_515_decode()
- sa acurite_6045_decode()
- sa acurite_899_decode()
- sa acurite_3n1_decode()
- sa acurite_5n1_decode()
- sa acurite_atlas_decode()
- sa acurite_tower_decode()
+@sa acurite_1190_decode()
+@sa acurite_515_decode()
+@sa acurite_6045_decode()
+@sa acurite_899_decode()
+@sa acurite_3n1_decode()
+@sa acurite_5n1_decode()
+@sa acurite_atlas_decode()
+@sa acurite_tower_decode()
 
 This callback is used for devices that use a very similar message format:
 
@@ -1489,7 +1489,7 @@ static int acurite_txr_callback(r_device *decoder, bitbuffer_t *bitbuffer)
         return error_ret;
 }
 
-/*
+/**
 Acurite 00986 Refrigerator / Freezer Thermometer.
 
 Includes two sensors and a display, labeled 1 and 2,

[zuckschwerdt]

Note, since it's easy to miss: you must attach the asterisk to the variable name, not the type.

[zuckschwerdt]

For reference:

• Every decode function needs a signature starting with exactly (r_device *decoder, bitbuffer_t *bitbuffer
• Every decode function needs a doc comment, either directly preceeding or linked with @fn
• Every decode function needs
  • either a plain "model" output line (no macros)
  • or a @sa link to another decode function

Reason: we want to automatically generate documentation for each r_device that explains the decoder and lists which "model"s are produced.

[rct]

@zuckschwerdt - thanks for your help. All passing now.

I'm not sure if it is intentional or not, but now symbolizer seems to be exiting non-zero on warnings. So in order to get the test to pass I had to add doc blocks for the other decoders I didn't touch in order to get symbolizer to exit 0 (so the test would pass).

Note, since it's easy to miss: you must attach the asterisk to the variable name, not the type.

Yes, this was an important tip. I was scratching my head over this.

[deviantintegral]

I built this branch locally and it's showing my rain meter correctly. I still need to rebuild it in the home assistant addon so it can run for a few days and have a chance to pick up any short frames, but can hopefully do that this weekend.

[rct]

@deviantintegral - Thanks -- two ways to verify that parity checking isn't blocking any 899 messages:

• you can check whether the 899 is generating parity on the data bytes by capturing samples or running verbose (-R 40:v) and noticing if the MSB on the 4th/5th bytes is ever set. The code is only using the lower 7 bits, of both bytes, so that's a good sign.
• During normal operation just verify that successive tips of the rain gauge are received correctly. A difference of just 1 should be enough to change the bit parity. So if it only incremented by 1 between each message, you should still be able to see each increment, rather than only seeing the even ones or the odd ones. Hope that makes sense.

[zuckschwerdt]

Did the test and verification work out? Is this good for merge?

[rct]

@zuckschwerdt - I tested with my live devices (Atlas, Towers, 6045, 515) and samples for the 899, 1190, and 5n1 provided by @deviantintegral and @DAVe3283

Based on the samples from @DAVe3283 in #1953 (comment) I get the same decodes with the existing code and the new code, though I get one more successful decode of one of the towers.

I was hoping to get confirmation from @deviantintegral or someone else with the 899 that the 899 does in fact use parity checking on the data bytes.

I've been running the updated code for a number of weeks. The parity checking and sanity checking have eliminated a bunch of bad data.

[deviantintegral]

I’ve been running this for a week or two now with no issues. There’s been no incorrect readings or other issues. I’ve been unable to test manually tipping the bucket, but it did rain and appear to report correctly.