Skip to content

adafruit_bus_device-in-core crashes with non-busio objects #3856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jepler opened this issue Dec 21, 2020 · 11 comments
Closed

adafruit_bus_device-in-core crashes with non-busio objects #3856

jepler opened this issue Dec 21, 2020 · 11 comments
Assignees

Comments

@jepler
Copy link

jepler commented Dec 21, 2020

The out-of-core adafruit_bus_device can handle any kind of bus-like object (for instance, bitbangio.I2C and adafruit_bitbangio.I2C). The new in-core adafruit_bus_device can only handle busio.I2C, and crashes hard on other object types.

We could consider temporarily disabling the built-in module until this problem can be resolved.

@tannewt
Copy link
Member

tannewt commented Dec 22, 2020

@gamblor21 We should check the type of the incoming object and raise an error if it won't work. That will prevent the crash.

In the longer term and under a compile flag, we can have the C code either recognize bitbangio or use the Python mechanics for the call.

@gamblor21 gamblor21 self-assigned this Dec 22, 2020
@gamblor21
Copy link
Member

I think I know how to do that (or where to look to figure it out). I'll try to understand how the other bus-like objects work but if I catch someone on discord one day probably could be explained quickly if time works for that.

@tannewt
Copy link
Member

tannewt commented Dec 22, 2020

I think I know how to do that (or where to look to figure it out). I'll try to understand how the other bus-like objects work but if I catch someone on discord one day probably could be explained quickly if time works for that.

Feel free to mention me when you have time. We can also coordinate a time too.

@jepler
Copy link
Author

jepler commented Dec 22, 2020

To reject things with the wrong type, you can simply check that the type is the desired one, similar to this code in canio:

        mp_obj_type_t *type = mp_obj_get_type(match_objects[i]);
        if (type != &canio_match_type) {
            mp_raise_TypeError_varg(translate("expected '%q' but got '%q'"), MP_QSTR_Match, type->name);
        }

To make it work with busio and bitbangio (core implementation) you can just check the object's type and make a decision, similar to this code in displayio:

        } else if (bus_type == &displayio_fourwire_type) {
            common_hal_displayio_fourwire_deinit(&displays[i].fourwire_bus);
        } else if (bus_type == &displayio_i2cdisplay_type) {
            common_hal_displayio_i2cdisplay_deinit(&displays[i].i2cdisplay_bus);

To work with the pure python implementation adafruit_bitbangio, you need to arrange to call back into Python using mp_load_method+mp_call_method_n_kw, similar to this example from _pixelbuf:

    pixelbuf_pixelbuf_obj_t* self = native_pixelbuf(self_in);
    mp_obj_t dest[2 + 1];
    mp_load_method(self_in, MP_QSTR__transmit, dest);

    dest[2] = self->transmit_buffer_obj;

    mp_call_method_n_kw(1, 0, dest);

@dhalbert
Copy link
Collaborator

could we do a duck-typing thing where we just look in the type's dictionary for the operations we want?

@gamblor21
Copy link
Member

I understand the 3 methods you suggested @jepler. The first one is probably pretty quick to do, the second I'd have to figure out how to test bitbangio (core). Would the third with pure python fix all the problems? Would that result in a slow down for having this all in the core (the original point?).

I understand in the theory of what you're suggestion @dhalbert but I'm thinking that will take me a bit longer to figure out.

I should be around on discord for the next bit if someone wants to discuss it.

@jepler
Copy link
Author

jepler commented Dec 22, 2020

The third suggestion is how duck typing is implemented in the core (to the best of my knowledge)—in that case, it calls the _transmit method on the self_in object, no matter its type. If the method was not present or the argument requirements weren't met, it would throw an exception.

@gamblor21
Copy link
Member

I was able to play and get duck typing working. But see two possibilities

  1. Just use duck typing for all calls (busio, bitbangio or other).
  2. Use duck typing but for a known type (busio, bitbangio eventually) call the common_hal methods directly.

As I can see it the first option will be a smaller code base, but may be slower with known bus types. The second option will be a larger addition size wise to the core code but may be faster for the known bus types.

@jepler @tannewt any preference as to which I implement?

@jepler
Copy link
Author

jepler commented Jan 2, 2021

My impulse would be to do the simpler thing first, which is doing the duck typing approach ONLY. If it's measurably slower in a reasonable real world situation, then consider adding back the direct call method for busio object types only.

@tannewt
Copy link
Member

tannewt commented Jan 5, 2021

1 works for me too. It'll still be faster than the python impl I expect.

@gamblor21
Copy link
Member

Fixed in #3936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants