I've thought about this patch some more and I wanted to say up front I think original patch is good to submit. This applies even if you make minor modification to prefer HW over table values. Both ways:
Reviewed-by: Chris Bagwell <[email protected]> But I do have some longer term questions below. On Mon, Jan 24, 2011 at 3:01 PM, Ping Cheng <[email protected]> wrote: > On Mon, Jan 24, 2011 at 7:49 AM, Chris Bagwell <[email protected]> wrote: >> Hi Ping, >> >> I'm trying to catch up with you in understanding... You mentioned >> elsewhere that HW is reporting physical size in terms of 0.001 of a >> cm. > > Not just elsewhere :). It is also in this patch's commit comments (the > second line ;). Somehow I keep missing that. > >> Assuming thats true (what a crazy unit!), then the HID spec shows >> how to compute resolution like this: >> >> resolution in units per cm = logical_size / (physical_size * 10-3) >> >> Would you mind changing your math below to be closer to HID spec? I >> think it will help readers understand faster if they've read HID spec >> first. So I mean change like this: >> >> resolution in units per meter = logical_size / (physical_size * 0.1) > > I got questions for using this type of forms at linux-input ;). I do retract that comment now. I'm preferring your way in fact.. Somehow I need the fact its 0.001 of cm posted in about 5 different locations and then perhaps I'll stop getting caught up in math. > >> It would be cool if HID turns out to be reporting in terms of mm >> (10^-1 or 0.1 of cm). Then it reduces to: >> >> resolution in units per meter = logical_size / physical_size >> >> I have one more comment below. >> >> On Mon, Jan 24, 2011 at 1:39 AM, Ping Cheng <[email protected]> wrote: >>> resolution = logical size / physical size >>> >>> The touch physical size we get from the kernel is in thousandth cm. >>> The resolution unit we need to report to XInput is in point/m. So, >>> >>> touch resolution = logical size * 10 / physical size >>> >>> Signed-off-by: Ping Cheng <[email protected]> >>> --- >>> src/wcmUSB.c | 24 ++++++++++-------------- >>> 1 files changed, 10 insertions(+), 14 deletions(-) >>> >>> diff --git a/src/wcmUSB.c b/src/wcmUSB.c >>> index cfaf920..d8e8312 100644 >>> --- a/src/wcmUSB.c >>> +++ b/src/wcmUSB.c >>> @@ -483,33 +483,29 @@ int usbWcmGetRanges(InputInfoPtr pInfo) >>> common->wcmMaxTouchY = absinfo.maximum; >>> >>> /* max finger strip X for tablets with Expresskeys >>> - * or touch physical X for TabletPCs with touch */ >>> + * or physical X for touch device in thousandth cm */ >>> if (ioctl(pInfo->fd, EVIOCGABS(ABS_RX), &absinfo) == 0) >>> { >>> - if (is_touch) >>> - common->wcmTouchResolX = absinfo.maximum; >>> + if (is_touch && !common->wcmTouchResolX) >>> + common->wcmTouchResolX = >>> + (int)(((double)common->wcmMaxTouchX * 10.0 >>> + / (double)absinfo.maximum) + 0.5); >> >> This gives preference to the lookup table instead of hardware. Maybe >> we should give preference to HW reported value (and fall back to >> table)? > > Good point. I'll think about it a bit more. The issue isn't as simple > as just a HW reported v.s. a table. > > We are dealing with three cases: non-touch devices; touch devices with > resolution reported through RX/RY for kernels older than 2.6.35; and > resolution supported by the new EVIOC. > > For non-touch devices, the resolution is per-model based. Each product > ID has a predefined resolution which we can not retrieve from HID. > That's why we had the table. If we want to get the resolution for > those devices from the kernel, we would need to move the table to the > kernel driver. Thanks for mentioning that. It was a question I had for you on my brain queue but hadn't gotten around to asking if HID supported reporting it for stylus part. I want to eventually submit a kernel patch to add resolutions for the stylus devices as well and sounds like it will have to still be a hard code instead of getting from HID. > > Since touch and pen enabled devices share the same product ID, touch > resolution is default to pen resolution until we get it from the > kernel. > > When we get it from the kernel, we may get it by RX/RY or > input_absinfo depending on which kernel we are going to use. > > My intention for "if (is_touch && !common->wcmTouchResolX)" was to > give preference to input_absinfo over RX/RY. But I neglected the fact > that wcmTouchResolX has already had a default value when > absinfo.resolution is not supported. > > I'll make a version 2. I think it would be easier to merge the two > patches into one since they both deal with the same issue. > >> + if (is_touch && absinfo.maximum) >> >> It doesn't really matter much since this is code that will not be used >> with any new HW. > > Why? It will be for all touch devices that report resolution through > input_absinfo. Or maybe I missed your point? Opps, I didn't write that right. I meant its ok to have either "if (is_touch && !common->wcmTouchResolX)" or "if (is_touch && common->wcmTouchResolX)" in that block because it will not be executed for newer kernels and Touch devices. It is more important to decide on behavior of your 2/2 patch; which preferred HW values over table values. I prefer that behavior. Chris > > Thanks. > > Ping > >> Chris >> >>> else >>> common->wcmMaxStripX = absinfo.maximum; >>> } >>> >>> /* max finger strip Y for tablets with Expresskeys >>> - * or touch physical Y for TabletPCs with touch */ >>> + * or physical Y for touch device in thousandth cm */ >>> if (ioctl(pInfo->fd, EVIOCGABS(ABS_RY), &absinfo) == 0) >>> { >>> - if (is_touch) >>> - common->wcmTouchResolY = absinfo.maximum; >>> + if (is_touch && !common->wcmTouchResolY) >>> + common->wcmTouchResolY = >>> + (int)(((double)common->wcmMaxTouchY * 10.0 >>> + / (double)absinfo.maximum) + 0.5); >>> else >>> common->wcmMaxStripY = absinfo.maximum; >>> } >>> >>> - if (is_touch && common->wcmTouchResolX && common->wcmMaxTouchX) >>> - { >>> - common->wcmTouchResolX = >>> (int)(((double)common->wcmTouchResolX) >>> - / ((double)common->wcmMaxTouchX) + 0.5); >>> - common->wcmTouchResolY = >>> (int)(((double)common->wcmTouchResolY) >>> - / ((double)common->wcmMaxTouchY) + 0.5); >>> - } >>> - >>> /* max z cannot be configured */ >>> if (ioctl(pInfo->fd, EVIOCGABS(ABS_PRESSURE), &absinfo) == 0) >>> common->wcmMaxZ = absinfo.maximum; >>> -- >>> 1.7.3.4 >>> >>> >>> ------------------------------------------------------------------------------ >>> Special Offer-- Download ArcSight Logger for FREE (a $49 USD value)! >>> Finally, a world-class log management solution at an even better price-free! >>> Download using promo code Free_Logger_4_Dev2Dev. Offer expires >>> February 28th, so secure your free ArcSight Logger TODAY! >>> http://p.sf.net/sfu/arcsight-sfd2d >>> _______________________________________________ >>> Linuxwacom-devel mailing list >>> [email protected] >>> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel >>> >> > ------------------------------------------------------------------------------ Special Offer-- Download ArcSight Logger for FREE (a $49 USD value)! Finally, a world-class log management solution at an even better price-free! Download using promo code Free_Logger_4_Dev2Dev. Offer expires February 28th, so secure your free ArcSight Logger TODAY! http://p.sf.net/sfu/arcsight-sfd2d _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
