Hi Petri,
>>> Enable HID protocol handling in userspace when uHIDP=true in input.conf.
>>> ---
>>> Makefile.plugins | 3 +-
>>> profiles/input/device.c | 677 +++++++++++++++++++++++++++++++++++++++++++--
>>> profiles/input/device.h | 1 +
>>> profiles/input/hidp_defs.h | 77 ++++++
>>> profiles/input/input.conf | 4 +
>>> profiles/input/manager.c | 9 +
>>> 6 files changed, 754 insertions(+), 17 deletions(-)
>>> create mode 100644 profiles/input/hidp_defs.h
>>>
>>> diff --git a/Makefile.plugins b/Makefile.plugins
>>> index 6a1ddbf..f0eada9 100644
>>> --- a/Makefile.plugins
>>> +++ b/Makefile.plugins
>>> @@ -55,7 +55,8 @@ builtin_sources += profiles/network/manager.c \
>>> builtin_modules += input
>>> builtin_sources += profiles/input/manager.c \
>>> profiles/input/server.h profiles/input/server.c \
>>> - profiles/input/device.h profiles/input/device.c
>>> + profiles/input/device.h profiles/input/device.c \
>>> + profiles/input/uhid_copy.h profiles/input/hidp_defs.h
>>>
>>> builtin_modules += hog
>>> builtin_sources += profiles/input/hog.c profiles/input/uhid_copy.h \
>>> diff --git a/profiles/input/device.c b/profiles/input/device.c
>>> index f1fa716..01891f4 100644
>>> --- a/profiles/input/device.c
>>> +++ b/profiles/input/device.c
>>> @@ -53,9 +53,13 @@
>>> #include "src/sdp-client.h"
>>>
>>> #include "device.h"
>>> +#include "hidp_defs.h"
>>> +#include "uhid_copy.h"
>>>
>>> #define INPUT_INTERFACE "org.bluez.Input1"
>>>
>>> +#define UHID_DEVICE_FILE "/dev/uhid"
>>> +
>>> enum reconnect_mode_t {
>>> RECONNECT_NONE = 0,
>>> RECONNECT_DEVICE,
>>> @@ -81,16 +85,30 @@ struct input_device {
>>> enum reconnect_mode_t reconnect_mode;
>>> guint reconnect_timer;
>>> uint32_t reconnect_attempt;
>>> + bool uhidp_enabled;
>>> + int uhid_fd;
>>> + guint uhid_watch;
>>> + bool uhid_created;
>>> + uint8_t report_req_pending;
>>> + guint report_req_timer;
>>> + uint32_t report_rsp_id;
>>> };
>>>
>>> static int idle_timeout = 0;
>>> +static bool uhidp_enabled = false;
>>
>> I am not really in favor of naming this uhidp. I realize how you get to this, but I am not sure this is a good naming. Maybe we should just stick with hidp for this.
>
> I picked this name following typical kernel approach to naming
> something that happens in userspace -- uhid, uinput, udev, etc.
> I don't know how to name this better. Do you feel "hidp" is better?
> input.conf would then have an item "hidp=<true|false>" as well.
internally we might just call this uhid_enabled. Since that is what we are doing here. We are using uHID. For HoG it just happens to be the only possible option.
For the input.conf file, I would propose using UserspaceDriver or maybe UserspaceHID. I am open for better names. I have not come up with a really good one yet.
>> And I am just thinking out load, but maybe creating a src/shared/hidp.[ch] as generalized implementation of the HID protocol over L2CAP might be something to think about it a bit. No requirement to do it right away, but something that should be considered.
>>
>
> Sounds good. Action item after initial commit.
>
>>>
>>> void input_set_idle_timeout(int timeout)
>>> {
>>> idle_timeout = timeout;
>>> }
>>>
>>> +void input_enable_uhidp(bool state)
>>> +{
>>> + uhidp_enabled = state;
>>> +}
>>> +
>>> static void input_device_enter_reconnect_mode(struct input_device *idev);
>>> +static int connection_disconnect(struct input_device *idev, uint32_t flags);
>>>
>>> static void input_device_free(struct input_device *idev)
>>> {
>>> @@ -124,14 +142,189 @@ static void input_device_free(struct input_device *idev)
>>> if (idev->reconnect_timer > 0)
>>> g_source_remove(idev->reconnect_timer);
>>>
>>> + if (idev->report_req_timer > 0)
>>> + g_source_remove(idev->report_req_timer);
>>> +
>>> g_free(idev);
>>> }
>>>
>>> +static bool hidp_send_message(GIOChannel *chan, uint8_t hdr,
>>> + const uint8_t *data, size_t size)
>>> +{
>>> + int fd;
>>> + ssize_t len;
>>> + uint8_t msg[size + 1];
>>> +
>>> + if (data == NULL)
>>> + size = 0;
>>> +
>>> + msg[0] = hdr;
>>> + if (size > 0)
>>> + memcpy(&msg[1], data, size);
>>> + ++size;
>>> +
>>> + fd = g_io_channel_unix_get_fd(chan);
>>> +
>>> + len = write(fd, msg, size);
>>> + if (len < 0) {
>>> + error("BT socket write error: %s (%d)", strerror(errno), errno);
>>> + return false;
>>> + }
>>> +
>>> + if (len < size) {
>>> + error("BT socket write error: partial write (%zd of %zu bytes)",
>>> + len, size);
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> +static bool hidp_send_ctrl_message(struct input_device *idev, uint8_t hdr,
>>> + const uint8_t *data, size_t size)
>>> +{
>>> + return hidp_send_message(idev->ctrl_io, hdr, data, size);
>>> +}
>>> +
>>> +static bool hidp_send_intr_message(struct input_device *idev, uint8_t hdr,
>>> + const uint8_t *data, size_t size)
>>> +{
>>> + return hidp_send_message(idev->intr_io, hdr, data, size);
>>> +}
>>> +
>>> +static bool uhid_send_feature_answer(struct input_device *idev,
>>> + const uint8_t *data, size_t size,
>>> + uint32_t id, uint16_t err)
>>> +{
>>> + struct uhid_event ev;
>>> + ssize_t len;
>>> +
>>> + if (data == NULL)
>>> + size = 0;
>>> +
>>> + if (size > sizeof(ev.u.feature_answer.data))
>>> + size = sizeof(ev.u.feature_answer.data);
>>> +
>>> + if (!idev->uhid_created) {
>>> + DBG("HID report (%zu bytes) dropped", size);
>>> + return false;
>>> + }
>>> +
>>> + memset(&ev, 0, sizeof(ev));
>>> + ev.type = UHID_FEATURE_ANSWER;
>>> + ev.u.feature_answer.id = id;
>>> + ev.u.feature_answer.err = err;
>>> + ev.u.feature_answer.size = size;
>>> +
>>> + if (size > 0)
>>> + memcpy(ev.u.feature_answer.data, data, size);
>>> +
>>> + len = write(idev->uhid_fd, &ev, sizeof(ev));
>>
>> Can we just use writev here and avoid the memcpy?
>
> I would prefer not to, initially. It gets a bit ugly calculating from
> field offsets how much to send from ev and how much from data. I
> modeled this based on report_value_cb() in hog.c.
Fair point. No problem. We can fix that later. Just keep it in mind.
> Also, this function is not getting called that often. It would be more
> beneficial for uhid_send_input_report() which is called for input
> events. But, it is even uglier there:
>
> struct uhid_input_req {
> __u8 data[UHID_DATA_MAX];
> __u16 size;
> } __attribute__((__packed__));
>
> struct uhid_event {
> __u32 type;
>
> union {
> struct uhid_create_req create;
> struct uhid_input_req input;
> struct uhid_output_req output;
> struct uhid_output_ev_req output_ev;
> struct uhid_feature_req feature;
> struct uhid_feature_answer_req feature_answer;
> } u;
> } __attribute__((__packed__));
>
> Because the size field is last in uhid_input_req, writev would need 4
> different vectors: type + actual data + rest of data filled with 0s +
> size.
>
> I have actually got this fixed in mainline kernel with new UHID_INPUT2
> + UHID_CREATE2 message types:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4522643aa9630be17238edf1b4c0b690c5dd7f5d
>
> Can we optimize these writes later?
If we can easily detect UHID_CREATE2 is available in the kernel or not, I am more than happy to go with what you have right now and then move on to a more optimized version. Just make sure it is easy enough to detect the support so we can have a graceful fallback.
>> And strictly speaking we should use an asynchronous non-blocking write handling here. Similar to what src/shared/io.[ch] is giving us the correct helpers for. Similar to how we implement it in src/shard/hci.[ch] or src/shared/mgmt.[ch].
>>
>
> I'm not familiar with those. I just modeled this based on how
> report_value_cb() does it in hog.c.
We can fix this later on as well. However have a look at how we do a POLLOUT before writing anything to a file descriptor. Long term goal is to turn into using fully asynchronous non-blocking IO for writing.
Regards
Marcel
--
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
