pgularski / pysm

Versatile and flexible Python State Machine library
http://pysm.readthedocs.io/
MIT License
73 stars 11 forks source link

Pushing items into an empty stack on micropyhton esp32 #6

Closed RaphaelMaschinsen closed 5 years ago

RaphaelMaschinsen commented 5 years ago

Hi,

thanks alot for the effort you put into this great library. It's really awesome you made it available for micropython!

I think there might be a small problem with the way items are added to an empty stack on micropython (esp32): If i try to make the rpn_calculator.py example work on my esp32 i run into the following problem:

>>> calc.caluculate(' 167 3 2 2 * * * 1 - =')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 38, in caluculate
  File "<stdin>", line 35, in parse
  File "/lib/pysm/pysm.py", line 673, in dispatch
  File "<stdin>", line 44, in start_building_number
  File "/lib/pysm/pysm.py", line 329, in push
  File "/lib/pysm/pysm.py", line 55, in append
TypeError: unsupported types for __gt__: 'NoneType', 'int' 

The problem is that: self.sm.stack.push(int(digit)) in rpn_calculator.py tries to push into an empty stack, initialized like this:

    def __init__(self, maxlen=None):
        self.deque = deque(maxlen=maxlen)

maxlen ends up as NoneType and therefore can't be compared with > in the append() function in the class deque_maxlen:

        def append(self, item):
            if self.maxlen > 0 and len(self.q) >= self.maxlen:
                self.q.popleft()
            self.q.append(item)

And in the rpn_calculator.py file there is a typo i think it should be calculate() and not caluculate()

RaphaelMaschinsen commented 5 years ago

As a quick fix i added this try ... except ... in pysm.py:

    class deque_maxlen(object):
        def __init__(self, iterable=None, maxlen=0):
            # pylint: disable=no-member
            self.q = deque_module.deque(iterable)
            self.maxlen =  maxlen

        def pop(self):
            return self.q.pop()

        def append(self, item):
            try:
                if self.maxlen > 0 and len(self.q) >= self.maxlen:
                    self.q.popleft()
            except:
                pass
pgularski commented 5 years ago

Hi Raphael,

Good finding! Thanks!

The quick fix you're suggesting is silencing the exception but it doesn't fix the problem with pysm.Stack instances that do not specify explicitly the maxlen property. Ie. stack = pysm.Stack() is flawed. The stack basically doesn't work and you're not able to push anything to it.

The Python documentation states that If maxlen is not specified or is None, deques may grow to an arbitrary length.

Therefore, the fix should read something like this:

    class deque_maxlen(object):
        def __init__(self, iterable=None, maxlen=0):
            # pylint: disable=no-member
            if maxlen in [None, 0]:
                maxlen = float('Inf')
            self.q = deque_module.deque(iterable)
            self.maxlen = maxlen

So for a quick fix you can try the above.

In fact I've created a branch with a fix for this bug here with some unit tests but I haven't had a chance to test it on a ESP32 yet.

pgularski commented 5 years ago

With the latest change on branch Issue-6-Pushing-items-into-an-empty-stack-on-micropyhton-esp32 (see the diff) I got the _testrpn.py working with MicroPython v1.10-230-ge0c6dfe90 on 2019-03-22; linux version. Still didn't test it with ESP32 though. And the code needs some polishing. But you get the idea. P.

RaphaelMaschinsen commented 5 years ago

Thanks a lot for the updates. I just tested _testrpn.py with the updated pysm.py on the esp32 hardware and everything works without a problem now. Also I just wanted to say thanks again for the great project!

pgularski commented 5 years ago

Raphael,

Thanks again for raising this issue and submitting a proposed solution.

I fixed the bug and tested it against the following Micropython versions:

I published a new release - 0.3.8-alpha so you can easily use upip to install the fixed version.

P.