Message-ID: <20170306090450.GW7064@mail.corp.redhat.com>
Date:   Mon, 6 Mar 2017 10:04:50 +0100
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Tomasz Kramkowski <tk@...-tk.com>
Cc:     Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Denilson Figueiredo de Sá 
        <denilsonsa@...il.com>
Subject: Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

On Mar 04 2017 or thereabouts, Tomasz Kramkowski wrote:
> On Fri, Mar 03, 2017 at 05:15:01PM +0100, Benjamin Tissoires wrote:
> > > Firstly, the value reported by the device is still unusual and does not
> > > correctly represent the state of the device.
> > 
> > That's a little bit worrying. I still think the patch could be taken,
> > but it would be interesting to fix the device. What behavior are you
> > seeing?
> 
> This device is an adapter for ATARI and SEGA GENESIS controllers.
> 
> The genesis controller I am testing with has a basic directional pad, if
> I am interpreting the HID spec correctly, such a control should report
> its state in two fields, one for X one for Y, and the range of values
> should be between -1 and 1. The logical minimum and maximum specified by
> the device's report descriptor does indeed give -1 to 1 as the range,
> however the reports themselves give values of -2 and 1 (when they should
> be -1 and 1). For more information, I recommend perusing the previous
> iteration of this patch which directly addressed this issue:
> 
> https://lkml.org/lkml/2016/12/16/595
> 
> (And as a side note, this bug has been reported by other users of this
> adapter and the GENESIS controller I am testing with is genuine so I do
> not think that the controller itself is at fault here.)
> 
> > > "If an 8-bit field is declared and the range of valid values is 0 to
> > > 0x7F then any value between 0x80 and 0xFF will be considered out of
> > > range and ignored when received. The initialization of null values in a
> > > report is much easier if they are all the same."
> > 
> > I think the whole paragraph is conditioned to the fact that the Null bit
> > is set ("This is accomplished by declaring bit field in a report that is
> > capable of containing a range of values larger than those actually
> > generated by the control.")
> 
> I'm not entirely sure about this, because at this point in the spec the
> "No null position / Null state" bit has not even been mentioned.
> 
> The sentence "This is accomplished by declaring bit field in a report
> that is capable of containing a range of values larger than those
> actually generated by the control." (aside from having a small
> grammatical mistake which might be the source of ambiguity) seems to be
> referring to a "bit field" in the way that the rest of the spec refers
> to "bit fields" as fields comprised of n bits. This is further supported
> by a number of mentions of 8-bit fields and 1-bit fields throughout the
> specification, and also by the existence of the 8th data bit of Input,
> Output and Feature items: {Bit Field (0) | Buffered Bytes (1)} This bit
> distinguishes between a field of n bits and a 8-bit aligned "fixed-size
> stream of bytes".
> 
> So what I think this sentence is saying is: If you have a field of 4
> bits but a logical minimum of -7 and a logical maximum of 7. In this
> example, the range that a 4-bit field could store would be 0-15 or -8 to
> 7, since the logical minimum excludes the -8, this is a field capable of
> producing a null value.
> 
> (This is also mentioned in the thread you mentioned before:
> https://lkml.org/lkml/2011/11/2/470 Denilson comes to the same
> conclusion)
> 
> > > Which suggests that leaving the bit unset means "the control is always
> > > sending meaningful data" as this can simply be interpreted to mean that
> > > the control should never be reporting out-of-range values, not that out
> > > of range values should stop being ignored.
> > 
> > I interpret this as:
> > - No Null Position (0) -> all the data sent by the device is valid and
> >   should be treated as such. Out of range values should then be forwarded
> > - Null State (1) -> the device can send out-of-range values that are
> >   meaningless and should be ignored
> 
> The thing I think we can both agree on is that the "No Null Position |
> Null State" bit seems rather unnecessary if it is interpreted this way.
> If all values should be forwarded, it would be more obvious if the
> logical range was simply extended rather than have this ambiguous bit
> set. This is why I personally think that if the spec is interpreted to
> understand this bit as "descriptive" of how the device should behave, it
> makes more sense to see it as a way of detecting if the device is
> malfunctioning rather than as a way of interpreting the report.
> 
> I think to this end it might make more sense to patch the code to
> produce a warning message on the kernel log if a device with the "No
> Null State" bit set does in fact produce an out of range value, but I'm
> personally not sure if the kernel attempts to make information about
> buggy USB devices known.
> 
> > > However, part of the Universal Serial Bus HID Usage Tables document
> > > appears to contradict this interpretation to some extent.
> > > §A.4 (page 132) states:
> > > 
> > > "The No Null Position flag indicates that there is never a state in
> > > which it is not sending meaningful data. The returned values are Null =
> > > No event (outside of the Logical Minimum / Logical Maximum range) 1 =
> > > Stat Not Ready, 2 = Stat Ready, or 3 = Err Not a loadable character."
> > 
> > IMO, the report descriptor above is buggy:
> >   ReportSize(2), ReportCount(1), 
> >   Logical Maximum(2), 
> >   Usage(Display Status), 
> >   Collection(Logical), 
> >       Usage(Stat Not Ready), 
> >       Usage(Stat    Ready),    
> >       Usage(Err Not a loadable character), 
> >       Input(Data, Array, Absolute, No Null Position),  ; 3-bit status field 
> >   End Collection(), 
> > 
> > The explicit logical minimum is not set, implying that "Stat Not Ready"
> > should be numerical value 0x00, not 0x01.
> 
> I think the fact this descriptor is buggy might be a good indication
> that it is not representative of the usage of the "No Null Position |
> Null State" bit.

