Skip to content
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

Feature request: Access to _lookup #330

Open
Gestas opened this issue Sep 25, 2020 · 12 comments
Open

Feature request: Access to _lookup #330

Gestas opened this issue Sep 25, 2020 · 12 comments

Comments

@Gestas
Copy link

Gestas commented Sep 25, 2020

Using this USB device as an example -

>>> dev = usb.core.find(idVendor=7531, idProduct=3)
... print(dev)

DEVICE ID 1c8b:0056 on Bus 002 Address 001 =================
 bLength                :   0x12 (18 bytes)
 bDescriptorType        :    0x1 Device
 bcdUSB                 :  0x300 USB 3.0
 bDeviceClass           :    0x9 Hub
 bDeviceSubClass        :    0x0
 bDeviceProtocol        :    0x3
...

I'd like the option to get access to the value of bDeviceClass as Hub via some method. I can think of two ways to do this; via utils.get_string something like this -

>>> dev = usb.core.find(idVendor=7531, idProduct=3)
... print(usb.util.get_string(dev, dev.bDeviceClass))

Hub

or adding a lookup export to usb.util something like this -

>>> type = "device"
... val = "9"
... print(usb.util.lookup(type, val))

Hub

where type would support descriptor | device | interface | bmattribute.

@jonasmalacofilho
Copy link
Member

This is a good idea.

Alternative #1 is not great because get_string has a pretty specific meaning in this context (get a string descriptor); but alternative #2 is ok, just not the easiest to use interface.

How about an IntEnum? I need to check, but it may provide enough backwards compatibility and would be a better abstraction. Although IIRC it's Python 3.6+, and so would need to wait until the end of the year.

@Gestas
Copy link
Author

Gestas commented Sep 28, 2020

@jonasmalacofilho

I don't think Enum will work here, member names can't have spaces.

This works -

class Shape(IntEnum):
... 	CIRCLE = 1
... 	SQUARE = 2
... 	
Shape(1)
<Shape.CIRCLE: 1>

This doesn't -

class DeviceClasses(IntEnum):
... 	"Specified at interface" = 1
... 	"Communications Device" = 2
... 	
  File "<input>", line 2
SyntaxError: cannot assign to literal

@jonasmalacofilho
Copy link
Member

Right, but the enum type can implement __str__ or __repr__ for the human readable names.

@Gestas
Copy link
Author

Gestas commented Sep 29, 2020

I didn't know that, thanks. The approach below might be simpler and it avoids the IntEnum requirement so we can implement it sooner. It also works as a IntEnum so we can update it to get the constraint later.

_lookup.py would look like this -

import sys
from enum import Enum

Descriptors = Enum(
    value="deviceid",
    names={
        "Specified at interface": 0x0,
        "Communications Device": 0x2,
        "Hub": 0x9,
        "Personal Healthcare Device": 0xF,
        "Diagnostic Device": 0xDC,
        "Wireless Controller": 0xE0,
        "Miscellaneous": 0xEF,
        "Vendor-specific": 0xFF,
    },
)

EpAttributes = Enum(
    value="deviceid",
    names={
        "Control": 0x0,
        "Isochronous": 0x1,
        "Bulk": 0x2,
        "Interrupt":  0x3,
        }
)
# Plus the other two tables


class Lookup(object):
    """ Returns names of attributes """
    def __init__(self, instance, value):
        self._instance = instance
        self._value = value

    def _lookup(self, default=""):
        try:
            self._instance = getattr(sys.modules[__name__], self._instance)
        except AttributeError:
            raise
        try:
            name = self._instance(self._value).name
            return name
        except ValueError:
            return default

    @property
    def name(self):
        return self._lookup()

We would need to update core.py -

from usb._lookup import Lookup

def _try_lookup(table, value):
    string = Lookup(instance=table, value=value)
    return string.name

Plus a few other minor changes. Thoughts? If you are OK with this I can submit a pull request.

@jonasmalacofilho
Copy link
Member

I was thinking of returing the enumerations directly (for example from <device>.bDeviceClass), instead of requiring the user to manually do the lookup.

In this context, the main issue I see with Enum is that it would break existing code that compares or looks up the currently returned values of the descriptors (e.g. <device>.bDeviceClass == 9).

# intenums.py

from enum import IntEnum

class _CustomNamedEnum(IntEnum):

    def __new__(cls, value, human_name):
        member = int.__new__(cls, value)
        member._value_ = value
        member._human_name = human_name
        return member

    def __str__(self):
        return self._human_name


class DeviceClasses(_CustomNamedEnum):
    SPECIFIED_AT_INTERFACE = (0x0, "Specified at interface")
    COMUNICATIONS_DEVICE = (0x2, "Communications Device")
    HUB = (0x9, "Hub")
    PERSONAL_HEALTHCARE_DEVICE = (0xf, "Personal Healthcare Device")
    DIAGNOSTIC_DEVICE = (0xdc, "Diagnostic Device")
    WIRELESS_CONTROLLER = (0xe0, "Wireless Controller")
    MISCELLANEOUS = (0xef, "Miscellaneous")
    VENDOR_SPECIFIC = (0xff, "Vendor-specific")


