Skip to content

[PluggableHID] API simplification proposal #3840

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 28, 2015
Merged

[PluggableHID] API simplification proposal #3840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0cf13c
HID: Renamed fields in HIDDescriptorListNode and HID_Descriptor
cmaglie Sep 21, 2015
401c4f0
HID: merged HID_Descriptor into HIDDescriptorListNode
cmaglie Sep 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions hardware/arduino/avr/libraries/HID/HID.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ int HID_GetDescriptor(int8_t t)
HIDDescriptorListNode* current = rootNode;
int total = 0;
while(current != NULL) {
total += USB_SendControl(TRANSFER_PGM,current->cb->descriptor,current->cb->length);
total += USB_SendControl(TRANSFER_PGM,current->data,current->length);
current = current->next;
}
return total;
Expand All @@ -82,7 +82,7 @@ void HID_::AppendDescriptor(HIDDescriptorListNode *node)
current->next = node;
}
modules_count++;
sizeof_hidReportDescriptor += (uint16_t)node->cb->length;
sizeof_hidReportDescriptor += (uint16_t)node->length;
}

void HID_::SendReport(u8 id, const void* data, int len)
Expand Down
11 changes: 4 additions & 7 deletions hardware/arduino/avr/libraries/HID/HID.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,13 @@
#define HID_REPORT_DESCRIPTOR_TYPE 0x22
#define HID_PHYSICAL_DESCRIPTOR_TYPE 0x23

typedef struct __attribute__((packed)) {
uint16_t length;
const void* descriptor;
} HID_Descriptor;

class HIDDescriptorListNode {
public:
HIDDescriptorListNode *next = NULL;
const HID_Descriptor * cb;
HIDDescriptorListNode(const HID_Descriptor *ncb) {cb = ncb;}
HIDDescriptorListNode(const void *d, uint16_t l) : data(d), length(l) { }

const void* data;
uint16_t length;
};

class HID_
Expand Down
6 changes: 3 additions & 3 deletions hardware/arduino/sam/libraries/HID/HID.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ int HID_GetDescriptor(int8_t t)
HIDDescriptorListNode* current = rootNode;
int total = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you always use int in the Arduino programs? uint16_t would make life much easier in most cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, since I'm not the author of the USB stack. I guess the reason is that USBD_SendControl returns -1 in case of failure.

(well, for the sake of precision, it seems that USBD_SendControl in the SAM core will never returns -1 but the equivalent function in the AVR core, USB_SendControl does, probably this is something that has been inherited when the USB stack has been ported from AVR to SAM).

To fix this we probably should handle the error condition, and eventually return -1 if one call to USB(D)_SendControl fails.

while(current != NULL) {
total += USBD_SendControl(0,current->cb->descriptor,current->cb->length);
total += USBD_SendControl(0,current->data,current->length);
current = current->next;
}
return total;
Expand All @@ -89,7 +89,7 @@ void HID_::AppendDescriptor(HIDDescriptorListNode *node)
current->next = node;
}
modules_count++;
sizeof_hidReportDescriptor += node->cb->length;
sizeof_hidReportDescriptor += node->length;
}

void HID_::SendReport(uint8_t id, const void* data, int len)
Expand Down Expand Up @@ -165,4 +165,4 @@ HID_::HID_(void)
int HID_::begin(void)
{
return 0;
}
}
12 changes: 4 additions & 8 deletions hardware/arduino/sam/libraries/HID/HID.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,12 @@
#define HID_REPORT_DESCRIPTOR_TYPE 0x22
#define HID_PHYSICAL_DESCRIPTOR_TYPE 0x23

typedef struct __attribute__((packed)) {
uint8_t length;
const void* descriptor;
} HID_Descriptor;

class HIDDescriptorListNode {
public:
HIDDescriptorListNode *next = NULL;
const HID_Descriptor * cb;
HIDDescriptorListNode(const HID_Descriptor *ncb) {cb = ncb;}
HIDDescriptorListNode(const void *d, uint16_t l) : data(d), length(l) { }
uint8_t length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint16_t? Also this could be const (avr as well)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint16_t?

It should allow for descriptors >256.

Also this could be const (avr as well)?

Mbah, does it really matter? l is passed by copy so the method can't change the "original" parameter in any way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. but its uint8_t. look ;D

Its not about changing permissions, its about code style, debugging and compiler optimization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, right good catch! ;-)

const void* data;
};

class HID_
Expand Down Expand Up @@ -90,4 +86,4 @@ typedef struct

#define WEAK __attribute__ ((weak))

#endif
#endif
6 changes: 1 addition & 5 deletions libraries/Keyboard/src/Keyboard.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,7 @@ static const uint8_t _hidReportDescriptor[] PROGMEM = {

Keyboard_::Keyboard_(void)
{
static HID_Descriptor cb = {
.length = sizeof(_hidReportDescriptor),
.descriptor = _hidReportDescriptor,
};
static HIDDescriptorListNode node(&cb);
static HIDDescriptorListNode node(_hidReportDescriptor, sizeof(_hidReportDescriptor));
HID.AppendDescriptor(&node);
}

Expand Down
6 changes: 1 addition & 5 deletions libraries/Mouse/src/Mouse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,7 @@ static const uint8_t _hidReportDescriptor[] PROGMEM = {

Mouse_::Mouse_(void) : _buttons(0)
{
const static HID_Descriptor cb = {
.length = sizeof(_hidReportDescriptor),
.descriptor = _hidReportDescriptor,
};
static HIDDescriptorListNode node(&cb);
static HIDDescriptorListNode node(_hidReportDescriptor, sizeof(_hidReportDescriptor));
HID.AppendDescriptor(&node);
}

Expand Down