Thanks for the reply,
2011/12/2 Jeremy Fitzhardinge <jeremy@xxxxxxxx>:
>> 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..
>
>> - 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
Best,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
