ole00 / ch554_sdcc_usb_blinky

CH55x USB demo and Host controller app
11 stars 2 forks source link

suggested improvements #1

Open nerdralph opened 4 years ago

nerdralph commented 4 years ago

Nice work! Coincidentally, I have recently been working on a better way to represent string descriptors. The most important thing I was trying to avoid was manual size calculations. Here's how I'd change your code:

#define USB_CUST_VENDOR_NAME u"Acme"
struct {uint8_t bLength; uint8_t bDscType; uint16_t string[sizeof(USB_CUST_VENDOR_NAME)/2 -1];}
sd001 = {
    sizeof(sd001), USB_DESC_STR,
    USB_CUST_VENDOR_NAME
};

The reason for subtracting 1 from the string size is because the null termination is not needed.

I'd also recommend enums instead of defines for things like USB_DESC_STR, since they provide type safety. i.e.:

enum {USB_DESC_DEV = 0x01, USB_DESC_CFG, USB_DESC_STR, USB_DESC_INTF, USB_DESC_EP};
ole00 commented 4 years ago

Thanks for the suggestions. The descriptor name definition looks definitely better, but it does not compile in my environment (I copy / paste your code into usb_intr.h). Here is the error: .../projects/include/usb_intr.h:186: syntax error: token -> '"' ; column 73

enums: I agree about type safety argument of enums. For PC based apps and embedded projects compiled by fully fledged C compilers - by all means. For MCU projects that may be compiled (and ported) on feature-trimmed C compilers I prefer using plain defines. But fair point.

nerdralph commented 4 years ago

I just made a fork and will update the descriptor definition and create a pull request.

As for enums, they've been part of the language since K&R. What C compiler doesn't support enums?