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

Extend Usb recv control + some HID definitions #4105

Closed
wants to merge 2 commits into from

Conversation

NicoHood
Copy link
Contributor

@NicoHood NicoHood commented Nov 7, 2015

In order to implement some fancy feature/out requests/reports I need to read more than 64 bytes from the control pipe. I could work around this to implement this in my own library, but since this was marked as TODO why not add it? There is still no timeout in the function, expecting the user is knowing what he is doing.

The HID definitions are required to differentiate between those 3 types.

FYI:
You can abuse a Keyboard this way under linux, that it works almost like a raw hid device, just passive. Meaning you can send data from the PC and request data. You just cannot active send data. not idea about windows though. With consumer keys in the reserved byte and bios compatibility the Keyboard gets REALLY powerful now. Just different keylayouts are missing yet.
NicoHood/HID@a8c292a
NicoHood/HID@0487754
(Hyperion patch is not published yet, but it doesnt require much. You can use this as quick and reliable ambilight!)

However we should really consider to renew the USB Core API this time. If you really want to use some basic functions, they are a) not visible via .h file, only via .cpp file or b) not useful/unclear. Any chance to see some updates here?

@cmaglie @facchinm

@NicoHood
Copy link
Contributor Author

Hyperion patch released and merged:
hyperion-project/hyperion#407

Please consider to merge this PR or give me feedback if you reject it. We can also add it as 2nd function for long control EP reads. However people are not able to test the latest dev again.

@cmaglie cmaglie added the Component: USB Device Opposed to USB Host. Related to the USB subsystem (SerialUSB, HID, ...) label Nov 12, 2015
@NicoHood
Copy link
Contributor Author

Could anyone please have a look at this?
I'd like to publish the new dev version of HID Project, since the current version has a .development file inside which causes problems. But before I need this PR to be merged. Or at least let me know if you reject this PR.

@sandeepmistry

@matthijskooijman
Copy link
Collaborator

I had a look at this PR, looks ok to me. I commented one nitpick about the looping used, but that's not very important. I can't comment on to what degree the first commit actually works with the USB hardware, since I'm not familiar with that part, but it looks reasonable to me (and I expect @NicoHood has tested it).

@facchinm
Copy link
Member

facchinm commented Dec 9, 2015

Hi Nico,
sorry for the long delay but I've been really busy with other tasks.
The commit adding the defines is surely ok, about the other one I believe it's ok too (in fact it works and it keeps the same behaviour as master code for messages < 64 byte) but every time I read (u8*)d + len - length I can't understand where the pointer is going to be 😉
In fact, the rationale is perfect but both code and comments are quite confusing (as @matthijskooijman noted).
Also, it adds 50bytes to flash usage (maybe someone will notice).
Do you really need that patch? Can't you use other endpoints for recv operations?

@NicoHood
Copy link
Contributor Author

NicoHood commented Dec 9, 2015

I need the Control endpoints, there is no other way.

However we could make it a different function like "USB_RecvControlLong()" ore something like that. Then we dont need to add an overhead to the main API. However it adds even more if someone uses my function, so we have a duplication here.

but every time I read (u8*)d + len - length I can't understand where the pointer is going to be 😉

I dont understand your question. You need to add an offset to the array to write to every position. First you write to the first position (len(aka transmit length) - length(aka bytes left) = 0) then to the next position (len - length(x bytes left) = 64).

This results in:

  • Writing to 0, 64, 128 etc to fill the buffer with 64 bytes packages (except the last).

Maybe the function can be improved, but the USB API itself is not flexible enough for this. Maybe we should adapt the Lufa core functions and build ontop of this. This makes the usb core more stable, still flexible and it has a good documentation.

For now I just need a decision about this. Please merge the first commit and tell me if you want to merge the 2nd or not. If not I can make a different named function or simply add it into my library as special patch (which isnt that ultra nice, but would work for now.)

@facchinm
Copy link
Member

Hi Nico,
the operation is clear when you read very carefully (because length is decrementing) while the comment
// Write data to fit to the end (not the beginning) of the array
makes me think (wrongly) that we are filling the array from the end.

I'm still curious about where you would need to use that patch; if the case is to revive this code (from HID library)

    if (request == HID_SET_REPORT)
    {
        //uint8_t reportID = setup.wValueL;
        //uint16_t length = setup.wLength;
        //uint8_t data[length];
        // Make sure to not read more data than USB_EP_SIZE.
        // You can read multiple times through a loop.
        // The first byte (may!) contain the reportID on a multreport.
        //USB_RecvControl(data, length);
    }

you can perform the loop externally. The only live code which uses that function is CDC and receives 7 bytes so your use-case is too narrow for the overall overhead, sorry.

Anyway, I'm merging the first one immediately

@facchinm
Copy link
Member

9b66f5d applied

@NicoHood
Copy link
Contributor Author

Thanks!
I need it here:
https://github.com/NicoHood/HID/blob/dev/src/SingleReport/RawHID.cpp#L126

However I could add this as internal function myself to the lib. Or as suggested as different named function. I should rename the variables though. What about adding a 2nd function for long controlRecv calls? What name do you suggest?

@facchinm
Copy link
Member

Hi Nico,
seeing that piece of code I believe you can retrieve 64 bytes n times using the same approach of the PR (calculating how many iterations are needed and calling USB_RecvControl + updating the buffer head after every iteration) without adding any function to the core.
Soon we'll need to rework heavily the whole USB core (and give it a cross-core API) so I wouldn't add any public function in the meantime. Anyway I'd really appreciate if you could help us in this process 😄

@NicoHood
Copy link
Contributor Author

Sure, just let me know.
I'd then also like to get a bit more information. When 1.6.6 was released I was a bit overwhelmed. HID Project was not really release ready at that time. I am willing to help and test, but then we should exchange a bit more information.

Also I asked cmagile and ffisore via mail if i can get access to trigger the arduino bot. I always have to ask matthijs on irc for it. Also not all arduino members provide a PGP mail key which is really bad. I do not send unencrypted mails.

Closing this PR, will add a wrapper to my program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: USB Device Opposed to USB Host. Related to the USB subsystem (SerialUSB, HID, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants