-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add an alternative way to number the USB endpoints #2189
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
Add an alternative way to number the USB endpoints #2189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! I like how it can simplify SAMD21 MSC too. One naming suggestion and a couple of other suggestions on the other PR.
Thank you for your feedback! I have added changes according to your suggestions. |
Two options available: - relative numbering (USB_RELATIVE_EP_NUM = 1) - default - absolute numbering (USB_RELATIVE_EP_NUM = 0) - new!
1dc00f9
to
1205d3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you!
@dhalbert please re-review and merge if it looks ok. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor style stuff.
tools/gen_usb_descriptor.py
Outdated
if 'CDC' in args.devices: | ||
if (args.cdc_ep_num_notification == 0 or args.cdc_ep_num_data_out == 0 or | ||
args.cdc_ep_num_data_in == 0): | ||
raise ValueError("Endpoint address must not be 0") | ||
|
||
if 'MSC' in args.devices: | ||
if args.msc_ep_num_out == 0 or args.msc_ep_num_in == 0: | ||
raise ValueError("Endpoint address must not be 0") | ||
|
||
if 'HID' in args.devices: | ||
if args.args.hid_ep_num_out == 0 or args.hid_ep_num_in == 0: | ||
raise ValueError("Endpoint address must not be 0") | ||
|
||
if 'AUDIO' in args.devices: | ||
if args.midi_ep_num_out == 0 or args.midi_ep_num_in == 0: | ||
raise ValueError("Endpoint address must not be 0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the specific endpoint names here? We don't need to save space. E.g. "CDC Endpoint address must not be 0"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tools/gen_usb_descriptor.py
Outdated
parser.add_argument('--msc_num_endpoint_pairs', type=int, default=1, | ||
help='Use 1 or 2 endpoint pairs for MSC (1 bidirectional, or 1 input + 1 output (required by SAMD21))') | ||
parser.add_argument('--renumber_endpoints', type=int, default=1, | ||
help='use relative(1) or absolute(0) endpoint number') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too important, but a nice style for boolean argparse
arguments is given here: https://stackoverflow.com/a/15008806/142996
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
The way to disable and enable endpoint renumbering has been changed. Now instead of using '--renumber-endpoint 1' or '--renumber-endpoint 0' we can use the '--no-renumber_endpoint' argument to disable renumbering. By default, endpoints are renumbered without providing this argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you!
Two options available:
It needs: adafruit/usb_descriptor#8