def test_basic_enum_api():
    some_class = DeviceClasses.HUB
    assert some_class == DeviceClasses.HUB
    assert some_class.value == 9
    assert some_class.name == "HUB"
    assert str(some_class) == "Hub"


def test_comparison_with_int():
    some_class = DeviceClasses.HUB
    assert some_class == 9


def test_lookup_on_int_dict():
    some_class = DeviceClasses.HUB
    table = { 9: "Hub Device" }
    assert table[some_class] == "Hub Device"
$ pytest -rA intenums.py
...
PASSED intenums.py::test_basic_enum_api
PASSED intenums.py::test_comparison_with_int
PASSED intenums.py::test_lookup_on_int_dict
...

While the above worked, renaming the file and replacing the beginning with:

# enums.py

from enum import Enum

class _CustomNamedEnum(bytes, Enum):

    def __new__(cls, value, human_name):
        member = bytes.__new__(cls, [value])
[...]

results in:

$ pytest -rA enums.py
========================================== test session starts ===========================================
platform linux -- Python 3.8.5, pytest-6.1.0, py-1.9.0, pluggy-0.13.1
rootdir: /home/jonas/Code/issues/pyusb/0330-human-readable-descriptors
collected 3 items                                                                                        

enums.py .FF                                                                                       [100%]

================================================ FAILURES ================================================
________________________________________ test_comparison_with_int ________________________________________

    def test_comparison_with_int():
        some_class = DeviceClasses.HUB
>       assert some_class == 9
E       assert <DeviceClasses.HUB: 9> == 9

enums.py:36: AssertionError
________________________________________ test_lookup_on_int_dict _________________________________________

    def test_lookup_on_int_dict():
        some_class = DeviceClasses.HUB
        table = { 9: "Hub Device" }
>       assert table[some_class] == "Hub Device"
E       KeyError: <DeviceClasses.HUB: 9>

enums.py:42: KeyError
================================================= PASSES =================================================
======================================== short test summary info =========================================
PASSED enums.py::test_basic_enum_api
FAILED enums.py::test_comparison_with_int - assert <DeviceClasses.HUB: 9> == 9
FAILED enums.py::test_lookup_on_int_dict - KeyError: <DeviceClasses.HUB: 9>
====================================== 2 failed, 1 passed in 0.04s =======================================

P.S. I was wrong and IntEnum doesn't require Python 3.6.


Apart from this, I think the enum names should be kept as valid Python identifiers. But there may be no need for a field like _human_name like I showed above.

(I thought about doing something like self.name.replace("_", " ").title() in __str__, but that wouldn't work in cases like OTG -> "OTG").

I'm fine with the use of the functional API, I didn't use it simply out of habit.

I can submit a pull request.

Please do! We can check and work on any remaining (or new issues if the CI complains) there.

@jonasmalacofilho
Copy link
Member

Hm, is it legal for a USB device to return an undefined value, say bDeviceClass: 0xf0 (instead of 0xff/vendor specific)?

If that's the case (with any of the affected tables) we would need to implement _missing_ before actually switching the fields to enums. And that really is Python 3.6+, so in the meantime we would need to keep the raw int values, and only use/experiment with the enums in _try_lookup.

@Gestas
Copy link
Author

Gestas commented Oct 3, 2020

Would you be comfortable with accepting a pull request implementing this as I described above for now? That approach already manages for device id's not in a table by returning default. I'll commit to delivering a new pull request that re-implements this to return it as a property of <device>.bDeviceClass using IntEnum and _missing_ when the project is ready for it.

@jonasmalacofilho
Copy link
Member

We shouldn't create a public interface just to deprecate it in three months. And as a private interface (=which a user could rely on at their own risk), I don't see a benefit over just using the current _lookup tables.

I also made a mistake in suggesting the use of enums: while IntEnum doesn't require Python 3.6, it and even basic Enum require Python 3. And I've also planned to keep Python 2.7 compatibility until the end of the year.[1]

@Gestas
Copy link
Author

Gestas commented Oct 5, 2020

@jonasmalacofilho sounds good. I don't want to start working on a new public interface now and end up with too many merge conflicts next year. Please do me a favour and update this ticket when you think the master branch is stable enough to support me building this for 3.6+ and I'll get started.

@jonasmalacofilho
Copy link
Member

Ok, thanks!

@mcuee
Copy link
Member

mcuee commented Jul 24, 2021

@jonasmalacofilho Just wondering if it is a good time to look at this again now. Thanks.

@jonasmalacofilho
Copy link
Member

I think so.

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

3 participants