Opened 9 years ago

Closed 8 years ago

#6115 closed enhancement (fixed)

Extension of the USB HID Usages

Reported by: x-ist Owned by: axeld
Priority: normal Milestone: R1
Component: - General Version: R1/alpha2
Keywords: usb_hid.h Cc:
Blocked By: Blocking: #6114
Has a Patch: yes Platform: All

Description

The following patch advances the definitions in /trunk/src/add-ons/kernel/drivers/input/usb_hid of HID Usage Pages along with their Usage IDs to a large extend.

To provide a better overview and accessibility of the defined Usage IDs I decided to move them out of USB_hid.h into separate files, all being included by USB_hid.h. This prepares also for future implementations (e.g. of datastructures possibly required for specific HID Usage Pages).

Usage Pages start with HID_USAGE_PAGE_... Usage IDs start with HID_UID_<pn>_ Here <pn> represents a short version of the usage page.

Attachments (3)

USB_HID_USAGES.2.diff (70.8 KB) - added by x-ist 9 years ago.
Extension of USB HID Usages
USB_HID_USAGES.v2.diff (74.1 KB) - added by x-ist 9 years ago.
Discovered Typos corrected.
UsbHidDriver.diff (6.4 KB) - added by x-ist 9 years ago.
USB HID Driver adapted to the new names.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by x-ist

Has a Patch: set

Changed 9 years ago by x-ist

Attachment: USB_HID_USAGES.2.diff added

Extension of USB HID Usages

comment:3 Changed 9 years ago by x-ist

Sorry for resending multiple times! WebPositive has tricked me..

Correction: The location of USB_hid.h I meant was /trunk/headers/os/drivers/usb

comment:4 Changed 9 years ago by axeld

Owner: changed from nobody to mmlr
Status: newassigned

Thanks for your work!

Just a few remarks:

  • The changes of the HID usage IDs probably require an updated HID driver?
  • No blank line between the copyright header, and the header guard.
  • The references and other comments do not belong into the copyright header.
  • We usually don't add authors to public headers; they specify the system API, and even though someone actually took the time to wrote it, everyone needs to be okay with them - it's your call in the end, though.
  • Two blank lines between sections, ie. after the header guard, and before the last #endif.
  • If those should really be public headers, we should add the B_ prefix to all constants.
  • I find the header names a bit odd: why is hid_page lower case and Keyboard upper case? I would prefer lower case (with an extra _ for alphanumeric_display)
  • In general, the constant names look good IMO.

Maybe mmlr has some other comments, as he is more into the subject.

comment:5 Changed 9 years ago by x-ist

Have fixed the style-related objections above. So what about adding the "B_" prefix now? BTW, the updated HID driver will be submitted too.

comment:6 Changed 9 years ago by axeld

Great! I would be all for adding the "B_" prefix now, too.

comment:7 in reply to:  6 ; Changed 9 years ago by phoudoin

Replying to axeld:

Great! I would be all for adding the "B_" prefix now, too.

I'm against: all USB definitions are from external specifiers, either USB-IF or vendors. Neither Be Inc at his time neither us can influence USB standards specification. We made some effort to keep these definitions names from being as crypted as seen on other platforms, witch is great, but prefixing with B_ wont add any value but could make someone think it's specific to BeOS/Haiku.

On the other side, I'm for having one single prefix for every USB constants in our USB_*.h, which we don't have yet. Same should applied for typenames too (usb_*).

Anyway, if you choose to prefix all USB constants, there is several others USB_*.h and components including them to fix too for consistency.

To be short:

  1. I'm against prefixing with B_,
  2. but I'm for always prefixing with USB_

;-)

comment:8 in reply to:  7 Changed 9 years ago by x-ist

Replying to phoudoin:

I'm against: all USB definitions are from external specifiers, either USB-IF or vendors. Neither Be Inc at his time neither us can influence USB standards specification. We made some effort to keep these definitions names from being as crypted as seen on other platforms, witch is great, but prefixing with B_ wont add any value but could make someone think it's specific to BeOS/Haiku.

That's my opinion too.

On the other side, I'm for having one single prefix for every USB constants in our USB_*.h, which we don't have yet. Same should applied for typenames too (usb_*).

IMO, when working with USB one definitely knows about HID, such that using USB_ wouldn't povide more meaning to the name.

Here is an example of a name lend from the spec as is, with alternative prefixes:

HID_UID_WD_SCALE_STATUS_STABLE_AT_CENTER_OF_ZERO B_HID_UID_WD_SCALE_STATUS_STABLE_AT_CENTER_OF_ZERO USB_HID_UID_WD_SCALE_STATUS_STABLE_AT_CENTER_OF_ZERO

Even without the USB_ there is a bunch of prefixes and I would try to keep the prefixes smaller than the name though :-) Are there more points of view?

comment:9 Changed 9 years ago by phoudoin

Considering that the only other place where HID terminoly is used that I know of is Bluetooth, which is a wrapper to USB HID standard, I eventually agree with your position: HID_ current prefix is fine.

comment:10 Changed 9 years ago by axeld

The point of "B_" is not to make it Be/Haiku specific, the point is not to pollute the global name space. I'm aware that Be Inc. did not care about that for ie. the PCI definitions, but that doesn't mean we should follow here.

More opinions are, as always welcome :-)

comment:11 in reply to:  7 Changed 9 years ago by bonefish

Replying to phoudoin:

Replying to axeld:

Great! I would be all for adding the "B_" prefix now, too.

