Hi On Mon, Jul 28, 2014 at 12:21 PM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > Hi David, Murat, > > On Mon, Jul 28, 2014 at 11:53 AM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote: >> Hi >> >>>> The kernel bails out if size==0, so this never happens. The bt_uhid_* >>>> helpers are fine, however, hog.c forward_report() is totally broken. >>>> You use: >>>> >>>> bt_uhid_register(hogdev->uhid, UHID_OUTPUT, forward_report, hogdev); >>>> bt_uhid_register(hogdev->uhid, UHID_FEATURE, forward_report, hogdev); >>>> >>>> Therefore, you register *THE SAME* handler for UHID_OUTPUT *AND* >>>> UHID_FEATURE. However, in forward_report() you access ev->u.output, >>>> but this is only valid for UHID_OUTPUT. If you get a UHID_FEATURE >>>> report you must never access anything but ev->u.feature! >>> >>> Does the ev->u.features structure matches HID protocol, can I send as >>> it is or do I need to reformat it? >> >> The uhid.h header includes these definitions: >> >> struct uhid_feature_req { >> __u32 id; >> __u8 rnum; >> __u8 rtype; >> } __attribute__((__packed__)); >> >> struct uhid_feature_answer_req { >> __u32 id; >> __u16 err; >> __u16 size; >> __u8 data[UHID_DATA_MAX]; >> } __attribute__((__packed__)); > > That I got it, what I was asking was if that is the exact same format > used in HID so that I could sent as it is to the remote device. Im > afraid the answer is no because id and some other fields seems to be > just for context tracking as you mention bellow. Sorry, I wasn't really clear. uhid_feature_req contains 3 fields: "id" contains a random ID from the kernel. This has no meaning besides tracking feature_req and feature_answer_req. That is, once your device answers to the feature request, you must copy the ID into the uhid_feature_answer_req and send it to the kernel. Note that UHID ever only has one uhid_feature_req is parallel. Therefore, you can safe the "id" field in some global context and use it for the next uhid_feature_answer_req you send. If the kernel cancelled a request (timeout, interrupted, ...), it will ignore any answer with that ID in the future. "rnum": This is the report-number of the feature request. "rtype": This is the report-type of the feature request. It can be INPUT, OUTPUT or FEATURE. This is really misleading as you might expect this is always FEATURE. That's not true though. The UHID_FEATURE request really should have been called UHID_GET_REPORT. What you should do once you receive a UHID_FEATURE event from the kernel, is send a GET_REPORT request to the device. Setting the report-number to "rnum" and the report-type to "rtype". Once the device answers to the GET_REPORT, write a "uhid_feature_answer_req" to the kernel by copying the "id" into it and the answer from the device. "err" should be set to 0. If something failed, set "err" to a error-number and leave the other fields untouched (besides "id"). The answer from the device should be copied untouched into "data" and "size" set to the length. The GET_REPORT logic is described in the HIDP specs, so I guess the HoG specs contain it, too. Note that the spec only allows a _single_ GET_REPORT requests at a time. Therefore, if you sent one out, you must wait for the answer before sending a new one. Luckily, the kernel enforces this, so it only sends a new request in case the old one timed out (5s I think, or 1s?). You can safely do the same in HoG. >>>> Furthermore, if you receive UHID_FEATURE, the kernel blocks until you >>>> send the answer as UHID_FEATURE_ANSWER. >>> >>> So if I got this right in addition to send the request we should >>> process the response as UHID_FEATURE_ANSWER otherwise it will block >>> other events? >> >> Kernel HID drivers can use a kernel-internal function to query an >> HID-feature. This call is blocking and waits for the answer from the >> device. uhid copied that design to avoid rewriting all drivers. >> Therefore, IFF you get an UHID_FEATURE event, you have to send the >> feature request to the device, somewhere store the context, and once >> the device answers you have to find that context again and write the >> answer via UHID_FEATURE_ANSWER to the kernel. The "id" field in >> uhid_feature_req and uhid_feature_answer_req must match and are used >> for context-tracking. >> >> Note that this is only used for HID drivers that require synchronous >> feature queries. All other drivers use UHID_OUTPUT for all outoing >> reports. The "rtype" field specifies whether it's an INPUT, OUTPUT or >> FEATURE report that has to be written. It it thus totally ok to ignore >> UHID_FEATURE as only few drivers use it. > > @Murat, I guess your input is needed here, why are you using > UHID_FEATURE instead of UHID_OUTPUT? Can we drop UHID_FEATURE or your > driver really requires it? I really recommend reading Documentation/hid/hid-transport.txt in the kernel sources. It's a pretty new document (linux-3.14 I think), but it describes the design of generic HID transport drivers in detail. The kernel-internal APIs have been adjusted accordingly, UHID still follows the old style. I'm currently reworking the API. I will put linux-bluetooth on CC once ready. It should be a lot easier to write HoG when you understand these internals, and hid-transport.txt was written for exactly that purpose. Please provide feedback to linux-input@xxxxxxxxxxxxxxx in case something is unclear. If you put me on CC, I promise to answer any questions. Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html