Skip to content

stmhal: fix USB HID Receive#2796

Closed
prusnak wants to merge 3 commits intomicropython:masterfrom
trezor:hidfix
Closed

stmhal: fix USB HID Receive#2796
prusnak wants to merge 3 commits intomicropython:masterfrom
trezor:hidfix

Conversation

@prusnak
Copy link
Copy Markdown
Contributor

@prusnak prusnak commented Jan 16, 2017

I found a bug in the USB HID Receive code that resulted in first read packet always containing no data just zeroes. All subsequent reads were OK.

This patch fixes the issue by adding Init method to HID_fops, which sets the buffer correctly even for the first packet.

Also there is a SNAK/CNAK mechanism that implements flow control in case user does not call recv method often enough (it tells host side to stop sending more data).

Copy link
Copy Markdown
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

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

Thanks!

I assume you tested the read-poll function?

Comment thread stmhal/usbd_hid_interface.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To make it robust, I think this function should initialise all the static variables (current_read_buffer, etc).

@prusnak
Copy link
Copy Markdown
Contributor Author

prusnak commented Jan 17, 2017

Added initialization of static variables to Init function.

@prusnak prusnak changed the title stmhal: fix USB HID Receive not receiving the first packet stmhal: fix USB HID Receive Jan 17, 2017
@prusnak
Copy link
Copy Markdown
Contributor Author

prusnak commented Jan 17, 2017

Added SNAK/CNAK mechanism for flow control.

Comment thread stmhal/usbd_hid_interface.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a short comment saying what this code does.

Comment thread stmhal/usbd_hid_interface.c Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add short comment here as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These shouldn't be exposed. Instead one should use the hid_out_ep variable defined in usbdev/class/src/usbd_cdc_msc_hid.c. Two options to make it work: 1) make a function in usbd_cdc_msc_hid.c to get the hid out EP (eg USBD_HID_GetOutEP); 2) add functions to usbd_cdc_msc_hid.c to set the SNAK/CNAK bits and call those functions from usbd_hid_interface.c. Probably the latter is cleaner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used the approach used in usbd_cdc_interface (exposing defines via .h file), but apporach 2) makes much more sense. Will update the PR.

@prusnak
Copy link
Copy Markdown
Contributor Author

prusnak commented Jan 18, 2017

Reworked last commit according to comments

@dpgeorge
Copy link
Copy Markdown
Member

Great, thank you! Rebased and merged, see 0883a7e

@dpgeorge dpgeorge closed this Jan 19, 2017
@prusnak prusnak deleted the hidfix branch January 19, 2017 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants