On 29/05/18(Tue) 22:29, David Bern wrote:
> Sorry for the spamming.
> After some research and finding that my fix for issue nr: 2 (
> hid_usage_in_page() )
> will break the functionality inside /usr.bin/usbhidaction/usbhidaction.c
> https://goo.gl/1cWFtR (link to usbhidaction.c)
>
> I now change my patch to only include a fix for issue nr: 1
> More details is described in the previous mail
Are you sure the name always contain an underscore? Can't it be
"Button:Button42" ?
You're passing the string directly to sscanf(3), is that safe?
Don't you want to parse only unsigned numbers? Or is it possible to
have "Button:Button_-3" ?
> Index: usage.c
> ===================================================================
> RCS file: /cvs/src/lib/libusbhid/usage.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 usage.c
> --- usage.c 8 Oct 2014 04:49:36 -0000 1.16
> +++ usage.c 29 May 2018 19:45:25 -0000
> @@ -265,8 +265,9 @@ int
> hid_parse_usage_in_page(const char *name)
> {
> const char *sep;
> + const char *usage_sep;
> unsigned int l;
> - int k, j;
> + int k, j, us, parsed_usage;
>
> sep = strchr(name, ':');
> if (sep == NULL)
> @@ -278,9 +279,19 @@ hid_parse_usage_in_page(const char *name
> return -1;
> found:
> sep++;
> - for (j = 0; j < pages[k].pagesize; j++)
> + for (j = 0; j < pages[k].pagesize; j++) {
> + us = pages[k].page_contents[j].usage;
> + if (us == -1) {
> + usage_sep = strchr(sep, '_');
> + if (usage_sep == NULL)
> + return -1;
> + if (sscanf(usage_sep, "_%d", &parsed_usage))
> + return (pages[k].usage << 16) |
> + parsed_usage;
> + }
> if (strcmp(pages[k].page_contents[j].name, sep) == 0)
> return (pages[k].usage << 16) |
> pages[k].page_contents[j].usage;
> + }
> return -1;
> }
>
>
> comments? ok?
>
> 2018-05-28 13:01 GMT+02:00 David Bern <david.ml.b...@gmail.com>:
>
> > I was suggested off list to give an explanation on what the patch does.
> >
> > So please, tell me if I need to clarify more, or make further changes to
> > the code.
> >
> > The patch tries to fix two things.
> > 1. Changes in hid_parse_usage_in_page() fixes problems in parsing usages
> > defined as: * Button %d
> >
> > hid_parse_usage_in_page():
> > Previously - With input "Button:Button_1" returns -1
> > Now - With input "Button:Button_1" returns 589825
> >
> > In the scenario of parsing Button:Button_1 we will not find a usage name
> > matching that string. For example Button:Button_1 is defined as
> > Button %d in the table.
> >
> > We are still able to calculate the proper usage number in the same way we
> > are
> > able to calculate the proper usage name in hid_usage_in_page().
> >
> > The first step is to identify if usage name is shortened. If it is,
> > usage will hold a value of -1. Then I try to locate a separator char in
> > the name as "_".
> > If a separator char is found I use it to read the value as "_%d" to get
> > the
> > corresponding usage number
> >
> > >+ us = pages[k].page_contents[j].usage;
> > >+ if (us == -1) {
> > >+ usage_sep = strchr(sep, '_');
> > >+ if (usage_sep == NULL)
> > >+ return -1;
> > >+ if (sscanf(usage_sep, "_%d", &parsed_usage))
> > >+ return (pages[k].usage << 16) |
> > >+ parsed_usage;
> >
> >
> > 2. The text-string that is returned by hid_usage_in_page() misses page
> > information.
> > So the changes made in hid_usage_in_page() is to make it the inverse of
> > hid_parse_usage_in_page()
> >
> > In details what the code previously did and now does.
> >
> > hid_usage_in_page():
> > Previously - With input 589825 returns Button_1
> > Now - With input 589825 returns Button:Button_1
> >
> >
> > The change just adds a pages[k].name to the string, a format that
> > hid_parse_usage_in_page() expects it to have.
> > I make formatting in two steps when us == -1 which it is when usage is
> > shortened
> > as for example: * Button %d.
> >
> > >+ snprintf(fmt, sizeof fmt,
> > >+ "%%s:%s", pages[k].page_contents[j].name);
> > >+ snprintf(b, sizeof b, fmt, pages[k].name, i);
> >
> > The first step is to create a format string that will result in something
> > like
> > "%s:Button_%d".
> > The last step I use the fmt-string to create a complete string that will
> > result in
> > "Button:Button_1"
> >
> >
> >
> >
> > 2018-05-24 18:44 GMT+02:00 David Bern <david.ml.b...@gmail.com>:
> >
> >> While I was waiting for comments and feedback I came up with some
> >> improvements.
> >> The "logic" is still the same, but the execution is hopefully more sane.
> >>
> >> Index: usage.c
> >> ===================================================================
> >> RCS file: /cvs/src/lib/libusbhid/usage.c,v
> >> retrieving revision 1.16
> >> diff -u -p -r1.16 usage.c
> >> --- usage.c 8 Oct 2014 04:49:36 -0000 1.16
> >> +++ usage.c 24 May 2018 16:37:54 -0000
> >> @@ -224,6 +224,7 @@ hid_usage_in_page(unsigned int u)
> >> {
> >> int i = HID_USAGE(u), j, k, us;
> >> int page = HID_PAGE(u);
> >> + char fmt[100];
> >> static char b[100];
> >>
> >> for (k = 0; k < npages; k++)
> >> @@ -234,12 +235,16 @@ hid_usage_in_page(unsigned int u)
> >> for (j = 0; j < pages[k].pagesize; j++) {
> >> us = pages[k].page_contents[j].usage;
> >> if (us == -1) {
> >> - snprintf(b, sizeof b,
> >> - pages[k].page_contents[j].name, i);
> >> + snprintf(fmt, sizeof fmt,
> >> + "%%s:%s", pages[k].page_contents[j].name);
> >> + snprintf(b, sizeof b, fmt, pages[k].name, i);
> >> + return b;
> >> + }
> >> + if (us == i) {
> >> + snprintf(b, sizeof b, "%s:%s", pages[k].name,
> >> + pages[k].page_contents[j].name);
> >> return b;
> >> }
> >> - if (us == i)
> >> - return pages[k].page_contents[j].name;
> >> }
> >> bad:
> >> snprintf(b, sizeof b, "0x%04x", i);
> >> @@ -265,8 +270,9 @@ int
> >> hid_parse_usage_in_page(const char *name)
> >> {
> >> const char *sep;
> >> + const char *usage_sep;
> >> unsigned int l;
> >> - int k, j;
> >> + int k, j, us, parsed_usage;
> >>
> >> sep = strchr(name, ':');
> >> if (sep == NULL)
> >> @@ -278,9 +284,19 @@ hid_parse_usage_in_page(const char *name
> >> return -1;
> >> found:
> >> sep++;
> >> - for (j = 0; j < pages[k].pagesize; j++)
> >> + for (j = 0; j < pages[k].pagesize; j++) {
> >> + us = pages[k].page_contents[j].usage;
> >> + if (us == -1) {
> >> + usage_sep = strchr(sep, '_');
> >> + if (usage_sep == NULL)
> >> + return -1;
> >> + if (sscanf(usage_sep, "_%d", &parsed_usage))
> >> + return (pages[k].usage << 16) |
> >> + parsed_usage;
> >> + }
> >> if (strcmp(pages[k].page_contents[j].name, sep) == 0)
> >> return (pages[k].usage << 16) |
> >> pages[k].page_contents[j].usage;
> >> + }
> >> return -1;
> >> }
> >>
> >>
> >>
> >> 2018-05-21 23:12 GMT+02:00 David Bern <david.ml.b...@gmail.com>:
> >>
> >>> First diff "solves" point 1 & 2. Second diff is on the parts of the
> >>> manual that does not match the usbhid.h
> >>>
> >>> Index: usage.c
> >>> ===================================================================
> >>> RCS file: /cvs/src/lib/libusbhid/usage.c,v
> >>> retrieving revision 1.16
> >>> diff -u -p -r1.16 usage.c
> >>> --- usage.c 8 Oct 2014 04:49:36 -0000 1.16
> >>> +++ usage.c 21 May 2018 21:06:24 -0000
> >>> @@ -224,6 +224,7 @@ hid_usage_in_page(unsigned int u)
> >>> {
> >>> int i = HID_USAGE(u), j, k, us;
> >>> int page = HID_PAGE(u);
> >>> + char usage_name[100];
> >>> static char b[100];
> >>>
> >>> for (k = 0; k < npages; k++)
> >>> @@ -234,12 +235,18 @@ hid_usage_in_page(unsigned int u)
> >>> for (j = 0; j < pages[k].pagesize; j++) {
> >>> us = pages[k].page_contents[j].usage;
> >>> if (us == -1) {
> >>> - snprintf(b, sizeof b,
> >>> + snprintf(usage_name, sizeof usage_name,
> >>> pages[k].page_contents[j].name, i);
> >>> + snprintf(b, sizeof b,
> >>> + "%s:%s", pages[k].name, usage_name);
> >>> + return b;
> >>> + }
> >>> + if (us == i) {
> >>> + snprintf(b, sizeof b,
> >>> + "%s:%s", pages[k].name,
> >>> + pages[k].page_contents[j].name);
> >>> return b;
> >>> }
> >>> - if (us == i)
> >>> - return pages[k].page_contents[j].name;
> >>> }
> >>> bad:
> >>> snprintf(b, sizeof b, "0x%04x", i);
> >>> @@ -265,8 +272,9 @@ int
> >>> hid_parse_usage_in_page(const char *name)
> >>> {
> >>> const char *sep;
> >>> + const char *usage_sep;
> >>> unsigned int l;
> >>> - int k, j;
> >>> + int k, j, us, parsed_usage;
> >>>
> >>> sep = strchr(name, ':');
> >>> if (sep == NULL)
> >>> @@ -277,10 +285,20 @@ hid_parse_usage_in_page(const char *name
> >>> goto found;
> >>> return -1;
> >>> found:
> >>> + usage_sep = strchr(sep, '_');
> >>> sep++;
> >>> - for (j = 0; j < pages[k].pagesize; j++)
> >>> + for (j = 0; j < pages[k].pagesize; j++) {
> >>> + us = pages[k].page_contents[j].usage;
> >>> + if (us == -1) {
> >>> + if (usage_sep == NULL)
> >>> + return -1;
> >>> + if (sscanf(usage_sep, "_%d", &parsed_usage))
> >>> + return (pages[k].usage << 16 |
> >>> + parsed_usage);
> >>> + }
> >>> if (strcmp(pages[k].page_contents[j].name, sep) == 0)
> >>> return (pages[k].usage << 16) |
> >>> pages[k].page_contents[j].usage;
> >>> + }
> >>> return -1;
> >>> }
> >>>
> >>> Index: usbhid.3
> >>> ===================================================================
> >>> RCS file: /cvs/src/lib/libusbhid/usbhid.3,v
> >>> retrieving revision 1.17
> >>> diff -u -p -r1.17 usbhid.3
> >>> --- usbhid.3 13 May 2014 14:05:02 -0000 1.17
> >>> +++ usbhid.3 21 May 2018 21:02:21 -0000
> >>> @@ -65,22 +65,22 @@
> >>> .Fn hid_report_size "report_desc_t d" "hid_kind_t k" "int id"
> >>> .Ft int
> >>> .Fn hid_locate "report_desc_t d" "u_int usage" "hid_kind_t k"
> >>> "hid_item_t *h" "int id"
> >>> -.Ft char *
> >>> +.Ft const char *
> >>> .Fn hid_usage_page "int i"
> >>> -.Ft char *
> >>> +.Ft const char *
> >>> .Fn hid_usage_in_page "u_int u"
> >>> .Ft int
> >>> .Fn hid_parse_usage_page "const char *"
> >>> -.Ft char *
> >>> +.Ft int
> >>> .Fn hid_parse_usage_in_page "const char *"
> >>> .Ft void
> >>> .Fn hid_init "char *file"
> >>> .Ft int
> >>> .Fn hid_start "char *file"
> >>> -.Ft int
> >>> +.Ft int32_t
> >>> .Fn hid_get_data "void *data" "hid_item_t *h"
> >>> .Ft void
> >>> -.Fn hid_set_data "void *data" "hid_item_t *h" "u_int data"
> >>> +.Fn hid_set_data "void *data" "hid_item_t *h" "int32_t data"
> >>> .Sh DESCRIPTION
> >>> The
> >>> .Nm
> >>>
> >>>
> >>>
> >>> 2018-05-21 12:07 GMT+02:00 Martin Pieuchot <m...@openbsd.org>:
> >>>
> >>>> On 18/05/18(Fri) 10:01, David Bern wrote:
> >>>> > Hello!
> >>>> >
> >>>> > Have been using libusbhid and discovered a couple of discrepancies in
> >>>> > the man-page (libusbhid.3) and the source of usage.c
> >>>> >
> >>>> > Some are just factual misses, but I also got (what I think is) 2
> >>>> errors.
> >>>> > I will try to explain them;
> >>>> >
> >>>> > 1. (This is the I think is an error but not sure). The man-page tells
> >>>> me
> >>>> > that hid_usage_in_page and hid_parse_usage_in_page are each
> >>>> > others inverse.
> >>>> > If I haven't misunderstood the practical meaning of inverse in this
> >>>> > case then this should be true:
> >>>> > x == hid_parse_usage_in_page(hid_usage_in_page(x)).
> >>>> >
> >>>> > My observation:
> >>>> > The main reason to why this isnt true, is that hid_usage_in_page()
> >>>> > returns the data of pages[k].page_contents[j].name
> >>>> > while hid_parse_usage_in_page() expects the data to
> >>>> > contain "%s:%s", pages[k].name, pages[k].page_contents[j].name
> >>>> >
> >>>> > The reason I ask instead of just posting working code is this:
> >>>> > Am I misunderstanding the manual? In that case, the solution I want
> >>>> > to send in is a change in that sentence
> >>>> > Or Is the manual correct and the behavior of hid_usage_in_page()
> >>>> wrong,
> >>>> > is this something I can correct without breaking other systems?
> >>>> >
> >>>> >
> >>>> > 2. The second error I found is located in hid_parse_usage_in_page().
> >>>> > It is unable to parse values found in page Button.
> >>>> >
> >>>> > My observation:
> >>>> > usage.c is using a standard table named usb_hid_pages. In that table
> >>>> > we got a page called Buttons.
> >>>> > the usages in that page is shortened to "* Button %d".
> >>>> > I believe this is the cause of why hid_parse_usage_in_page() is
> >>>> getting
> >>>> > the pagesize "wrong" and therefor unable to parse any button in the
> >>>> > button page. I guess this is the case with other similar cases as
> >>>> well.
> >>>> >
> >>>> > my conclusion is that it would be possible to handle similar cases in
> >>>> a
> >>>> > similar way as it is handled in hid_usage_in_page().
> >>>> >
> >>>> >
> >>>> > As this is my first "issue" I would love to get as much feedback as
> >>>> > possible so I can work on delivering a desirable and usable patch in
> >>>> > my first try.
> >>>>
> >>>> Just send a diff, then we'll look at it 8)
> >>>>
> >>>
> >>>
> >>
> >
