Fix Security+ 2.0 decoder: cache halves across callbacks

[asjoyner]

The Security+ 2.0 protocol sends two packets (Set 1 and Set 2) separated by ~10ms. The PCM demodulator delivers each packet as a separate callback invocation with one row. The decoder expected both halves to arrive in the same bitbuffer, which never happens because the ~10ms inter-packet gap exceeds the 9ms reset_limit.

An alternative fix would be to increase reset_limit to ~12000us so both halves land in the same bitbuffer. However, this would risk accumulating noise during the gap and could merge unrelated transmissions. Instead, this fix follows the more robust pattern used by the Security+ 1.0 decoder (4657128), which caches the first half in static variables with a timeout.

Add static caching to the v2 decoder:

• When only one half is decoded, cache it with a timestamp
• On the next callback, check if the other half is cached
• Combine both halves if the cache is fresh (<800ms)
• Clear the cache on timeout or successful decode

History of the bug:

The Security+ 2.0 decoder was added in 37cb397 (Aug 2020). The reset_limit was set to 9000us, but the protocol's ~10ms inter-packet gap exceeds this, so the PCM demodulator has always delivered the two halves as separate callback invocations. Additionally, the signal detector's PD_MIN_GAP_MS (10ms) causes the ~10ms gap to split the pulse data into separate packages at the detection stage. The decoder has never been able to combine the two halves from live hardware. Commit 50b613d (Sep 2020) changed the decoder from single-row scanning to multi-row iteration, but this doesn't help since the two halves arrive in separate callbacks (which is always the case with the current code), not separate rows of the same bitbuffer.

The Security+ 1.0 decoder, added later in 4657128 (Feb 2021), correctly handles this by caching the first half in static variables with a timeout, matching the pattern used by other stateful decoders in rtl_433. This fix applies the same caching approach to the v2 decoder.

Tested with live RTL-SDR Blog V4 hardware receiving a Security+ 2.0 garage door opener remote. Before this fix: zero decodes. After: reliable decoding of all button presses.

[merbanan]

Well LGTM. @zuckschwerdt what do you think?

[zuckschwerdt]

I really don't like the idea of time-based actions in decoders. A decoder should not be coupled to passage of wall clock time -- it will be a problem with simulations, tests, api calls, …
And hidden state invalidates viewing a decoder as pure function.

That said, in special cases we can temporarily accept it. Few enough cases that we can rewrite it later.

We should start adding notes to offending decoders to keep track.

Maybe something like

@attention stateful
This decoder has an internal state that may change between invocations and influence the output.

@attention time-based
This decoder depends on wall clock time and exact timing might influence the output.

A second sentence to each might reason and explain why it is not as dangerous as it looks. Somewhat similar to Rust's Safety comments.

[asjoyner]

I like the noble goal to minimize time references and hidden state in the code, but I think its an unavoidable necessity in this case? The Security+ 2.0 protocol has an implicit timing consideration as part of how its defined. If the code ignores that, it will limit how accurately it can match the real signals from a real source. As I mentioned, I did consider just extending reset_limit so the data ends up looking like one bit stream, but that did not seem like the most robust approach. Should I pivot to that? (It's actually way simpler, just a one-line constant change. :) )

I'm happy to add additional tests that help guard against future regressions. I did most of my validation with a .cu8 format capture from real data, but felt it was a bit too large to include in the repo as test data and the local pattern seemed to favor synthetic tests.

If you can suggest a pattern you'd prefer that avoids the timing or hidden state, I'm happy to pivot the implementation. I'll also be glad to update the others while I'm here if we have sufficient test data to be sure I won't break them.

[zuckschwerdt]

Having hidden state breaks e.g. reentrancy i.e. threading. We already use threading for the SDR data and we plan to run multiple demods someday. Some exceptions can be fixed later, but we need to stress that stateless decoders should be the default.

Thanks for adding the notes. I'd didn't even see that you had @warning already.

[merbanan]

I really don't like the idea of time-based actions in decoders. A decoder should not be coupled to passage of wall clock time -- it will be a problem with simulations, tests, api calls, … And hidden state invalidates viewing a decoder as pure function.

That said, in special cases we can temporarily accept it. Few enough cases that we can rewrite it later.

I agree fully.

Maybe supporting a larger reset time but emitting partial bit buffers that can be buffered outside of the decoders. Ie if a smaller bitbuffer matches in a decoder but it needs a companion packet it can return a status that it needs more data. In that case the decoder does not keep any hidden states it just requests more bitbuffer. And at the next decode call it will have enough data to decode.

Ideally the reset limit is then for the complete duration of the 2 signal segments. Some clever logic would be needed for keeping track of time and how to manage a set of several bitbuffers in some decoder cache.

I think this would be the best solution for this problem. Ie move the hidden state from the decoders to the calling context.