linuxconsole-dev

Re: Interactive Effects

[Vojtech P. <vo...@su...>]

On Sat, Jan 26, 2002 at 07:12:10PM +0100, Bj|rn Augustsson wrote:

> Actually, having thought about this some more, this is NOT how PID
> works, unless you're in driver-managed mode (which the MS FF2 isn't -
> it uses the "device managed"  memory model. (Maybe it can switch, I
> haven't checked that carefully, but I don't think so.)
> 

No, it cannot be switched. "Device managed" is MSFF, "driver managed" is
i-force basically. That's why both the models were put into the spec.

-- 
Vojtech Pavlik
SuSE Labs

Re: Interactive Effects

[Bj|rn A. <d3a...@dt...>]

Quoting Johann Deneux <jo...@Do...>:
> On Sat, 26 Jan 2002, Rodrigo Damazio wrote:
> > Johann Deneux wrote:
> > > On Sat, 26 Jan 2002, Rodrigo Damazio wrote:
> >    Okay, will do so...also, is itme who has an outdated version of 
> > input.h, or does it really not have ET Ramp, Damper, and Inertia?? And 
> 
> Ramps did not seem to useful to me. Indeed, they can be realised using
> other effects, for example, using a shaped constant effect.

You can't emulate a shaped ramp effect that way though.

> However, I think they should be added in the API. It's up to the driver to
> decide how to implement this effect.

I agree that they should be added to the API. I don't think a driver
should decide to emulate one effect with another, except in extreme
cases. This sort of thing is much better done in userspace. (A library
for example.) 

> About interactive effects: See these effects as functions from something
> to a force.
> Position -> Force : That's a spring
> Velocity -> Force : That's friction (or damping, why does pid define both
> as different effects ???)

No idea. Note how the spec avoids talking about that about halfway thru
section 5.3, on page 12.

> Acceleration -> Force : That should be inertia. It's not implemented in
> I-Force. Actually, Immersion seems to consider this to be the same as
> friction.
> 
> I agree that we should add FF_INERTIA and FF_RAMP, but FF_DAMPER looks
> pretty useless to me. Unless I missed the point ?

Beats me.
 
> > what would FF_RUMBLE be?? Is it I-force specific?? If it's okay, I'll 
> 
> No, this effect is the "force feedback for the poor". It's used in IFeel
> mice for example. It's just a simple vibration. You can find it in
> some console joypads, too. For example, the AVB Pegasus joystick has
> (should have, mine has none) a connector to add a joypad supporting rumble
> effects.

Right.  This is not even defined in the PID spec, which is kind of
weird. 

> > > Anyway, we will have to change the API to take custom effects into
> > > account.
> > >
> >    Yes, I know...have you decided how we'll do that?? I guess that 
> 
> No. The "1 parameter block - n effects" is appealing, but we must make it
> hardware independant. Right now, there is no obstacle to doing it, as
> I-Force and PID seems to share the same view of what to put into parameter
> blocks. Is it a safe bet to assume future specs won't change ?

Actually, having thought about this some more, this is NOT how PID
works, unless you're in driver-managed mode (which the MS FF2 isn't -
it uses the "device managed"  memory model. (Maybe it can switch, I
haven't checked that carefully, but I don't think so.)

Here you get a number (in the FF2 case, 40) "slots" to put effects in,
and they each have room for whatever extra parameter blocks are needed.
(Ie, they're often oversized.) You can _not_ share parameter blocks 
between effects in this model. 

> The other solution would be to hide these parameter blocks and let the
> driver optimize its memory usage. This looks pretty ambitious to me,
> though.

Absolutely. Again, it could be done in a library.

Hmm, Rodrigo, did you have a look at what the MS DirectInput looks like? How
do they do this?

> The last solution is to go for "1 parameter block - 1
> effect" solution. It's safe, easy to implement, but it kills the
> possibility to share parameters between several effects.

Right. 
 
Now for custom effects, I think we should have a separate call to upload
those. Having the variable length array thing you suggested is nice, but
then we can't add more fields to the strut after that if we choose to
later. (Or can we? I seem to remember that the variable length array has
to be the last member of a struct, but I'm not sure...)

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: Interactive Effects

[Johann D. <jo...@Do...>]

On Sat, 26 Jan 2002, Rodrigo Damazio wrote:

> Johann Deneux wrote:
> 
> > On Sat, 26 Jan 2002, Rodrigo Damazio wrote:
> >
> >>    I just realized - the interactive effect struct doesn't allow us 
> >> to assign independent parameters to the X and Y axes...it should 
> >> though, since the PID spec says there are two condition blocks for it 
> >> - one for each axis...is it okay to modify it to include separate 
> >> parameters??
> >>
> >
> > Definitely yes. I-Force also allows that.
> >
>    Okay, will do so...also, is itme who has an outdated version of 
> input.h, or does it really not have ET Ramp, Damper, and Inertia?? And 

Ramps did not seem to useful to me. Indeed, they can be realised using
other effects, for example, using a shaped constant effect.
However, I think they should be added in the API. It's up to the driver to
decide how to implement this effect.

About interactive effects: See these effects as functions from something
to a force.
Position -> Force : That's a spring
Velocity -> Force : That's friction (or damping, why does pid define both
as different effects ???)
Acceleration -> Force : That should be inertia. It's not implemented in
I-Force. Actually, Immersion seems to consider this to be the same as
friction.

I agree that we should add FF_INERTIA and FF_RAMP, but FF_DAMPER looks
pretty useless to me. Unless I missed the point ?

> what would FF_RUMBLE be?? Is it I-force specific?? If it's okay, I'll 

No, this effect is the "force feedback for the poor". It's used in IFeel
mice for example. It's just a simple vibration. You can find it in
some console joypads, too. For example, the AVB Pegasus joystick has
(should have, mine has none) a connector to add a joypad supporting rumble
effects.

> add FF_RAMP, FF_DAMPER, and FF_INERTIA since they're defined in the
> spec(5.1.1)...
> 
> >
> 
> > Anyway, we will have to change the API to take custom effects into
> > account.
> >
>    Yes, I know...have you decided how we'll do that?? I guess that 

No. The "1 parameter block - n effects" is appealing, but we must make it
hardware independant. Right now, there is no obstacle to doing it, as
I-Force and PID seems to share the same view of what to put into parameter
blocks. Is it a safe bet to assume future specs won't change ?

The other solution would be to hide these parameter blocks and let the
driver optimize its memory usage. This looks pretty ambitious to me,
though.

The last solution is to go for "1 parameter block - 1
effect" solution. It's safe, easy to implement, but it kills the
possibility to share parameters between several effects.

> since(in PID specs at least) custom effect data is sent on one(or more) 
> report by itself, saved in a memory position(just like an effect), etc., 
> we could just make an IOCTL to upload the custom data, then we can 
> normally upload an effect which points to that custom data...that would 
> require only minor changes to the existing code...
> 
> Rodrigo
> 
> P.S.: I've started libff too...send me all ideas you have for nice 
> things it could do...I'm using re-usable parameter blocks(something like 
> FF_create_constant(float direction, float magnitude, FF_envelope 
> *envelope, FF_trigger *trigger, FF_time *time, FF_flags flags)  )
> 

