[etherlab-dev] Multiple mailbox protocols and other issues

[Gavin Lambert]

On 11 July 2014, quoth Knud Baastrup:
> Patch 2 (ethercat_152_patch_2_foe.patch):
> Wrong datagram used for timeout calculation.

You'll be happy to learn that this patch is not required if you use the
latest code; it was fixed in 8bb574da5da2.

> Patch 4 (ethercat_152_patch_4_eoe_mac.patch)
> Change the MAC address for eoe0sY devices to real local 
> administrated MAC addresses based on the NIC part of eth0 and 
> the EoE slaves ring position.

In the line "if (ETH_ALEN >= 5)", shouldn't that be > 5 or >= 6?  Also, if
this test fails (which seems unlikely, but then why test if it can't
happen?), this will leave the address uninitialized, which seems
undesirable.  Maybe it should fall back to the prior code in that case?

> Patch 5 (ethercat_152_patch_5_priority_inheritance.patch)
> Replaced semaphores with mutexes to utilize priority inheritance 
> and limit impact from lower priority tasks (EtherCAT-EOE) running 
> as sched_other task. 

While I like the idea of using RT-mutexes, they do have a minimum kernel
version (I'm not sure exactly what that is except that it's somewhere in the
2.6.x series) and currently Etherlab provides drivers for some kernel
versions that are before that cutoff, I suspect.  (And do rt_mutexes and
RTAI play nicely together or not?)  So this might break compatibility, and
so possibly should be a configure option.  Or maybe nobody cares about those
older kernel versions any more?  (I don't personally, I'm just wondering if
it might be a concern.)

Also I found a few cases where "down" and "up" hadn't been changed to
"rt_mutex_lock" etc.  Not sure if this was the result of a patch application
failure or if this was code added since the patches' base version.

> Patch 6 (ethercat_152_patch_6_mailbox.patch)
> Alternative solution to Patch 9-10-11 provided by Frank Heckenbach for 
> 1.5.0 (that I did not succeed to get up running on 1.5.2).

Did you look at the updated-to-1.5.2 patches that I posted?

> In this solution I accept that a mailbox read request (e.g. FP-RD) for 
> a given mailbox protocol can return data from any other mailbox protocol 
> running at the same time.  [...]

In master/master.c near line 1240, on receipt of a datagram you're searching
through the slave list to check its mailbox settings.  This code appears
unsafe in the case when this search fails (which presumably could occur eg.
if a datagram with a corrupted address arrives or if slave scanning has just
started and cleared master->slaves).

Also this seems to generate quite a lot of "Await configured mailbox
address" spam at debug level 1 even when no mailbox activity is taking
place..?

When compiling with a recent kernel version (3.13) I needed to #include
<linux/rtmutex.h> in master/slave.h in order to compile several files (the
first is master/fmmu_config.c).  This doesn't appear to be needed in 3.2
however (or by some of the other .c files); I guess the indirect includes
have changed?

> Patch 10 (ethercat_152_patch_10_scan_skip_stats.patch)
> No reason to write output statistics in syslog when issuing a slave
scanning 
> where UNMATCHED datagrams are expected behavior.

I'm not sure I follow this one.  Why are unmatched datagrams expected?
Also, this patch isn't going to stop them printing, just delay it until the
scan completes.

Otherwise, this patchset seems to work ok.  I've only given it fairly basic
testing thus far though.

However there was a bit of fuzz and other conflicts when trying to apply
these to the latest HG source, so in the interests of making it easier for
Florian and anyone else using the latest source to examine and test these
patches, I've attached updated versions.  These have only had the following
changes:
 - de-fuzzed based on 8dd49f6f6d32 (close to stable-1.5 tip).
 - various tabs converted to whitespace and related minor whitespace
changes.
   - I noticed some inconsistent newline-brace styles but I left those
alone.
 - omitted patch 2 as specified above (it was empty after defuzzing).
 - converted a few extra down/ups in patch 5 as specified above.
 - added #include to master/slave.h in patch 6 as specified above.

In particular I've basically just included changes required to compile; I
haven't tried to fix any of the other possible issues mentioned above.

I've also included a series file, so in theory it should immediately be
MQ-compatible if extracted into the .hg dir.  (You might need to "hg qq -c
knud" first.)

Regards,
Gavin Lambert

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches-knud.tar.gz
Type: application/octet-stream
Size: 22882 bytes
Desc: not available
URL: <http://lists.etherlab.org/pipermail/etherlab-dev/attachments/20140806/6ff55acc/attachment-0003.obj>