linuxconsole-dev

Force feedback bugs

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

I suggest we continue this discussion on the linuxconsole mailing
list. People on this list may be interested in the discussion.

Oliver and I have been talking about bugs in the force feedback code.

On Sat, 29 Sep 2001, Oliver Hamann wrote:

> OK, here we have two bugs so far:
> 
> BUG 1: machine freeze
> =====================
> 
> No, i did not encounter machine freezes, but I did not test for so many hours.
> I will send you more discussion about this bug on another day.
> 
> BUG 2: force freeze
> ===================
> 
> The device stops carrying out the commands from a sudden moment, the
> current force remains forever. Sometimes this happens right away after
> program start, so that a zero force is played forever. (I think this is
> one bug)
>

This bug I did not encounter with my AVB devices (Pegasus Top Shot and
their latest wheel)
 
> 
> BUG 3: force interrupts
> =======================
> 
> > > Another bug is, that the force often interrupts radomly for a few milliseconds.
> > 
> > I think I managed to track down the cause of this. The point is that I
> > have no idea how to work around this annoying behaviour. I guess I will
> > have to spy at the usb or rs232 traffic under windows.
> 
> Somehow I tend to believe that the bug nests in the serial part, because the
> highler level code looks so clear. My wheel is on rs232. Did you ever
> encounter the bug with usb? But read on, there is a solution to lessen bug 3.

Yes, I do have the same problem with usb. I thought that maybe the cause
was my asynchronous serial code. But even when coming back to a
synchronous model, the problem subsists.

> 
> > For your interest: There are two kinds of packets sent to the device to
> > describe an effect:
> > - some parameter packets. For example, one parameter is the magnitude of
> > the force, another parameter packet is the shape (attack + fade)
> > - an effect packet. It describes the direction of an effect, for how long
> > it should be played, which buttons can trigger it...
> 
> (Yes, i know the iforce protocol. Some years ago I even hacked it out with
> the idea to develop support for Linux. But it was one of the several projects
> that have been sorted out in lack of time... Now you are there ;o)
> 
> > Sending many parameter packets is ok, but sending an effect parameter too
> > often stops (sometimes) the playback. My intent is to prevent effect to
> > stop when they are not supposed to. Another solution would be to restart
> > the effect when it "crashes".
> > 
> > As the API is today, users do not know what will go into the parameter
> > packet, and will go in the effect one. I wonder if I should change that.
> 
> I think your "intent to prevent effect to stop when they are not supposed to"
> is a good idea. This could be done by adding an update function that only
> sends parameter packets. Additionally, to make this work with the magnitude
> packet, it would be nice if you could make it possible to send effects
> without paramter packets which are not essential (especially shape), because:

I am not especially willing to make the distinction between effect packets
and parameter packets visible to the user. Other protocoles may have a
different understanding of what goes into an effect packet, and what goes
into a parameter packet.
What I could do is store the effects uploaded with ioctl in the
driver. When other ioctls are performed to update an effect, I could
analyse the difference and send only the necessary packets.
Or I could change the API. Each call to ioctl would set one property of an
effect:

struct effect_property prop;
prop.type = FF_PROP_ATTACK_LEVEL;
prop.value = ...
ioctl(fd, EVIOCSFF, &prop);

prop.type = FF_PROP_ATTACK_DURATION;
prop.value = ...
ioctl(fd, EVIOCSFF, &prop);
...

Instead of the one big ff_effect structure used now.

> 
> In the hope to find out more about bug 2 and 3, i again removed the sending
> of the shape from iforce.c for a test. Moreover i hacked it all to send only
> a magnitude packet on each update. Unfortunately bug 2 is resistant against
> that. But it lessens bug 3: the force interrupt rate went down to about one
> per 5 seconds (at -u 25). In other words, it fixes 80% of bug 3. :-}
> Hm, on the other side, this possibly could be an indication for that bug 3
> may have something to do with the amount of packet traffic.
> By the way, with this test i have found bug 4 (see more below).