So you are going towards a user-visible 1-parameter-n-effects approach ?

-- 
Johann Deneux

Re: Interactive Effects

[Rodrigo D. <cu...@uo...>]

Johann Deneux wrote:

> On Sat, 26 Jan 2002, Rodrigo Damazio wrote:
>
>>    I just realized - the interactive effect struct doesn't allow us=20
>> to assign independent parameters to the X and Y axes...it should=20
>> though, since the PID spec says there are two condition blocks for it=20
>> - one for each axis...is it okay to modify it to include separate=20
>> parameters??
>>
>
> Definitely yes. I-Force also allows that.
>
   Okay, will do so...also, is itme who has an outdated version of=20
input.h, or does it really not have ET Ramp, Damper, and Inertia?? And=20
what would FF_RUMBLE be?? Is it I-force specific?? If it's okay, I'll=20
add FF_RAMP, FF_DAMPER, and FF_INERTIA since they're defined in the=20
spec(5.1.1)...

>
> Anyway, we will have to change the API to take custom effects into
> account.
>
   Yes, I know...have you decided how we'll do that?? I guess that=20
since(in PID specs at least) custom effect data is sent on one(or more)=20
report by itself, saved in a memory position(just like an effect), etc.,=20
we could just make an IOCTL to upload the custom data, then we can=20
normally upload an effect which points to that custom data...that would=20
require only minor changes to the existing code...

Rodrigo

P.S.: I've started libff too...send me all ideas you have for nice=20
things it could do...I'm using re-usable parameter blocks(something like=20
FF_create_constant(float direction, float magnitude, FF_envelope=20
*envelope, FF_trigger *trigger, FF_time *time, FF_flags flags)  )

--=20
*******************************
* Rodrigo Damazio             *
* *************************** *
* cu...@uo...           *
* rod...@po... *
* ICQ: #3560450               *
* http://www.vros.com/cuddly/ *
* *************************** *
* Engenharia da Computa=E7=E3o    *
* Escola Polit=E9cnica          *
* USP - S=E3o Paulo             *
*******************************

Re: Interactive Effects

[Johann D. <jo...@Do...>]

On Sat, 26 Jan 2002, Rodrigo Damazio wrote:

>     I just realized - the interactive effect struct doesn't allow us to 
> assign independent parameters to the X and Y axes...it should though, 
> since the PID spec says there are two condition blocks for it - one for 
> each axis...is it okay to modify it to include separate parameters??

Definitely yes. I-Force also allows that.
Anyway, we will have to change the API to take custom effects into
account.

-- 
Johann Deneux

Interactive Effects

[Rodrigo D. <cu...@uo...>]

    I just realized - the interactive effect struct doesn't allow us to=20
assign independent parameters to the X and Y axes...it should though,=20
since the PID spec says there are two condition blocks for it - one for=20
each axis...is it okay to modify it to include separate parameters??

Rodrigo

--=20
*******************************
* Rodrigo Damazio             *
* *************************** *
* cu...@uo...           *
* rod...@po... *
* ICQ: #3560450               *
* http://www.vros.com/cuddly/ *
* *************************** *
* Engenharia da Computa=E7=E3o    *
* Escola Polit=E9cnica          *
* USP - S=E3o Paulo             *
*******************************

Re: PID code

[Rodrigo D. <cu...@uo...>]

Bj|rn Augustsson wrote:

>Quoting Rodrigo Damazio <cu...@uo...>:
>
>>    Hey man...how's the PID code going...boring days are going by with=20
>>very little coding =3Dc) Send me what you have just so I can take a loo=
k=20
>>at it, even if it doesn't work yet...
>>
>
>Yeah, I've attached it.
>
>This is against 2.4.17 + Johann's input-only patch from the list.
>
>There are some problems with this, but some stuff work too.
>
>Starting and stopping effects work, for example.
>
>Uploading doesn't, so the only effect to play with is the default
>spring effect (Effect Block Index 1)
>
    What's missing for basic upload to work?? Only block management??

>
>Setting the gain should work, but oopses the machinne here right now. I
>don't think that's my fault it doesn't happen in my code and it started
>when I upgraded the kernel.
>
>Next step for me: Syncing with the current console tree/2.5.2.
>
>Have fun,
>
>/August.
>
        Okay, I'm working on it...first thing I did was, as you=20
suggested in one of your comments, to move pid-specific stuff to=20
pid.c...I'll see what I can implement for the upload functions...

Rodrigo
--=20

*******************************
* Rodrigo Damazio             *
* *************************** *
* cu...@uo...           *
* rod...@po... *
* ICQ: #3560450               *
* http://www.vros.com/cuddly/ *
* *************************** *
* Engenharia da Computa=E7=E3o    *
* Escola Polit=E9cnica          *
* USP - S=E3o Paulo             *
*******************************

