[Bug] Calling inherited sensor methods crashes MicroPython

[laurensvalk]

Describe the bug
Classes that inherit a sensor will crash when calling inherited methods.

It's possible we don't have the right flags enabled to support this.

Now that we support multi-file projects, making classes is kind of expected/encouraged, so we should probably re-enable them if we can.

To reproduce

from pybricks.parameters import Port
from pybricks.iodevices import PUPDevice

class NewSensor(PUPDevice):

    def __init__(self, port):

        # Read-only so doesn't crash but gives bad value:
        print(self.info())

        # Crashes:
        self.read(0)

sensor = NewSensor(Port.E)

[dlech]

I don't think there are any config switches for this. I think we just have to call super().__init__() as the first line of the constructor if we want to access the base class inside of the __init__() method.

If we really want to avoid this crash, we could possibly define a __new__ attribute to make mp_obj_instance_make_new() take a different code path. (This seems like an expensive fix in terms of code size to me compared to how often this is likely to happen.)

https://github.com/pybricks/micropython/blob/d21eef4993dac9d62b7cbaa30b9777bb36314484/py/objtype.c#L277

Or maybe we need to add checks that fields are initialized (not zero) before using them?

[laurensvalk]

Yep, planning to look at this soonish, there’s some things I was thinking to rework in this area anyway.

[laurensvalk]

On closer inspection, it isn't due to inheritance per se. It also crashes with simply PUPDevice.read(PUPDevice, 0). Simpler types (StopWatch) also misbehave to a lesser extent.

It's perhaps just because we don't assert the type of self in each of our methods.

We could do that, but we'd need to consider the trade off with build size.

On that note, I've been thinking we could do some fun tricks to reduce the code size of sensor types...

I don't think there are any config switches for this. I think we just have to call super().init() as the first line of the constructor if we want to access the base class inside of the init() method.

They don't have a __init__ method, so overriding __init__ isn't really supported. But as mentioned above, that may not have been the root cause.

We could probably encourage composition over inheritance instead.

[dlech]

We could probably encourage composition over inheritance instead.

👍

[dlech]

This works:

from pybricks.parameters import Port
from pybricks.iodevices import PUPDevice

class NewSensor(PUPDevice):

    def __init__(self, port):
        super().__init__(port)
        print(self.info())
        print(self.read(0))

sensor = NewSensor(Port.C)

[dlech]

On closer inspection, it isn't due to inheritance per se. It also crashes with simply PUPDevice.read(PUPDevice, 0). Simpler types (StopWatch) also misbehave to a lesser extent.

We should probably open a separate issue for this one, but there is MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG to control the behavior of mp_check_self(), so we could add this everywhere and disable it on Move hub to save space.

[laurensvalk]

there is MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG to control the behavior of mp_check_self(),

It costs 156 bytes on Move Hub. Considering that it prevents a crash, that seems like it would be worth it.

[laurensvalk]

I see your WIP commmit now. That's nice for a more descriptive error, but we shouldn't need it to just fix the crash. Just enabling MICROPY_BUILTIN_METHOD_CHECK_SELF_ARG suffices to raise a TypeError.

[dlech]

Considering that it prevents a crash, that seems like it would be worth it.

Except it doesn't fix the crash in the __init__() method in the original comment here.

It only fixes crashing in cases like to list.append(str, 0) and only for MicroPython builtins, not in any Pybricks code. The WIP branch extends it to fix Pybricks code like PUPDevice.read(PUPDevice, 0).

Calling methods in __init__() without calling super().__init__() first still crashes with the WIP branch (which is why I suggested we consider PUPDevice.read(PUPDevice, 0) to be a separate issue).

[laurensvalk]

The WIP branch extends it to fix Pybricks code like PUPDevice.read(PUPDevice, 0).

Just enabling the check self config (without the WIP commit) fixed this for me as well.

[laurensvalk]

Except it doesn't fix the crash in the init() method in the original comment here.

I split it to separate issue.

If we really want to avoid this crash, we could possibly define a new attribute to make mp_obj_instance_make_new() take a different code path. (This seems like an expensive fix in terms of code size to me compared to how often this is likely to happen.)

Agreed that this might be too expensive. Alternatively, it could be handled by the .attr function but that may also be too expensive.

I was hoping we could avoid writing any user code that could crash the hub, but maybe this is just one of those limitations we just have to document.

[laurensvalk]

After a closer look, it does look like this is pretty much the same as #805. Better yet, it can be fixed. It could be done with a variant of the WIP branch (and defining the check self macro correctly, and also doing it manually for non-kwarg funcs), but I'll have a look whether we can do it with a smaller code size impact.

[laurensvalk]

Found it. One line of code :)