Implement nonblocking decoder

[danielballan]

Thanks for maintaining this excellent ecosystem of software and hardware. This is my first attempt to contribute to an Adafruit repository.

The Problem

As described in #32, the non-blocking code path in GenericDecoder is not really non-blocking. Several people have reproduced the issue: if you hold down a button on a remote control to produce a steady stream of pulses, GenericDecoder.read_pulses(..., blocking=False) blocks indefinitely until the pulse stream stops.

There is also a more subtle issue in the design. When the application is juggling multiple tasks, it's possible for more than one message to accumulate in PulseIn before we get around to reading it. But (if I understand correctly) the current GenericDecoder API has no way to return more than one code when it is read.

Background

I am new to IR, but I wrote caproto, a beginner-friendly library that solves similar problems for EPICS, an industrial and scientific control protocol used by US Department of Enengy facilities and others. In this PR, I have tried to apply what I learned from that work about non-blocking message-parsing code, while taking as a light a touch as I can and carefully maintaining backward-compatibility with existing APIs.

Proposed Solution

I cannot see how to fix the GenericDecoder in a backward-compatible way, so I have created a new class NonblockingGenericDecoder. This is a usage example:

pulses = PulseIn(...)
decoder = NonblockingGenericDecoder(pulses)
while True:
    for message in decoder.read():
        if isinstance(message, IRMessage):
            message.code  # TA-DA! Do something with this in your application.
        else:
            # message is either NECRepeatIRMessage or
            # UnparseableIRMessage. You may decide to ignore it, raise
            # an error, or log the issue to a file. If you raise or log,
            # it may be helpful to include message.pulses in the error message.
        # There may be more messages left in the PulseIn, but we can
        # go do other stuff in the appliacation before parsing them if we want
        # before getting the next message.
    # No more messages left for now. We are caught up.
    # Do other stuff in your application and check back later.

Notes on the design:

• decoder.read() is a generator: it returns an iterator that you can loop over to get messages. This allows for the possibility that multiple messages accumulate between reads. When there are no messages, decoder.read() is simply an empty iterator.
• If there are some pulses in PulseIn but they happen to be part-way through a message when we read(), those pulses are stashed internally by NonblockingGenericDecoder and held until read() is called again and the message is completed.
• The PulseIn wouldn't necessary have to be an argument passed to __init__. It could be passed to read(), more akin to how it's done in GenericDecoder. But since we need a class anyway to store the partial message state, it seems convenient to pass PulseIn in the __init__ so we only have to do it once. Also, since the decoder consumes messages from PulseIn, mutating it by popleft()-ing contents off of it, it "owns" it in a sense. Thus, having it belong to NonblockingGenericDecoder feels cleaner to me than passing it in to read(pulses) as a method argument.
• I have refactored bin_data and decode_bits from methods to functions so that they can be shared between GenericDecoder and NonblockingGenericDecoder. I noticed the comment that these are methods because "we may add object level config later". I suggest that config could be passed in as a parameter without making these into methods. They would only need to be methods if they need to modify instance state. I don't think that should come up, so I think it is safe to make them simple functions.
• I have added some new classes namedtuple types (IRMessage and friends) to do the job that GenericDecoder does with exceptions. That is, the try/except control flow is replaced by if isinstance(...) control flow. There are couple reasons for this.
  • When we change from returning a single code to returning an iterable of potentially multiple codes, the exception raising becomes a hassle. When a bad message is raised, we have catch it and then start reading again.
  • Even when parsing succeeds, it's convenient to have this IRMessage object bundling together the code and the pulses in case we want to do something with the pulses. (In caproto, we get a lot of use out of similar objects which contain both human-friendly properties and the raw bytes received.)
  • Exception throwing is not a cheap operation in Python. Since repeat codes and unparseable messages are not especially rare or unexpected in IR, using exceptions to represent them may lead to excess stack-thrashing in the interpreter that puts a burden on the application.

Evidence that it works smoothly

Performing the same test that I performed in #32 with this new API, messages are parsed quickly and the application loop is given frequent opportunity to do other work, as in indicated by the "Heartbeat" messages.