Re: HID issues.

[Vojtech P. <vo...@su...>]

On Fri, Jan 25, 2002 at 02:03:34AM +0100, Bj|rn Augustsson wrote:
> Quoting Vojtech Pavlik <vo...@su...>:
> > On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> > > 
> > > Hello, folks!
> > > 
> > > Looking thru the code, I found a bunch of things that I think someone
> > > should have a look at.
> > > 
> > > * There's a race in the HID init code. What happens is
> > > 
> > > send for all input reports;
> > > send for all feature reports;
> > > block until all feature reports are in;
> > > configure device based on input & feature report data. (hid_init_reports)
> > > 
> > > Some input reports might not have arrived yet when we call
> > > hid_init_reports.
> > > 
> > > (And yes, this actually happens in real life.)
> > 
> > Ugh? I don't think I understand you here fully. hid_init_reports() is
> > the function that makes all the request to get the input and feature
> > data. So *no* reports come before it is called. 
> 
> Quite. My mistake. 
> 
> > > Some input reports might not have arrived yet when we call
> > > hid_init_reports.
> 
> Should have been
> 
> Some input reports might not have arrived yet when 
> hid_init_reports returns.
> 
> This means that we call hidinput_connect() before we have all the 
> data.
> 
> Sorry for the confusion.

You mean when we time out? Well, yes, then we have a problem. But the
timeout is the error case which shouldn't happen. Otherwise, I don't see
how any pending 'get report' requests could stay in the queue - we wait
for both the control and out queue to clear completely. 

> > And it waits for all the
> > report to arrive then - it waits till the queue is empty. Maybe we are
> > each reading a different version? I'm looking at the current CVS.
> 
> Well, I'm not, but fairly close (CVS version tag 1.35).

Maybe update, there was a serious rewrite of that part.

> The way I read the code, it waits for all feature reports to come in.
> (On the control pipe.)
> All the input reports aren't neccessarily in yet though. (On the input
> interrupt pipe.)
> 
> Maybe there's some interaction here that I don't see/know about, but
> that's what it looks like to me.

Both input and feature reports come over the control pipe. The input
interrupt pipe isn't open at that time yet, and thus no reports are
coming over it.

> > > * When you do hid_set_field, there's code that checks the value against
> > > logical max/min for that field. That's useful information, but you're
> > > supposed to be able to set a value outside of that interval - a
> > > Null Value, it's called in the HID spec (5.10 in the spec). 
> > > We should probably warn if the value is out of the interval, but only 
> > > error if it won't actually fit in the field. 
> > > Ie: settting an 8-bit field with logical min = 1 and logical 
> > > max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> > > it should be allowed.
> > 
> > Correct, feel free to implement a better fix, I'll just remove the check
> > for now.
> 
> Right. This is my next sub-project then. 
> 
> > > * Why hasn't the hid_report struct got a usage field in it?
> > > This makes it difficult to find a certain report by walking the tree,
> > > and worse, information is lost, at least when we parse arrays:
> > 
> > Because it wasn't needed by anything yet. All decisions are made based
> > on the field usage data for the input-only devices and it is enough. So,
> > if you need it, add it.
> 
> Will do.
> 
> /August.
> -- 
> Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
> being a pointer to const Foo.  const Foo const *foo; would mean the same but is
> illegal (double const).  You are confusing this with Foo * const foo; and const
> Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

-- 
Vojtech Pavlik
SuSE Labs

Re: HID issues.

[Bj|rn A. <d3a...@dt...>]

Quoting Vojtech Pavlik <vo...@su...>:
> On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> > 
> > Hello, folks!
> > 
> > Looking thru the code, I found a bunch of things that I think someone
> > should have a look at.
> > 
> > * There's a race in the HID init code. What happens is
> > 
> > send for all input reports;
> > send for all feature reports;
> > block until all feature reports are in;
> > configure device based on input & feature report data. (hid_init_reports)
> > 
> > Some input reports might not have arrived yet when we call
> > hid_init_reports.
> > 
> > (And yes, this actually happens in real life.)
> 
> Ugh? I don't think I understand you here fully. hid_init_reports() is
> the function that makes all the request to get the input and feature
> data. So *no* reports come before it is called. 

Quite. My mistake. 

> > Some input reports might not have arrived yet when we call
> > hid_init_reports.

Should have been

Some input reports might not have arrived yet when 
hid_init_reports returns.

This means that we call hidinput_connect() before we have all the 
data.

Sorry for the confusion.

> And it waits for all the
> report to arrive then - it waits till the queue is empty. Maybe we are
> each reading a different version? I'm looking at the current CVS.

Well, I'm not, but fairly close (CVS version tag 1.35).

The way I read the code, it waits for all feature reports to come in.
(On the control pipe.)
All the input reports aren't neccessarily in yet though. (On the input
interrupt pipe.)

Maybe there's some interaction here that I don't see/know about, but
that's what it looks like to me.

> > * When you do hid_set_field, there's code that checks the value against
> > logical max/min for that field. That's useful information, but you're
> > supposed to be able to set a value outside of that interval - a
> > Null Value, it's called in the HID spec (5.10 in the spec). 
> > We should probably warn if the value is out of the interval, but only 
> > error if it won't actually fit in the field. 
> > Ie: settting an 8-bit field with logical min = 1 and logical 
> > max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> > it should be allowed.
> 
> Correct, feel free to implement a better fix, I'll just remove the check
> for now.

Right. This is my next sub-project then. 

> > * Why hasn't the hid_report struct got a usage field in it?
> > This makes it difficult to find a certain report by walking the tree,
> > and worse, information is lost, at least when we parse arrays:
> 
> Because it wasn't needed by anything yet. All decisions are made based
> on the field usage data for the input-only devices and it is enough. So,
> if you need it, add it.

Will do.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: HID issues.

[Vojtech P. <vo...@su...>]

