[etherlab-dev] ethercat-1.5: Various issues

[Frank Heckenbach]

> I have not applied your locking patches as yet, partly because the 1.5.2
> code seems quite different and I'm not sure how to correctly apply them, and
> because most of them seem to be focused on locking for RTAI applications,
> and (at least at the moment) I'm using a userspace app, which has a somewhat
> different locking model.

The issues regarding io_sem (#18, #19, #20) apply to all code, RTAI
or not.

I've looked at your changes, but as I said, I can't test partially
patched versions (and I can't test anything right now anyway), so
for now I only picked a few of your changes into my patch set which
I also append for reference; basically those changes that seem
absolutely safe to me.

(I also didn't take most of your reformatting changes. In my
patches, I tried to stick to the apparent coding style in 1.5.0;
maybe it has changed in 1.5.2, but since my patches are, and
possibly might remain, for 1.5.0, I'll stick to that style. I only
took a few changes where my version indeed didn't match that style.)

> > If it's something else (which I don't see ATM), you could make it wrap-
> > around-safe by changing
> > 
> >   jiffies_poll > jiffies_sent
> > 
> > to
> > 
> >   jiffies_poll - jiffies_sent > timeout_jiffies
> > 
> > and likewise with cycles. (But I wouldn't recommend this without finding
> > the root cause of the problem.)
> 
> That's exactly what the existing (pre-patch) code already does though,

Sorry, I meant the reverse check (but again, only as a last resort),
so datagrams that are just a little "too new" aren't timed out:

  jiffies_sent - jiffies_poll > timeout_jiffies

I'll do this in my version for now.

> > One reason would be patch #16 which means datagrams can be queued and
> > not sent for a while (instead of corrputing other frame data).
> > If so, the change would be in the wrong patch, of course. I'm not 100%
> > sure at the moment if something in #11 doesn't also require it, but I
> > think #16 is reason enough, so I don't have to check #11 in detail, do
> > I?
> 
> It didn't seem to me that #16 needed it, but maybe I missed something.
>
> 11-mailbox-buffer:
>   Omits your change to time out QUEUED datagrams as well as SENT ones.
> [Remainder of fix for invalid timeout values.]

I'm not sure that's the fix, or why this would produce such values
(did you try to debug it as I described in my last mail?), so for
now I'll stick to my code which works for me.

> 28-dictionary-fetched:
>   Also added to ec_slave_info_t (for ecrt_master_get_slave).
>   Bumped EC_IOCTL_VERSION_MAGIC to allow userspace apps to detect
> incompatible versions.

OK, but I think you should also add the line to
ecrt_master_get_slave in master/master.c (not only to
ecrt_master_get_slave in lib/master.c).

> 30-debug-level:
>   New patch; alters the debug level checks from patch 11 to level 2 from
> level 1 for the messages printed in the common case, to reduce log flooding
> at level 1.  (I made this a separate patch because the original behaviour
> can be useful when testing.)

In my version, I made this change in #11 directly. (When testing,
you can increase the debug level; having more different versions of
the patches makes them even more unmaintainable than they apparently
already are.)

Regards,
Frank

-- 
Dipl.-Math. Frank Heckenbach <f.heckenbach at fh-soft.de>
Stubenlohstr. 6, 91052 Erlangen, Germany, +49-9131-21359
Systems Programming, Software Development, IT Consulting
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ethercat-1.5.0-patches-v2.tar.bz2
Type: application/octet-stream
Size: 19363 bytes
Desc: not available
URL: <http://lists.etherlab.org/pipermail/etherlab-dev/attachments/20140627/7b348e4a/attachment-0004.obj>