Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/02/2011 10:29 AM, Daniel Nicoletti wrote:
> Thanks for the reply,
>
> 2011/12/2 Jeremy Fitzhardinge <jeremy@goop.org>:
>>> If I understood correctly when talking to Jeremy he said his device
>>> never actually
>>> reported the status as an input event (sorry if I didn't understand it
>>> correctly),
>> No, it does.  When the mouse first connects it reports the battery level
>> as an Input event.  Without my patch, X sees the battery level as a
>> strange little absolute axis on the mouse, which it ignores.
>>
>> I don't know if the mouse also reports the battery later on
>> spontaneously; I haven't observed it, so I suspect you may be right that
>> you have to poll it to get further updates.
> Right, my devices don't send any data if not asked, even if the battery change
> it's status (what did happen because I've put some old batteries.
> So in this case we will need to keep your code on the raw event and probably
> check that into the get_properties part.
>
>>>  and
>>> after reading HID specs I believe it's really because it was meant to be probed,
>>> I have an Apple Keyboard and Magic Trackpad both bluetooth batteries operated,
>>> so using PacketLogger I saw that Mac OSX always ask the battery status using
>>> the so called GetFeature.
>> Could you cite a specific reference?  I don't see any references to
>> "GetFeature" in HID spec I have.  The closest I see is
>> "Get_Report(INPUT/OUTPUT/FEATURE)", which specifically says "This
>> request is not intended to be used for polling the device state on a
>> regular basis".  Though I'm not at all surprised if device manufacturers
>> ignore this, or that it is contradictory with some other part of the spec.
> Sure, HUT1_12v2.pdf section 4.8 Feature Notifications, at the end you
> will see GetReport(Feature) (sorry it was GetReport() not GetFeature()).
> Also when I run my code hcidump shows this, which means it knows what
> a GetReport is: (and Mac OSX PackatLogger.app too)
> HIDP: Get report: Feature report
>
>>> What my patch does is basically:
>>> - store the report id that matches the battery_strength
>>> - setup the battery if 0x6.0x20 is found, even if that is reported as a feature
>>>   (as it was meant to be but only the MagicTrackpad does)
>>> - when upower or someone access /sys/class/power_supply/hid-*/capacity it
>>>   will probe the device and return it's status.
>> How often does usermode read this file?  Does it/can it select on the
>> file to wait for changes.
>>
>> Given that battery level shouldn't change that quickly, I wonder if
>> having a timer poll the device every minute or something of that order
>> might be useful?
> Yes, this is concerning but imo the /sys.../capacity file should return
> the current value, and to do that it need to probe, it the probe timer
> is too often then upower needs fixing, we have done our part into the
> kernel...
>
>>> It works great for both devices, but I have two concerns:
>>> - the report_features function has a duplicated code
>> Yes, but that's easy to fix.
>>
>>> - it would be nice if it was possible for specific drivers to provide their own
>>>   probe as there might be some strange devices... (but maybe it's
>>> already possible)
>> I'd wait until there's a device which can't be handled by this mechanism
>> before trying to generalize it.  Unless we can unify the wacom driver,
>> but that seems to be in its own little world.
> sounds like a better idea.
>
>>> I've talked to the upower dev and he fixed it to be able to show the
>>> right percentage.
>> Cool, but I have some concerns about that (see below).
>>
>>> Here how the uevent file (in /sys/class/power_supply/hid-*/) looks like:
>>> POWER_SUPPLY_NAME=hid-00:22:41:D9:18:E7-battery
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_ONLINE=1
>>> POWER_SUPPLY_CAPACITY=66
>>> POWER_SUPPLY_MODEL_NAME=MacAdmin’s keyboard
>>> POWER_SUPPLY_STATUS=Discharging
>>>
>>> POWER_SUPPLY_NAME=hid-70:CD:60:F5:FF:3F-battery
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_ONLINE=1
>>> POWER_SUPPLY_CAPACITY=62
>>> POWER_SUPPLY_MODEL_NAME=nexx’s Trackpad
>>> POWER_SUPPLY_STATUS=Discharging
>>>
>>> and the patch (note that this is my first kernel patch so sorry if
>>> formatting is not ok, should I attach it?):
>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>> index b9b8c75..8fac47c 100644
>>> --- a/drivers/hid/hid-input.c
>>> +++ b/drivers/hid/hid-input.c
>>> @@ -277,6 +277,7 @@ static enum power_supply_property
>>> hidinput_battery_props[] = {
>>>       POWER_SUPPLY_PROP_ONLINE,
>>>       POWER_SUPPLY_PROP_CAPACITY,
>>>       POWER_SUPPLY_PROP_MODEL_NAME,
>>> +     POWER_SUPPLY_PROP_STATUS
>>>  };
>>>
>>>  static int hidinput_get_battery_property(struct power_supply *psy,
>>> @@ -285,6 +286,9 @@ static int hidinput_get_battery_property(struct
>>> power_supply *psy,
>>>  {
>>>       struct hid_device *dev = container_of(psy, struct hid_device, battery);
>>>       int ret = 0;
>>> +     int ret_rep;
>>> +     __u8 *buf = NULL;
>>> +     unsigned char report_number = dev->battery_report_id;
>>>
>>>       switch (prop) {
>>>       case POWER_SUPPLY_PROP_PRESENT:
>>> @@ -293,28 +297,45 @@ static int hidinput_get_battery_property(struct
>>> power_supply *psy,
>>>               break;
>>>
>>>       case POWER_SUPPLY_PROP_CAPACITY:
>>> -             if (dev->battery_min < dev->battery_max &&
>>> -                 dev->battery_val >= dev->battery_min &&
>>> -                 dev->battery_val <= dev->battery_max)
>>> -                     val->intval = (100 * (dev->battery_val - dev->battery_min)) /
>>> -                             (dev->battery_max - dev->battery_min);
>>> -             else
>>> +             buf = kmalloc(2 * sizeof(__u8), GFP_KERNEL);
>> Why allocate a two byte buffer?  Why not just put it on the stack?  How
>> do you know that's the right size?
> Sorry I'm not kernel expert I just saw some other code working like this..
> I know the size because the first byte is the report_id (in my case 0x47 or 71),
> but the following bytes are reported by Report Size(8) and Report Count(1)
> as the raw function calculates the count by the buf size.
> (note that I might be wrong, but for my devices it works like this).
>
>>> +             if (!buf) {
>>> +                     ret = -ENOMEM;
>>> +                     break;
>>> +             }
>>> +
>>> +             memset(buf, 0, sizeof(buf));
>>> +             ret_rep = dev->hid_get_raw_report(dev, report_number, buf,
>>> sizeof(buf), HID_FEATURE_REPORT);
>>> +             if (ret_rep != 2) {
>>>                       ret = -EINVAL;
>>> +                     break;
>>> +             }
>>> +
>>> +             /* store the returned value */
>>> +             /* I'm not calculating this using the logical_minimum and maximum */
>>> +             /* because my device returns 0-100 even though the min and max are 0-255 */
>>> +             val->intval = buf[1];
>> That's definitely not the case for my mouse.  I'm not sure what the
>> range actually is, but I've seen values reported > 100 with fresh
>> alkaline batteries.  Initially I also thought it was using a 0-100 range
>> despite the min/max, and I added a per-device quirk mechanism - but
>> since my device didn't need it, I did't include it in the patch
>> posting.  I'd reinstate it if there are devices that report 0-100.
> Sure, then we need some quirks... As even the keyboar that reports the max as
> 255 actually returns in 0-100 interval..
>
>>>               break;
>>>
>>>       case POWER_SUPPLY_PROP_MODEL_NAME:
>>>               val->strval = dev->name;
>>>               break;
>>>
>>> +     case POWER_SUPPLY_PROP_STATUS:
>>> +             val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>>> +             break;
>>> +
>>>       default:
>>>               ret = -EINVAL;
>>>               break;
>>>       }
>>>
>>> +     if (buf) {
>>> +             kfree(buf);
>>> +     }
>>>       return ret;
>>>  }
>>>
>>> -static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
>>> +static void hidinput_setup_battery(struct hid_device *dev, unsigned
>>> id, s32 min, s32 max)
>>>  {
>>>       struct power_supply *battery = &dev->battery;
>>>       int ret;
>>> @@ -326,7 +347,7 @@ static void hidinput_setup_battery(struct
>>> hid_device *dev, s32 min, s32 max)
>>>       if (battery->name == NULL)
>>>               return;
>>>
>>> -     battery->type = POWER_SUPPLY_TYPE_BATTERY;
>>> +     battery->type = POWER_SUPPLY_TYPE_USB;
>> What's the purpose of this?  Is it to make upower behave in a
>> different/better way?  My reading is that this means that it's powered
>> by USB ("Standard Downstream Port").  A bluetooth device doesn't have
>> any USB involved at all (except if the BT adapter itself is USB, which
>> is moot).
>>
>> If upower is getting confused thinking that a "battery" is necessarily a
>> laptop battery, then I think it needs to be fixed to pay closer
>> attention to the device tree topology.
>>
>> (looks up upower) Ah, I see.
>>
>> The changes in the current upower.git look very suspect to me, if
>> nothing else because of the hard-coded "magicmouse" and "wacom", and the
>> use of POWER_SUPPLY_TYPE_USB.  Can upower not look to see if there's a
>> power-supply hanging off a HID device and report that as peripheral power?
> I have to talk to Richard (ah you CC him in this email..), but tbh I think the
> change in upower should just look at what properties the battery is
> capable of providing,
> I changed it to USB since IIRC using it as BATTERY made it think it was a power
> supply of the computer..

Yes, but I think that's either a bug in upower or an ambiguity in what
power_supply means (or perhaps, what a power_supply is powering) -
either way, that's something deserving of further discussion.

But either way, the mouse definitely isn't being powered by USB - the
whole point is that it's battery powered.  The kernel shouldn't tell
lies to work around other problems.

>>> -     if (!drv->feature_mapping)
>>> -             return;
>>> -
>>>       rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
>>>       list_for_each_entry(rep, &rep_enum->report_list, list)
>>>               for (i = 0; i < rep->maxfield; i++)
>>> -                     for (j = 0; j < rep->field[i]->maxusage; j++)
>>> -                             drv->feature_mapping(hid, rep->field[i],
>>> -                                                  rep->field[i]->usage + j);
>>> +                     for (j = 0; j < rep->field[i]->maxusage; j++) {
>>> +                             /* Verify if Battery Strength feature is available */
>>> +                             if (((rep->field[i]->usage + j)->hid & HID_USAGE_PAGE) ==
>>> HID_UP_GENDEVCTRLS &&
>>> +                                     ((rep->field[i]->usage + j)->hid & HID_USAGE) == 0x20) {
>>> +                                     hidinput_setup_battery(hid,
>>> +                                                            rep->id,
>>> +                                                            rep->field[i]->logical_minimum,
>>> +                                                            rep->field[i]->logical_maximum);
>>> +                             }
>>> +
>>> +                             if (drv->feature_mapping)
>>> +                                     drv->feature_mapping(hid, rep->field[i],
>>> +                                                          rep->field[i]->usage + j);
>>> +                     }
>> I'd be inclined to split all this out into a separate function, so that
>> you can make it common with the code on the input path.
> Yes, might be better I couldn't think of a nice way to share this part..
> But in the end does my Patch probes your device? :D

Yes, it does!  I changed it to ask for the appropriate report
(INPUT/FEATURE) depending on how it appeared in the original descriptor,
and made a few other cleanups, which I'll post shortly.

    J
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html