On Thu, Jan 24, 2002 at 10:33:02PM +0100, Bj|rn Augustsson wrote:
> 
> Hello, folks!
> 
> Looking thru the code, I found a bunch of things that I think someone
> should have a look at.
> 
> * There's a race in the HID init code. What happens is
> 
> send for all input reports;
> send for all feature reports;
> block until all feature reports are in;
> configure device based on input & feature report data. (hid_init_reports)
> 
> Some input reports might not have arrived yet when we call
> hid_init_reports.
> 
> (And yes, this actually happens in real life.)

Ugh? I don't think I understand you here fully. hid_init_reports() is
the function that makes all the request to get the input and feature
data. So *no* reports come before it is called. And it waits for all the
report to arrive then - it waits till the queue is empty. Maybe we are
each reading a different version? I'm looking at the current CVS.

> * When you do hid_set_field, there's code that checks the value against
> logical max/min for that field. That's useful information, but you're
> supposed to be able to set a value outside of that interval - a
> Null Value, it's called in the HID spec (5.10 in the spec). 
> We should probably warn if the value is out of the interval, but only 
> error if it won't actually fit in the field. 
> Ie: settting an 8-bit field with logical min = 1 and logical 
> max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
> it should be allowed.

Correct, feel free to implement a better fix, I'll just remove the check
for now.

> * Why hasn't the hid_report struct got a usage field in it?
> This makes it difficult to find a certain report by walking the tree,
> and worse, information is lost, at least when we parse arrays:

Because it wasn't needed by anything yet. All decisions are made based
on the field usage data for the input-only devices and it is enough. So,
if you need it, add it.

> This is a "PID Block Load Report", as reported by lsusb:
> (Yeah, I know, but this was the shortest one that shows this...)
> 
> Note how the "Block Load {Success,Full,Error}" fields
> have three layers of usages:
> * The report usage (PID Block Load Report)
> * The logical collection (Block Load Status)
> * The field's actual usage.
> 
>     Item(Global): Usage Page, data= [ 0x0f ] 15
>                     PID Page
>     Item(Local ): Usage, data= [ 0x89 ] 137
>                     PID Block Load Report
>     Item(Main  ): Collection, data= [ 0x02 ] 2
>                     Logical
>         Item(Global): Report ID, data= [ 0x02 ] 2 (feature)
>         Item(Local ): Usage, data= [ 0x22 ] 34
>                         Effect Block Index
>         Item(Global): Logical Maximum, data= [ 0x28 ] 40
>         Item(Global): Logical Minimum, data= [ 0x01 ] 1
>         Item(Global): Physical Minimum, data= [ 0x01 ] 1
>         Item(Global): Physical Maximum, data= [ 0x28 ] 40
>         Item(Global): Report Size, data= [ 0x08 ] 8
>         Item(Global): Report Count, data= [ 0x01 ] 1
>         Item(Main  ): Feature, data= [ 0x02 ] 2
>                     Data Variable Absolute No_Wrap Linear
>                     Preferred_State No_Null_Position Non_Volatile Bitfield
>         Item(Local ): Usage, data= [ 0x8b ] 139
>                     Block Load Status
>         Item(Main  ): Collection, data= [ 0x02 ] 2
>                         Logical
>             Item(Local ): Usage, data= [ 0x8c ] 140
>                             Block Load Success
>             Item(Local ): Usage, data= [ 0x8d ] 141
>                             Block Load Full
>             Item(Local ): Usage, data= [ 0x8e ] 142
>                             Block Load Error
>             Item(Global): Logical Maximum, data= [ 0x03 ] 3
>             Item(Global): Logical Minimum, data= [ 0x01 ] 1
>             Item(Global): Physical Minimum, data= [ 0x01 ] 1
>             Item(Global): Physical Maximum, data= [ 0x03 ] 3
>             Item(Global): Report Size, data= [ 0x08 ] 8
>             Item(Global): Report Count, data= [ 0x01 ] 1
>             Item(Main  ): Feature, data= [ 0x00 ] 0
>                             Data Array Absolute No_Wrap Linear
>                             Preferred_State No_Null_Position Non_Volatile Bitfield
>         Item(Main  ): End Collection, data=none
>         Item(Local ): Usage, data= [ 0xac ] 172
>                         RAM Pool Available
>         Item(Global): Logical Minimum, data= [ 0x00 ] 0
>         Item(Global): Logical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
>         Item(Global): Physical Minimum, data= [ 0x00 ] 0
>         Item(Global): Physical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
>         Item(Global): Report Size, data= [ 0x10 ] 16
>         Item(Global): Report Count, data= [ 0x01 ] 1
>         Item(Main  ): Feature, data= [ 0x00 ] 0
>                         Data Array Absolute No_Wrap Linear
>                         Preferred_State No_Null_Position Non_Volatile Bitfield
>     Item(Main  ): End Collection, data=none
> 
> This is how the kernel parses it:
> 
> localhost kernel:   FEATURE(2)[FEATURE]
> localhost kernel:     Field(0)
> localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
> localhost kernel:       Usage(1)
> localhost kernel:         PhysicalInterfaceDevice.Effect_Block_Index
> localhost kernel:       Logical Minimum(1)
> localhost kernel:       Logical Maximum(40)
> localhost kernel:       Physical Minimum(1)
> localhost kernel:       Physical Maximum(40)
> localhost kernel:       Report Size(8)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(0)
> localhost kernel:       Flags( Variable Absolute )
> localhost kernel:     Field(1)
> localhost kernel:       Logical(PhysicalInterfaceDevice.Block_Load_Status)
> localhost kernel:       Usage(3)
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Success
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Full
> localhost kernel:         PhysicalInterfaceDevice.Block_Load_Error
> localhost kernel:       Logical Minimum(1)
> localhost kernel:       Logical Maximum(3)
> localhost kernel:       Physical Minimum(1)
> localhost kernel:       Physical Maximum(3)
> localhost kernel:       Report Size(8)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(8)
> localhost kernel:       Flags( Array Absolute )
> localhost kernel:     Field(2)
> localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
> localhost kernel:       Usage(1)
> localhost kernel:         PhysicalInterfaceDevice.RAM_Pool_Available
> localhost kernel:       Logical Minimum(0)
> localhost kernel:       Logical Maximum(65535)
> localhost kernel:       Physical Minimum(0)
> localhost kernel:       Physical Maximum(65535)
> localhost kernel:       Report Size(16)
> localhost kernel:       Report Count(1)
> localhost kernel:       Report Offset(16)
> localhost kernel:       Flags( Array Absolute )
> 
> Presently, my strategy for finding a specific report is to walk the
> tree, looking for a field with a Logical Usage that's the same as the
> report Usage I'm looking for, and then return the report pointer from
> that field.
> 
> This happens to work for my device, since there's always some field in
> every report that isn't an array, but that's not neccesarily true for
> all devices.
> 
> In other words: If we had a report with all array fields, we wouldn't
> know which usage that report has/represents.