I do not know. If that was the case, we would have the bug occuring during 
high traffic communications, and disappear at lower ones. It just happens
more often at high than low traffics. May guess would be that a few 
packets are malformed, and those are the ones that cause interrupts.


> BUG 3: force interrupts - continued
> ===================================
> 
> > > The interrupt rate is about one per second, with large scatter.
> 
> New information: The force interrupt rate depends on the update rate.
> With -u 100 the interrupts are more frequent than with -u 1.
> 
> > > This bug can be
> > > felt well when starting ffcfstress with -a 0 and holding the wheel tight to the
> > > right or left. The feeling gets stronger when additionally moving the wheel
> > > slightly back and forth for avoidance of the friction. I'm sure that this is the
> > > bug i called "the forces are played scattered somehow" in one of the earlier mails.
> > 
> 
> BUG 4 - level -128
> ==================
> 
> This is an easier one. It's a feature of the iforce protocol, or it's
> a bug in my device (how is it with your device?).
> 
> When sending the signed byte -128 for the magnitude level to the device,
> the force is played as +128. Thus the range is not -128...+127, but
> -127...+128, in which +128 is encoded by 0x80. I tend to prefer that
> we should treat 0x80 as an undefined behavior and that we should reduce
> the range to -127...+127. Indeed, Windows never sends magnitude 0x80.
> 
> I really believe that this bug/feature is even for all other sigened
> byted level parameters. I know this for sure about the shape's attack
> level.

Hmm, I assumed the encoding of the negative values was the usual
complement to two, but I may have been wrong. Perhaps this could explain
the bug 2. While we expect the device to produce a force of value X, the
device understands something completely different. But this occurs only
for some values, most of values "feel" right.

> 
> The fix could be to replace lines like:
>   data[2] = HI(level);
> by for example:
>   data[2] = HI(level<0 ? level+255 : level);
>   /* This solution makes the roundings symmetrical: */
>   /* level +/-255 => data 0, level +/-256 => data +/-1 */
> 
> Another solution could be:
>   data[2] = HI(level<0 ? level+256 : level);
>   /* this one makes levels of -32768 valid */
> 
> When you have fixed it in iforce.c, you can simplify the code in ffcfstress.c
> 
> from:
> 
> 	/* Set force */
> 	if (force>0) {
> 		if (force>1.0) force=1.0;
> 		effect.u.constant.level=(short)(force*32767.0); /* only to be safe */
> 		effect.u.constant.direction=0xC000;
> 		effect.u.constant.shape.attack_level=(short)(force*32767.0); /* this one counts! */
> 		effect.u.constant.shape.fade_level=(short)(force*32767.0); /* only to be safe */
> 	}
> 	else {
> 		if (force<-1.0) force=-1.0;
> 		effect.u.constant.level=(short)(force*-32767.0); /* only to be safe */
> 		effect.u.constant.direction=0x4000;
> 		effect.u.constant.shape.attack_level=(short)(force*-32767.0); /* this one counts! */
> 		effect.u.constant.shape.fade_level=(short)(force*-32767.0); /* only to be safe */
> 	}
> 
> to:
> 
> 	/* Set force */
> 	if (force>1.0) force=1.0;
> 	else if (force<-1.0) force=-1.0;
> 	effect.u.constant.level=(short)(force*32767.0); /* only to be safe */
> 	effect.u.constant.shape.attack_level=(short)(force*32767.0); /* this one counts! */
> 	effect.u.constant.shape.fade_level=(short)(force*32767.0); /* only to be safe */
> 
> Thus, bug 4 was the reason why i needed to keep the level values positive.

Ok, I will add your fix when I come back home.

PS: ffcfstress sources attached.

-- 
Johann Deneux

Re: Force feedback bugs

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