Well, the HID spec is not very clear, that the least we can agree on :)

But Microsoft's interpretation is rather clear in the
multitouch/touchpad/pen specification:
https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).aspx
paragraph "Required HID usages for pen digitizers":

"It should be noted that the host will recognize the values outside the
logical range as signifying the implementation of this protocol only if
the report descriptor specifically includes the bit signifying the fact
that X and Y support NULL states. Otherwise, values outside the logical
range are simply moved to the nearest boundary value."

And the 2 report descriptors written after are correct concerning the
NULL state bit.

For Microsoft (in the pointer delivery protocol):
- NULL state -> ignore out of range values
- No NULL State -> clamp at the nearest boundary.

Following this would solve both issues If I understand correctly. Your
controller would be clamped to [-1..1] (No Null State), and the ones
that need to be ignored (like the ones from Denilson will be thanks to
the NULL state bit set.

Maybe we can follow this to say we are mimicking Microsoft's driver and
hope for the best?

> 
> > > Personally I am not entirely sure I am interpreting everything
> > > correctly, but it seems that the "No Null Position/Null State" bit is
> > > not prescriptive on how the data should be handled but rather
> > > descriptive of what data should be expected.
> > 
> > Agree. Though it seems that if the device explains if it will provide
> > Null values or not, we should handle this gracefully.
> 
> Indeed, the question is what does "gracefully" mean, the mentioned
> previous thread which introduced the initial check suggests that there
> are rare cases of devices which make use of out of range values and it
> seems dropping these values seemed like the correct approach. And this
> conclusion was made from the interpretation of the spec with the creator
> of a device on hand to explain how they personally interpreted the spec.
> 
> In the current case, we have an even rarer device and no creator to
> speak to. I think in these cases it is difficult to make a decision on
> how to handle things based on how one device (which by my interpretation
> is buggy even if it does work on Windows) expects things to work.
> 
> The other issue is that the ambiguity of the spec itself makes it
> difficult to decide how most other creators of devices might interpret
> it themselves in the future.
> 
> > I was a bit hesitant to give my Acked-by at the beginning, but
> > re-reading the original kernel thread made up my mind. Also, the fact
> > that most devices that I saw are not using NULL values and that this bit
> > needs to be explicitly set makes me feel comfortable testing this
> > upstream for v4.12.
> 
> I am personally not as confident that the original thread clears
> everything up, but I am entirely happy to take this slowly and carefully
> and discuss all the issues and ambiguities before this patch is
> accepted.
> 
> I think you're right that this patch is not likely to bring about any
> issues, but I think it's more likely that this bit is simply generally
> avoided due to the ambiguity of its specification rather than the
> explicit correctness of our interpretations.
> 
> I think to some extent it might even be worth bringing Denilson
> Figueiredo de Sá for his opinion on my and your interpretations of the
> spec.
> 
> Thank you for your review, I greatly appreciate your time spent going
> over my interpretation.
> 

Cheers,
Benjamin

Powered by blists - more mailing lists