-- 
Vojtech Pavlik
SuSE Labs

HID issues.

[Bj|rn A. <d3a...@dt...>]

Hello, folks!

Looking thru the code, I found a bunch of things that I think someone
should have a look at.

* There's a race in the HID init code. What happens is

send for all input reports;
send for all feature reports;
block until all feature reports are in;
configure device based on input & feature report data. (hid_init_reports)

Some input reports might not have arrived yet when we call
hid_init_reports.

(And yes, this actually happens in real life.)


* When you do hid_set_field, there's code that checks the value against
logical max/min for that field. That's useful information, but you're
supposed to be able to set a value outside of that interval - a
Null Value, it's called in the HID spec (5.10 in the spec). 
We should probably warn if the value is out of the interval, but only 
error if it won't actually fit in the field. 
Ie: settting an 8-bit field with logical min = 1 and logical 
max = 40 to 0xFF should probably warn (probably only if DEBUG), but 
it should be allowed.


* Why hasn't the hid_report struct got a usage field in it?
This makes it difficult to find a certain report by walking the tree,
and worse, information is lost, at least when we parse arrays:

This is a "PID Block Load Report", as reported by lsusb:
(Yeah, I know, but this was the shortest one that shows this...)

Note how the "Block Load {Success,Full,Error}" fields
have three layers of usages:
* The report usage (PID Block Load Report)
* The logical collection (Block Load Status)
* The field's actual usage.

    Item(Global): Usage Page, data= [ 0x0f ] 15
                    PID Page
    Item(Local ): Usage, data= [ 0x89 ] 137
                    PID Block Load Report
    Item(Main  ): Collection, data= [ 0x02 ] 2
                    Logical
        Item(Global): Report ID, data= [ 0x02 ] 2 (feature)
        Item(Local ): Usage, data= [ 0x22 ] 34
                        Effect Block Index
        Item(Global): Logical Maximum, data= [ 0x28 ] 40
        Item(Global): Logical Minimum, data= [ 0x01 ] 1
        Item(Global): Physical Minimum, data= [ 0x01 ] 1
        Item(Global): Physical Maximum, data= [ 0x28 ] 40
        Item(Global): Report Size, data= [ 0x08 ] 8
        Item(Global): Report Count, data= [ 0x01 ] 1
        Item(Main  ): Feature, data= [ 0x02 ] 2
                    Data Variable Absolute No_Wrap Linear
                    Preferred_State No_Null_Position Non_Volatile Bitfield
        Item(Local ): Usage, data= [ 0x8b ] 139
                    Block Load Status
        Item(Main  ): Collection, data= [ 0x02 ] 2
                        Logical
            Item(Local ): Usage, data= [ 0x8c ] 140
                            Block Load Success
            Item(Local ): Usage, data= [ 0x8d ] 141
                            Block Load Full
            Item(Local ): Usage, data= [ 0x8e ] 142
                            Block Load Error
            Item(Global): Logical Maximum, data= [ 0x03 ] 3
            Item(Global): Logical Minimum, data= [ 0x01 ] 1
            Item(Global): Physical Minimum, data= [ 0x01 ] 1
            Item(Global): Physical Maximum, data= [ 0x03 ] 3
            Item(Global): Report Size, data= [ 0x08 ] 8
            Item(Global): Report Count, data= [ 0x01 ] 1
            Item(Main  ): Feature, data= [ 0x00 ] 0
                            Data Array Absolute No_Wrap Linear
                            Preferred_State No_Null_Position Non_Volatile Bitfield
        Item(Main  ): End Collection, data=none
        Item(Local ): Usage, data= [ 0xac ] 172
                        RAM Pool Available
        Item(Global): Logical Minimum, data= [ 0x00 ] 0
        Item(Global): Logical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
        Item(Global): Physical Minimum, data= [ 0x00 ] 0
        Item(Global): Physical Maximum, data= [ 0xff 0xff 0x00 0x00 ] 65535
        Item(Global): Report Size, data= [ 0x10 ] 16
        Item(Global): Report Count, data= [ 0x01 ] 1
        Item(Main  ): Feature, data= [ 0x00 ] 0
                        Data Array Absolute No_Wrap Linear
                        Preferred_State No_Null_Position Non_Volatile Bitfield
    Item(Main  ): End Collection, data=none

This is how the kernel parses it:

localhost kernel:   FEATURE(2)[FEATURE]
localhost kernel:     Field(0)
localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
localhost kernel:       Usage(1)
localhost kernel:         PhysicalInterfaceDevice.Effect_Block_Index
localhost kernel:       Logical Minimum(1)
localhost kernel:       Logical Maximum(40)
localhost kernel:       Physical Minimum(1)
localhost kernel:       Physical Maximum(40)
localhost kernel:       Report Size(8)
localhost kernel:       Report Count(1)
localhost kernel:       Report Offset(0)
localhost kernel:       Flags( Variable Absolute )
localhost kernel:     Field(1)
localhost kernel:       Logical(PhysicalInterfaceDevice.Block_Load_Status)
localhost kernel:       Usage(3)
localhost kernel:         PhysicalInterfaceDevice.Block_Load_Success
localhost kernel:         PhysicalInterfaceDevice.Block_Load_Full
localhost kernel:         PhysicalInterfaceDevice.Block_Load_Error
localhost kernel:       Logical Minimum(1)
localhost kernel:       Logical Maximum(3)
localhost kernel:       Physical Minimum(1)
localhost kernel:       Physical Maximum(3)
localhost kernel:       Report Size(8)
localhost kernel:       Report Count(1)
localhost kernel:       Report Offset(8)
localhost kernel:       Flags( Array Absolute )
localhost kernel:     Field(2)
localhost kernel:       Logical(PhysicalInterfaceDevice.PID_Block_Load_Report)
localhost kernel:       Usage(1)
localhost kernel:         PhysicalInterfaceDevice.RAM_Pool_Available
localhost kernel:       Logical Minimum(0)
localhost kernel:       Logical Maximum(65535)
localhost kernel:       Physical Minimum(0)
localhost kernel:       Physical Maximum(65535)
localhost kernel:       Report Size(16)
localhost kernel:       Report Count(1)
localhost kernel:       Report Offset(16)
localhost kernel:       Flags( Array Absolute )

Presently, my strategy for finding a specific report is to walk the
tree, looking for a field with a Logical Usage that's the same as the
report Usage I'm looking for, and then return the report pointer from
that field.

This happens to work for my device, since there's always some field in
every report that isn't an array, but that's not neccesarily true for
all devices.

In other words: If we had a report with all array fields, we wouldn't
know which usage that report has/represents.


-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: [PATCH] Output Interrupt pipe in HID

[Vojtech P. <vo...@su...>]

On Tue, Jan 22, 2002 at 10:20:22PM +0100, Vojtech Pavlik wrote:
> On Tue, Jan 22, 2002 at 10:11:03PM +0100, Bj|rn Augustsson wrote:
> > Quoting Bj|rn Augustsson <d3a...@dt...>:
> > > It also adds two functions:
> > > [...]
> > 
> > Oops, the patch causes a compiler warning unless you move the function 
> > find_output_intpipe() up to before it's used. It should also be declared
> > "static int" instead of just "int". 
> > 
> > Sorry,
> > 
> > /August, so do I send a new patch, or do you clean it up?
> 
> I think I'll clean this one up. 

Now in CVS.

-- 
Vojtech Pavlik
SuSE Labs

Re: do_BUG

[James S. <jsi...@tr...>]

> When Kernel Debugging is enabled, the nfs code does not compile. This is
> due to kernel.h defining do_BUG. It's called from dcache.h by calling
> BUG(). I guess BUG is a macro calling do_BUG(). As kernel.h is apparently
> included after dcache.h from the nfs code, the compiler first complains
> about do_BUG being implicitely declared to return an int, and then
> complains again when it's defined in dcache.h to return something else.
> 
> The fix would be simple: change the nfs code. However, I guess it is not a
> very good idea to include the nfs code in the linuxconsole tree. Any other
> solution ?

Bad idea. I'm testing it now. Will commit fixes soon.

do_BUG

[Johann D. <jo...@Do...>]

Hi,

When Kernel Debugging is enabled, the nfs code does not compile. This is
due to kernel.h defining do_BUG. It's called from dcache.h by calling
BUG(). I guess BUG is a macro calling do_BUG(). As kernel.h is apparently
included after dcache.h from the nfs code, the compiler first complains
about do_BUG being implicitely declared to return an int, and then
complains again when it's defined in dcache.h to return something else.

The fix would be simple: change the nfs code. However, I guess it is not a
very good idea to include the nfs code in the linuxconsole tree. Any other
solution ?

-- 
Johann Deneux

Re: Call mk_dev in vt.c

[James S. <jsi...@tr...>]

> I get an error when trying to compile drivers/char/vt.c. The compiler
> complains on line 1426 about mk_dev not being declared. It therefore
> assumes that it returns an int, which is not coherent with kdev_t.
> 
> By the way, where does this mk_dev function come from ? I can't seem to
> find it in 2.4.17 or 2.5.2

Oops. Typo. It is mk_kdev. Fixing and commiting now. 

Call mk_dev in vt.c

[Johann D. <jo...@Do...>]

Hi,

I get an error when trying to compile drivers/char/vt.c. The compiler
complains on line 1426 about mk_dev not being declared. It therefore
assumes that it returns an int, which is not coherent with kdev_t.

By the way, where does this mk_dev function come from ? I can't seem to
find it in 2.4.17 or 2.5.2

-- 
Johann Deneux

Re: [PATCH] Output Interrupt pipe in HID

[Vojtech P. <vo...@su...>]

On Tue, Jan 22, 2002 at 10:11:03PM +0100, Bj|rn Augustsson wrote:
> Quoting Bj|rn Augustsson <d3a...@dt...>:
> > It also adds two functions:
> > [...]
> 
> Oops, the patch causes a compiler warning unless you move the function 
> find_output_intpipe() up to before it's used. It should also be declared
> "static int" instead of just "int". 
> 
> Sorry,
> 
> /August, so do I send a new patch, or do you clean it up?

I think I'll clean this one up. 

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] Output Interrupt pipe in HID

[Bj|rn A. <d3a...@dt...>]

Quoting Bj|rn Augustsson <d3a...@dt...>:
> It also adds two functions:
> [...]

Oops, the patch causes a compiler warning unless you move the function 
find_output_intpipe() up to before it's used. It should also be declared
"static int" instead of just "int". 

Sorry,

/August, so do I send a new patch, or do you clean it up?
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

[PATCH] Output Interrupt pipe in HID

[Bj|rn A. <d3a...@dt...>]

The current HID code sends all output reports over the control pipe to
the device. The HID spec states that if an output interrupt pipe is
available, it should be used. (The PID spec also mentions this; optput
interrupt pipes are optional for HID devices, but mandatory for PID.)