I'm against: all USB definitions are from external specifiers, either USB-IF or vendors. Neither Be Inc at his time neither us can influence USB standards specification. We made some effort to keep these definitions names from being as crypted as seen on other platforms, witch is great, but prefixing with B_ wont add any value but could make someone think it's specific to BeOS/Haiku.

If the headers are to be public and the defined interface doesn't belong to some standard (e.g. POSIX) there is really nothing to discuss. They have to be prefixed "B_". Which doesn't have anything to do with whether Be, Inc. influences standards, just with what namespace they chose for BeOS's public API. The simple reason for using this prefix is to avoid clashes with third party software.

comment:12 Changed 9 years ago by korli

Blocking: 6114 added

comment:13 Changed 9 years ago by idefix

I spotted two spelling mistakes:

In USB_hid_page_led.h:

B_HID_UID_LED_SURROND_ON,

In USB_hid_page_alphanumeric_display.h:

B_HID_UID_AD_CHAR_ATTR_ENAHNCE,

comment:14 Changed 9 years ago by idefix

Some more:

In USB_hid_page_keyboard.h:

B_HID_UID_KB_PERIOF_AND_RCHEVRON,
B_HID_UID_KP_3_AND_PADE_DOWN,

In USB_hid_page_led.h:

B_HID_UID_LED_CAMERA_OF,

In USB_hid_page_telephony.h:

B_HID_UID_TEL_TELPEHONY_KEY_PAD,
B_HID_UID_TEL_READIAL,
B_HID_UID_TEL_RING_EANBLE,
B_HID_UID_TEL_CONFRIMATION_TONE_1,

In USB_hid_page_bar_code_scanner.h:

B_HID_UID_BCS_PREIODICAL = 0xa9,
B_HID_UID_BCS_PREIODICAL_AUTO_DISCRIMINATE_PLUS_2,
B_HID_UID_BCS_PREIODICAL_ONLY_DECODE_WITH_PLUS_2,
B_HID_UID_BCS_PREIODICAL_IGNORE_PLUS_2,
B_HID_UID_BCS_PREIODICAL_AUTO_DISCRIMINATE_PLUS_5,
B_HID_UID_BCS_PREIODICAL_ONLY_DECODE_WITH_PLUS_5,
B_HID_UID_BCS_PREIODICAL_IGNORE_PLUS_5,
B_HID_UID_BCS_TRASNMIT_START_STOP = 0xd3,

In USB_hid_page_power_device.h:

B_HID_UID_POW_DELAY_BEOFRE_STARTUP,
B_HID_UID_POW_AWAITNING_POWER,

In USB_hid_page_pid.h:

B_HID_UID_PID_POOL_ALIGNEMNT,

In: USB_hid_page_battery_system.h:

B_HID_UID_BAT_USENEXT,
B_HID_UID_BAT_NEED_REAPLCEMENT,
B_HID_UID_BAT_AT_RATE_TOME_TO_EMPTY,
B_HID_UID_BAT_MAXERROR,
B_HID_UID_BAT_RUN_TIM_TO_EMPTY,
B_HID_UID_BAT_AVERGAE_TIME_TO_FULL,
B_HID_UID_BAT_IDEVICE_CHEMISTERY,

comment:15 Changed 9 years ago by idefix

In USB_hid_page_sport_controls.h:

B_HID_UID_SPO_LOFT_WADGE,
B_HID_UID_SPO_POWER_WADGE,

In USB_hid_page_consumer.h:

B_HID_UID_CON_FUNCITION_BUTTONS,
B_HID_UID_CON_ROOM_TERMPERATURE,
B_HID_UID_CON_AL_TELELPHONY_DIALER,
B_HID_UID_CON_AC_MAXIMZE,
B_HID_UID_CON_AC_MIRROE_VERTICAL,
B_HID_UID_CON_AC_NUMERED_LIST,

In USB_hid_page_alphanumeric_display.h:

B_HID_UID_AD_UNCODE_CHARACTER,

comment:16 Changed 9 years ago by x-ist

I expected there would be some typos, but not this much ... ;-) BTW, B_HID_UID_BAT_USENEXT in USB_hid_page_battery_system.h is correct with regard to the spec. Same for B_HID_UID_BAT_IDEVICE_CHEMISTERY, which I corrected nevertheless.

comment:17 Changed 9 years ago by idefix

Yeah, I didn't check with the specs but I expected those to be B_HID_UID_BAT_USE_NEXT (just as B_HID_UID_BAT_MAXERROR -> B_HID_UID_BAT_MAX_ERROR) and B_HID_UID_BAT_IDEVICE_CHEMISTRY.

Changed 9 years ago by x-ist

Attachment: USB_HID_USAGES.v2.diff added

Discovered Typos corrected.

Changed 9 years ago by x-ist

Attachment: UsbHidDriver.diff added

USB HID Driver adapted to the new names.

comment:18 Changed 9 years ago by x-ist

Is this one still being reviewed?

comment:19 Changed 8 years ago by mmadia

Owner: changed from mmlr to axeld

re-assigning to axeld. (hoping for review of updated patch).

comment:20 Changed 8 years ago by stippi

I've read the patches (not every page header in detail) and they look fine to me.

comment:21 Changed 8 years ago by stippi

Resolution: fixed
Status: assignedclosed

I have applied the patch in hrev39874. I proof-read the parts that affected the USB HID driver, to confirm no constants had been changed, and made sure everything compiles. Thanks a lot for your work and sorry for the delay!

Note: See TracTickets for help on using tickets.