On Sat, Sep 29, 2001 at 02:03:11PM +0200, Johann Deneux wrote:
 
> I am not especially willing to make the distinction between effect packets
> and parameter packets visible to the user. Other protocoles may have a
> different understanding of what goes into an effect packet, and what goes
> into a parameter packet.
> What I could do is store the effects uploaded with ioctl in the
> driver. When other ioctls are performed to update an effect, I could
> analyse the difference and send only the necessary packets.
> Or I could change the API. Each call to ioctl would set one property of an
> effect:
> 
> struct effect_property prop;
> prop.type = FF_PROP_ATTACK_LEVEL;
> prop.value = ...
> ioctl(fd, EVIOCSFF, &prop);
> 
> prop.type = FF_PROP_ATTACK_DURATION;
> prop.value = ...
> ioctl(fd, EVIOCSFF, &prop);
> ...
> 
> Instead of the one big ff_effect structure used now.

I think the differential uploading is nicest.

-- 
Vojtech Pavlik
SuSE Labs

Force feedback API

[Oliver H. <au...@od...>]

Hi!

Lessening bug 3 isn't any longer a reason to discuss about the API (see
my previous mail in the mailing list). But i like it anyway - do you even?

Vojtech Pavlik wrote:
> 
> On Sat, Sep 29, 2001 at 02:03:11PM +0200, Johann Deneux wrote:
> 
> > I am not especially willing to make the distinction between effect
> > packets and parameter packets visible to the user. Other protocoles
> > may have a different understanding of what goes into an effect packet,
> > and what goes into a parameter packet. What I could do is store the
> > effects uploaded with ioctl in the driver. When other ioctls are
> > performed to update an effect, I could analyse the difference and send
> > only the necessary packets. Or I could change the API. Each call to
> > ioctl would set one property of an effect:
> >
> > struct effect_property prop;
> > prop.type = FF_PROP_ATTACK_LEVEL;
> > prop.value = ...
> > ioctl(fd, EVIOCSFF, &prop);
> >
> > prop.type = FF_PROP_ATTACK_DURATION;
> > prop.value = ...
> > ioctl(fd, EVIOCSFF, &prop);
> > ...
> >
> > Instead of the one big ff_effect structure used now.
> 
> I think the differential uploading is nicest.

I think the nicest would be both, differential uploading _and_ property API.

I really like the property idea very much, it's cool! The structure will get
binary incompatible when ever an extension is needed. Even, for compatible
extensions, properties could get default values when they are not sent by
the user on effect creation.

But to the example above: When will the parameter packet be sent to the
device? On effect re-start or on each ioctl call? Both would'nt be good if
we have a packet type, wich does not need an effect re-start and which has
more than one parameter. So, what about sending an unsorted set of properties
on each ioctl call? I mean something like this:

struct effect_property prop[3];
prop[0].type = FF_PROP_ATTACK_LEVEL;
prop[0].value = ...
prop[1].type = FF_PROP_ATTACK_DURATION;
prop[1].value = ...
prop[2].type = FF_PROP_END;
ioctl(fd, EVIOCSFF, prop);

In addition, to make the start/stop concept easier, you could add
a property for it (e.g type=FF_PROP_PLAY, value=FF_START or FF_STOP),
so that the effect is (re-)started or stoped on each reception of
this property. Then the user could modify and re-start an effect
by just one ioctl call.

Hmm, my next thought is to write the set of properties to the device,
instead of calling ioctls. What about that?

- - - - - - -
Oliver Hamann

Re: Force feedback API

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

