Check for duplicate pins in rows and columns

[gamblor21]

Closes: #5034

KeyMatrix will check all the pins used for any duplicate against the rows and columns.

[jepler]

Looks like very plausible code. Didn't test.

[dhalbert]

I have looked at the best way to fix this bug, and even started to code it a bit, but only very briefly. It was more complicated than I liked.

It could be done by using a Python set in shared-bindings, adding all the pins to the set, and making sure the set length is correct. Or it might be done in the shared-module implementation, but it's really something that should be done as arg validation in shared-bindings.

Right now the pin-claiming logic checks some actual hardware state, so you can't see if the pin is claimed until you go to allocate it.

All the keypad classes should check for duplicate pins in the pin lists, not just KeyMatrix, so you might want to write a general duplicate object or duplicate pin checker which can be used in other cases as well. But KeyMatrix is special because it has two lists of pins, not just one.

[gamblor21]

I was looking at this more tonight after the discussions on discord. I did implement a argcheck.c methods mp_arg_validate_no_duplicates. This checked one array for any duplicates, returning an error if so.

For test purposes I made a mp_arg_validate_no_duplicates2 which checked two arrays against each other. I think modified KeyMatrix.c to run these checks against rows, columns and then the second function for rows+columns. It works.

A couple of thoughts:

• I haven't checked this to validate the objects are subscriptable (or if it is required) just did this to test
• This seems like the simplest way to due a duplicate item check and reusable in the future if it is required again
• This does not depend on any hardware pin checking/setting so it keeps the validation of arguments in shared-bindings

Let me know the thoughts. I can also try to be there on Monday and discuss this more during in the weeds, have to check if I am free at that time.

Code for mp_arg_validate_no_duplicates:

mp_obj_t mp_arg_validate_no_duplicates(mp_obj_t obj, qstr arg_name) {
    const size_t num_objects = (size_t)MP_OBJ_SMALL_INT_VALUE(mp_obj_len(obj));

    for (size_t object_cnt = 0; object_cnt < num_objects; object_cnt++) {
        mp_obj_t obj1 = mp_obj_subscr(obj, MP_OBJ_NEW_SMALL_INT(object_cnt), MP_OBJ_SENTINEL);

        for (size_t object_cnt_2 = object_cnt+1; object_cnt_2 < num_objects; object_cnt_2++) {
            mp_obj_t obj2 = mp_obj_subscr(obj, MP_OBJ_NEW_SMALL_INT(object_cnt_2), MP_OBJ_SENTINEL);
            if (obj1 == obj2)
                mp_raise_TypeError_varg(translate("%q contains duplicate objects"), arg_name);
        }
    }

    return obj;
}

[dhalbert]

I found a use for non-pin duplicate checking: in usb_hid the Device constructor will be passed a list of int report_ids which must not be duplicates. This is a pretty minor piece of validation to do, because there are other things that aren't validated, such as the whole HID report descriptor that is passed in. But it is another use for dupe checking.

[gamblor21]

The build failed for one board on one translation, running out of space. Any thoughts on what to do here?

[jepler]

Looks perfectly straightforward. No testing done.

[dhalbert]

Since this only checks pins, we can say "pins" instead of "objects"

Suggested change

	mp_raise_TypeError_varg(translate("%q and %q contain duplicate objects"), arg_name1, arg_name2);
	mp_raise_TypeError_varg(translate("%q and %q contain duplicate pins"), arg_name1, arg_name2);

[gamblor21]

Good catch, missed it the second time.

[dhalbert]

OK, great, thanks for all the discussion and your patience with this!