ir_tx deadlock with RP2.

[marued]

When calling NEC.transmit(...) method too fast, it will cause a deadlock on a RP2 platform (Pi Pico W). To solve the issue I added this check (I would of done a pull request but I don't have permission to do so.):

In ir_tx/__init__.py, I added the busy check.

    def trigger(self):  # Used by NEC to initiate a repeat frame
        if ESP32:
            self._rmt.write_pulses(tuple(self._mva[0 : self.aptr]))
        elif RP2:
            if not self._rmt.busy(): # If called while busy, it causes dead-lock
                self.append(STOP)
                self._rmt.send(self._arr)
        else:
            self.append(STOP)
            self.aptr = 0  # Reset pointer
            self._cb(self._tim)  # Initiate physical transmission.

[marued]

Could probably do the same on the ESP32 platform.

[peterhinch]

You have raised a good point but I'll need to think about the best approach. The Pyboard too could have this problem: in all three platforms transmit starts a background process so there is potential for failure. I guess in most cases transmission is initiated by a pushbutton which would explain why this has never arisen before.

My first thought is to have a .busy() method accessible to the user; underlying implementation would differ for each platform. The user could then check before issuing .transmit(). If .transmit() were called when .busy() it would pause:

    def transmit(self, addr, data, toggle=0, validate=False):  # NEC: toggle is unused
        while self.busy():
            pass
        # rest of code

Any thoughts on this approach?

Incidentally anyone can raise a PR on any repo. Create a fork of the repo, do your mods on that fork (normally on a branch), then you can raise a PR.

In cases like this one I think an issue is best as ESP32/STM32 are undecided.

[marued]

I'm not a big fan of the active waiting. The user looses control and they don't have the option to cancel or keep doing other stuff. I prefer giving the user the option to verify if it's busy themselves, but I understand it's a bit more error prone. You could also expose a new parameter with a default value that would give two options: wait or pass if the transmitter is busy.

What do you think about exposing the function for .busy() with what's achievable today and throw an exception for platforms that don't support it yet?

As for the PR, I'm still a beginner when it comes to open source! I've always assumed you had to do it withing the same repo. Thanks for the tip.

[marued]

I went ahead and created a pull request. Feel free to close if it's too early for any decision. I got carried away. I'm open to feedback.

[peterhinch]

I'm still a beginner when it comes to open source!

Part of the deal with a PR is that you test it and update the docs - there is at least one bug in your code...

What do you think about exposing the function for .busy() with what's achievable today and throw an exception for platforms that don't support it yet?

I'm not entirely sure what you mean here. My proposal was that .busy() would be exposed and documented. It can be supported for all three platforms (RP2, ESP32 and STM32).

I have pushed my proposed solution as a branch tx_deadlock for you to view. The only changes are to __init__.py. The fix also includes STM32. It is not yet tested, but by all means give it a try.

I agree with your doubts about methods that block. With this solution the user can avoid blocking by testing .busy() prior to calling .transmit(). The blocking wait is a safety mechanism to prevent a crash.

[marued]

Part of the deal with a PR is that you test it and update the docs - there is at least one bug in your code...

You are right. Sorry about that. I just wanted to show what I had in mind.

It can be supported for all three platforms (RP2, ESP32 and STM32)

I must of misunderstood what you meant.

I agree with your doubts about methods that block. With this solution the user can avoid blocking by testing .busy() prior to calling .transmit(). The blocking wait is a safety mechanism to prevent a crash.

I understand now. You would do both: expose .busy() and add the wait. That works!

[peterhinch]

I have now pushed an update to master. This has been tested on all three platforms and the transmitter doc is updated. (The version you have has a bug in the ESP32 .busy code).

Thank you for pointing out this rather nasty bug.