t=0.715 Heartbeat
t=0.742 New Message
Heard 42 Pulses: (2429, 560, 1228, 563, 637, 557, 1229, 567, 1282, 507, 1232, 560, 1228, 581, 620, 558, 634, 559, 1277, 521, 632, 578, 1208, 597, 1236, 528, 678, 517, 1223, 562, 632, 563, 632, 568, 627, 565, 1227, 561, 1233, 559, 1236, 11702)
Decoded: [67, 75, 8]
----------------------------
t=0.789 New Message
Heard 42 Pulses: (2434, 555, 1231, 570, 630, 555, 1231, 567, 1225, 561, 1237, 554, 1232, 564, 667, 529, 684, 508, 1219, 582, 613, 585, 1218, 593, 1220, 536, 629, 565, 1230, 562, 632, 563, 628, 565, 628, 572, 1225, 561, 1225, 567, 1225, 11712)
Decoded: [67, 75, 8]
----------------------------
t=0.816 Heartbeat
t=0.832 New Message
Heard 42 Pulses: (2423, 568, 1224, 565, 629, 571, 1223, 563, 1232, 560, 1227, 574, 1218, 571, 632, 563, 623, 578, 1214, 582, 612, 599, 1195, 605, 1237, 543, 639, 526, 1226, 567, 634, 560, 634, 561, 686, 509, 1237, 553, 1227, 568, 1224, 11715)
Decoded: [67, 75, 8]
----------------------------
t=0.879 New Message
Heard 42 Pulses: (2420, 569, 1281, 506, 687, 507, 1233, 559, 1280, 512, 1228, 570, 1222, 564, 630, 575, 620, 572, 1278, 530, 606, 594, 1212, 549, 1253, 543, 642, 548, 1230, 567, 680, 510, 638, 556, 637, 556, 1284, 510, 1230, 559, 1232, 11706)
Decoded: [67, 75, 8]
----------------------------
t=0.93 New Message
Heard 42 Pulses: (2429, 560, 1235, 558, 634, 560, 1238, 554, 1284, 513, 1227, 561, 1233, 560, 705, 489, 633, 561, 1233, 584, 611, 573, 1223, 557, 1250, 543, 636, 590, 1197, 567, 627, 564, 637, 554, 640, 554, 1233, 563, 1229, 559, 1233, 11706)
Decoded: [67, 75, 8]
----------------------------
t=0.945 Heartbeat
t=0.969 New Message
Heard 42 Pulses: (2429, 561, 1283, 510, 690, 504, 1283, 509, 1230, 573, 1219, 562, 1283, 510, 632, 562, 632, 562, 1283, 510, 632, 562, 1235, 592, 1195, 573, 622, 592, 1199, 602, 610, 548, 666, 526, 655, 549, 1220, 596, 1196, 562, 1233, 11704)
Decoded: [67, 75, 8]
----------------------------
t=1.01 New Message
Heard 41 Pulses: (2428, 562, 1236, 556, 634, 560, 1236, 556, 1283, 515, 1225, 562, 1235, 556, 634, 560, 634, 591, 1206, 595, 652, 502, 1242, 550, 1254, 546, 629, 606, 1237, 507, 1200, 629, 563, 621, 569, 1286, 511, 1223, 564, 1238, 11698)
Failed to decode Both even/odd pulses differ

Open questions

• I am not confident that I understand the purpose of the "pruning" code in GenericDecoder. Is that necessary because GenericDecoder can only return one code per read, and thus has to dump any pulses past the first code? Is it safe to assume that no pruning is needed in this new decoder that can yield multiple codes?
• What should we do with the broken code path read_pulses(..., blocking=False)? Should it raise an error pointing people to GenericNonblockingDecoder? Or issue a warning?

[danielballan]

The CI failures from the linted seem related to existing code, not the changes in this PR. Will close and reopen to see if subsequent changes to the main branch resolve this.

[tannewt]

Sorry for not replying earlier! Please merge in the latest changes from the repo into this branch and then I'll take a look.

[jposada202020]

@danielballan Hello, could you take a look to the following guide in order to the CI to pass the verification?
Instructions are available here: https://adafru.it/check-your-code
Thanks Again

[danielballan]

The linter’s complaints are related to pre-existing code that I hesitate to change too deeply for fear of breaking something unknowingly. Let me see how light a touch I can take....

[danielballan]

Rebased and then updated to satisfy pylint. You'll notice I disable it in a couple lines. I explain why in the comments; I think the alternative could break existing user code.

Needs maintainer approval to run CI. I haven't yet re-tested this with hardware after the rebase and addition of 48ca908. I might get a chance to do so later this weekend.

[danielballan]

Fixed one more complaint from pylint that for whatever reason didn't show up when I ran it locally.

[jposada202020]

@tannewt this will be ready for your review as requested. Thanks

[tannewt]

One super minor thing. Overall it looks really good. Thank you for the changes. It'll be really nice to have a real non-blocking API.

[tannewt]

Please use the pylint name instead of code so it's more readable.

[danielballan]

Thanks for the review. Comment addressed.

[tannewt]

Looks good to me! Thank you!