Here's a small(ish) patch that fixes that. It also fixes one memory
leak, adds some comments, and unlinks all the urbs in the 
hid_disconnect funtion, instead of just the input one. 

It also changes some things like

-       if (!dir)
+       if (dir == USB_DIR_OUT) {

which is IMHO better. (Who knows which direction is "true"?)

It also adds two functions:

int 
hid_find_field_in_report(struct hid_report *report, __u32
                         wanted_usage, struct hid_field **field)

int 
hid_find_report_by_usage(struct hid_device *hid, __u32 wanted_usage,
                         struct hid_report **report, int type)

These aren't used right now, but I have a PID patch almost ready for
submission that does use them. Later tonight or tomorrow, probably.

Like the last patch, this one is against 2.4.17+Johann's input-only
patch. But it should apply against the 2.5 tree.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: [PATCH] Debug stuff

[Vojtech P. <vo...@su...>]

On Tue, Jan 22, 2002 at 02:18:40PM +0100, Bj|rn Augustsson wrote:
> Quoting Vojtech Pavlik <vo...@su...>:
> > On Tue, Jan 22, 2002 at 04:58:04AM +0100, Bj|rn Augustsson wrote:
> > 
> > > Hi!
> > > 
> > > Here's a new version of the debug printing patch. I've indented it to
> > > match the existing code betteri, mostly. The patch also adds a little
> > > details to error messages, and fixes one that was plain wrong.
> > > 
> > > It's against Johann's input-only patch to 2.4.17, but it should patch
> > > cleanly against the 2.5 tree, I think.
> > 
> > Thanks for the patch, with minor offsets it patched cleanly into a
> > Dave Jones's 2.5.2-ac4-pre kernel. One minor bug in your patch - you
> > made hid-core use dump_field, and didn't define it in case DEBUG wasn't
> > set.
> 
> Ah, so I did.
> 
> Hmm. One fix is to add dump_field to this section in hid.h:
> 
> #ifdef DEBUG
> #include "hid-debug.h"
> #else
> #define hid_dump_input(a,b)     do { } while (0)
> #define hid_dump_device(c)      do { } while (0)
> #endif

Yes, that's what I did.

> But better (IHMO) is for hid.h to include hid_debug.h unconditionally
> and have it define these functions as "do { } while (0)" or the real
> thing depending on DEBUG. That moves some cruft from hid.h to the 
> debug file.
> 
> Any problems with that? 

I'm not sure which is less ugly - #defining the name of an existing
function to override it or having the #ifdef in hid.h which noone else
than the hid driver is supposed to include anyway.

-- 
Vojtech Pavlik
SuSE Labs

Re: [PATCH] Debug stuff

[Bj|rn A. <d3a...@dt...>]

Quoting Vojtech Pavlik <vo...@su...>:
> On Tue, Jan 22, 2002 at 04:58:04AM +0100, Bj|rn Augustsson wrote:
> 
> > Hi!
> > 
> > Here's a new version of the debug printing patch. I've indented it to
> > match the existing code betteri, mostly. The patch also adds a little
> > details to error messages, and fixes one that was plain wrong.
> > 
> > It's against Johann's input-only patch to 2.4.17, but it should patch
> > cleanly against the 2.5 tree, I think.
> 
> Thanks for the patch, with minor offsets it patched cleanly into a
> Dave Jones's 2.5.2-ac4-pre kernel. One minor bug in your patch - you
> made hid-core use dump_field, and didn't define it in case DEBUG wasn't
> set.

Ah, so I did.

Hmm. One fix is to add dump_field to this section in hid.h:

#ifdef DEBUG
#include "hid-debug.h"
#else
#define hid_dump_input(a,b)     do { } while (0)
#define hid_dump_device(c)      do { } while (0)
#endif

But better (IHMO) is for hid.h to include hid_debug.h unconditionally
and have it define these functions as "do { } while (0)" or the real
thing depending on DEBUG. That moves some cruft from hid.h to the 
debug file.

Any problems with that? 

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: Custom-shaped periodic effects in force feedback

[Johann D. <jo...@Do...>]

On Mon, 21 Jan 2002, Bj|rn Augustsson wrote:

> Quoting Johann Deneux <jo...@Do...>:
> > On Mon, 21 Jan 2002, Rodrigo Damazio wrote:
> > 
> > >    Then we'd have to write a little shapeid management(easy)...and 
> > > probably we'd delete a shape from kernel memory after it's used in an 
> > > effect(just so we don't need another ioctl for that), assigning -1 back 
> > > to shapeid in ff_effect(and to avoid bad programming, just make it 
> > > ignore any FF_CUSTOM where a shapeid is -1)...or even keep it and create 
> > > a EVIOCRMCUSTOM to remove it when desired...
> > > 
> > > Let me know what you think...
> > 
> > That's quite close to what I had in mind, except you apply it only for
> > custom effects. Deciding to keep or not the union looks like a rather
> > unimportant decision to me. I suggested to remove the union and use
> > parameter blocks to keep some kind of coherency in the API.
> 
> I agree, that's more like what the real thing (at least wrt PID) looks
> like anyway.

Yes, I know. But the reason I did not choose this scheme earlier was that
it seemed too close to the hardware.

> 
> > More important is the question about whether we should have parameter
> > blocks in the API at all.
> > 
> > The parameter block ioctl approach rises the following issues:
> > - One parameter block may be used by several effects. One may see that as
> > a feature, as it might allow for some memory to be saved. I am not
> > convinced it would be very useful, though.
> 
> I think it could, especially for envelope* blocks , which I imagine you
> could recycle quite a lot. But really, this is an application thing to
> decide, the kernel shouldn't involve itself in these kind of details.

Well, the kernel is involved anyway. Either it allows 1 parameter block to
n effects, or not. The first model seems more attracting, but it has the
issue described below.

> 
> (iforce refers to these as shape. Envelope is from the PID spec, so I'm
> going to use that naming.)
> 
> > - What should be put into parameters, and what should be put into the core
> > of the effect ? For example, the direction of a constant effect is encoded
> > in the core of the effect in the I-Force protocol, while the amplitude is
> > stored in a parameter block. It seems PID does the same. What if another
> > protocol chooses to put the direction into parameter blocks ?
> 
> That gets real ugly either way we do it. It makes sense to do it in the
> most logical way though, which IHMO is the iforce/PID way; all effects
> have a direction, it makes sense to have it in the main block.
> 
> (And at least in PID, the amplitude thing is a bit weird. the Set Effect
> report has "Gain", and for example the Constant Force effect block has 
> "Magnitude". Difference? Magnitude is signed, otherwise I think they're
> just multiplied together (along with the Device Gain.))
> 
> > This can lead to tricky situations. Imagine we put some attribute X into
> > the ff_effect, while a device really stores it in the parameter block. Now
> > the application decides to update the attribute X of an effect. The driver
> > has to change X in the parameter block (because that's what the device
> > wants). Imagine this parameter block is shared by another effect... If we
> > change it here, the attribute X of the second effect is also modified,
> > what is not the desired behaviour.
> 
> Ouch! Let's not do that. *IF* a device that behaves substantially

What do you mean ? We can't help it. For this problem to arise, it is
enough to have these two conditions:
1) We allow 1-to-n, instead or (1,2)-to-1 as it is now.
2) We don't control the placement of attributes