On Sun, Sep 30, 2001 at 06:37:47AM +0200, Oliver Hamann wrote:
> Hi!
> 
> Lessening bug 3 isn't any longer a reason to discuss about the API (see
> my previous mail in the mailing list). But i like it anyway - do you even?
> 
> Vojtech Pavlik wrote:
> > 
> > On Sat, Sep 29, 2001 at 02:03:11PM +0200, Johann Deneux wrote:
> > 
> > > I am not especially willing to make the distinction between effect
> > > packets and parameter packets visible to the user. Other protocoles
> > > may have a different understanding of what goes into an effect packet,
> > > and what goes into a parameter packet. What I could do is store the
> > > effects uploaded with ioctl in the driver. When other ioctls are
> > > performed to update an effect, I could analyse the difference and send
> > > only the necessary packets. Or I could change the API. Each call to
> > > ioctl would set one property of an effect:
> > >
> > > struct effect_property prop;
> > > prop.type = FF_PROP_ATTACK_LEVEL;
> > > prop.value = ...
> > > ioctl(fd, EVIOCSFF, &prop);
> > >
> > > prop.type = FF_PROP_ATTACK_DURATION;
> > > prop.value = ...
> > > ioctl(fd, EVIOCSFF, &prop);
> > > ...
> > >
> > > Instead of the one big ff_effect structure used now.
> > 
> > I think the differential uploading is nicest.
> 
> I think the nicest would be both, differential uploading _and_ property API.
> 
> I really like the property idea very much, it's cool! The structure will get
> binary incompatible when ever an extension is needed. Even, for compatible
> extensions, properties could get default values when they are not sent by
> the user on effect creation.
> 
> But to the example above: When will the parameter packet be sent to the
> device? On effect re-start or on each ioctl call? Both would'nt be good if
> we have a packet type, wich does not need an effect re-start and which has
> more than one parameter. So, what about sending an unsorted set of properties
> on each ioctl call? I mean something like this:
> 
> struct effect_property prop[3];
> prop[0].type = FF_PROP_ATTACK_LEVEL;
> prop[0].value = ...
> prop[1].type = FF_PROP_ATTACK_DURATION;
> prop[1].value = ...
> prop[2].type = FF_PROP_END;
> ioctl(fd, EVIOCSFF, prop);

Looks OK to me, too.

> In addition, to make the start/stop concept easier, you could add
> a property for it (e.g type=FF_PROP_PLAY, value=FF_START or FF_STOP),
> so that the effect is (re-)started or stoped on each reception of
> this property. Then the user could modify and re-start an effect
> by just one ioctl call.
> 
> Hmm, my next thought is to write the set of properties to the device,
> instead of calling ioctls. What about that?

I don't like this much. Ioctls shouldn't be used for starting/stopping
things, ioctls should be for setting things up. Just a matter of taste.

-- 
Vojtech Pavlik
SuSE Labs

Re: Force feedback bugs

[Oliver H. <au...@od...>]

Hi,

Johann Deneux wrote:
> On Sat, 29 Sep 2001, Oliver Hamann wrote:
> > BUG 1: machine freeze
> > =====================
> >
> > BUG 2: force freeze
> > ===================
> >
> > BUG 3: force interrupts
> > =======================
> >
> > Somehow I tend to believe that the bug nests in the serial part,
> > because the highler level code looks so clear. My wheel is on rs232.
> > Did you ever encounter the bug with usb?
> 
> Yes, I do have the same problem with usb. I thought that maybe the
> cause was my asynchronous serial code. But even when coming back to
> a synchronous model, the problem subsists.

The fact that the bug is under both, rs232 and usb, motivated me to
take a careful look again into your send_packet function, and this
time i found it quickly. It's the circular buffer wrapping:

   memcpy(&iforce->xmit.buf[head],
          data,
          c);
   if (n != c) {
      memcpy(&iforce->xmit.buf[0],
             data,         <-- here's the mistake, should be: data+c
             n - c);
   }

It's the clear reason for bug 2 and 3. (proved by test)

Could this, somehow, even be the reason for bug 1?
Remember: the packet size is stored in the packet, the rs232 and usb
code depends on this. There may be great confusion when the size is
damaged by the circular buffer mistake.

- - - - - - 

Oliver Hamann