1) would be nice to have, but for 2), we can only hope all future devices
won't change too much.

> differently from iforce and PID shows up, it's probably time to
> introduce some kind of libff or whatever to deal with this stuff.

What do you mean ? having two different interfaces, and hiding their
differences in a libff ? I fear this may only move the problem from the
driver to libff.

-- 
Johann Deneux

Re: [PATCH] Debug stuff

[Vojtech P. <vo...@su...>]

On Tue, Jan 22, 2002 at 04:58:04AM +0100, Bj|rn Augustsson wrote:

> Hi!
> 
> Here's a new version of the debug printing patch. I've indented it to
> match the existing code betteri, mostly. The patch also adds a little
> details to error messages, and fixes one that was plain wrong.
> 
> It's against Johann's input-only patch to 2.4.17, but it should patch
> cleanly against the 2.5 tree, I think.

Thanks for the patch, with minor offsets it patched cleanly into a
Dave Jones's 2.5.2-ac4-pre kernel. One minor bug in your patch - you
made hid-core use dump_field, and didn't define it in case DEBUG wasn't
set.

-- 
Vojtech Pavlik
SuSE Labs

[PATCH] Debug stuff

[Bj|rn A. <d3a...@dt...>]

Hi!

Here's a new version of the debug printing patch. I've indented it to
match the existing code betteri, mostly. The patch also adds a little
details to error messages, and fixes one that was plain wrong.

It's against Johann's input-only patch to 2.4.17, but it should patch
cleanly against the 2.5 tree, I think.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system

Re: Custom-shaped periodic effects in force feedback

[Bj|rn A. <d3a...@dt...>]

Quoting Johann Deneux <jo...@Do...>:
> On Mon, 21 Jan 2002, Rodrigo Damazio wrote:
> 
> >    Then we'd have to write a little shapeid management(easy)...and 
> > probably we'd delete a shape from kernel memory after it's used in an 
> > effect(just so we don't need another ioctl for that), assigning -1 back 
> > to shapeid in ff_effect(and to avoid bad programming, just make it 
> > ignore any FF_CUSTOM where a shapeid is -1)...or even keep it and create 
> > a EVIOCRMCUSTOM to remove it when desired...
> > 
> > Let me know what you think...
> 
> That's quite close to what I had in mind, except you apply it only for
> custom effects. Deciding to keep or not the union looks like a rather
> unimportant decision to me. I suggested to remove the union and use
> parameter blocks to keep some kind of coherency in the API.

I agree, that's more like what the real thing (at least wrt PID) looks
like anyway.

> More important is the question about whether we should have parameter
> blocks in the API at all.
> 
> The parameter block ioctl approach rises the following issues:
> - One parameter block may be used by several effects. One may see that as
> a feature, as it might allow for some memory to be saved. I am not
> convinced it would be very useful, though.

I think it could, especially for envelope* blocks , which I imagine you
could recycle quite a lot. But really, this is an application thing to
decide, the kernel shouldn't involve itself in these kind of details.

(iforce refers to these as shape. Envelope is from the PID spec, so I'm
going to use that naming.)

> - What should be put into parameters, and what should be put into the core
> of the effect ? For example, the direction of a constant effect is encoded
> in the core of the effect in the I-Force protocol, while the amplitude is
> stored in a parameter block. It seems PID does the same. What if another
> protocol chooses to put the direction into parameter blocks ?

That gets real ugly either way we do it. It makes sense to do it in the
most logical way though, which IHMO is the iforce/PID way; all effects
have a direction, it makes sense to have it in the main block.

(And at least in PID, the amplitude thing is a bit weird. the Set Effect
report has "Gain", and for example the Constant Force effect block has 
"Magnitude". Difference? Magnitude is signed, otherwise I think they're
just multiplied together (along with the Device Gain.))

> This can lead to tricky situations. Imagine we put some attribute X into
> the ff_effect, while a device really stores it in the parameter block. Now
> the application decides to update the attribute X of an effect. The driver
> has to change X in the parameter block (because that's what the device
> wants). Imagine this parameter block is shared by another effect... If we
> change it here, the attribute X of the second effect is also modified,
> what is not the desired behaviour.

Ouch! Let's not do that. *IF* a device that behaves substantially
differently from iforce and PID shows up, it's probably time to
introduce some kind of libff or whatever to deal with this stuff.

/August.
-- 
Wrong on most accounts.  const Foo *foo; and Foo const *foo; mean the same: foo
being a pointer to const Foo.  const Foo const *foo; would mean the same but is
illegal (double const).  You are confusing this with Foo * const foo; and const
Foo * const foo; respectively. -David Kastrup, comp.os.linux.development.system