[etherlab-dev] Multiple mailbox protocols and other issues

[Gavin Lambert]

On 2 February 2015 20:18, you quoth:
> 
> I will just update you on some additional pathes I have prepared for
EtherCAT
> Master. I have attached the complete set of patches that we currently use,
> but only the below patches have been updated added.

I'm just having a more detailed look through these patches now, and there's
a few niggles. ;)

1. There are several files that appear to have tabs in them; it's usually a
good idea when sharing patches with others to use spaces only, as different
people/editors use different tab sizes.

2. There's several diffs in various files (eg. 12_sdo_directory.patch's
master/ioctl.h) that contain only whitespace changes on various lines for no
readily apparent reason.  These sorts of things can cause unnecessary
conflicts and hide the true intent of the patch.  This may have been the
result of a space -> tab -> space conversion gone wrong.

3. This is optional, but I think it's good style to include a short text
description at the top of each patch file, which can act as a commit
message, and helps people reading the patch later without having to hunt
down the original email.  (If you're using HG MQ it does this automatically;
I think git format-patch will also do this for you if you have a branch
structured with one commit per patch, though it also adds quite a bit of
extra email-header junk.)

4. Regarding 13_domain_lock.patch, I believe the original rationale of the
master is that locking between concurrent application tasks is the
responsibility of the application, not the master -- that's why in
kernelspace it has send/receive callbacks (formerly lock/unlock callbacks)
so that eg. RTAI locks can be substituted, or locking can be avoided if the
application has some other way to schedule things to avoid actual
concurrency (or if it's only running a single task).  See the "Concurrent
Master Access" section in the docs.  I don't have any personal objections to
this patch though.

5. Regarding 14_fix_string_handling.patch, I don't think this is the "right"
fix.  I've attached Frank's 04 patch which fixes this a different way.

6. Regarding 16_improved_ethercat_rescan_performance.patch, it looks like a
stray temporary file was included in the patch.  Also, I'm not sure it's
safe to retrieve the data only by serial number.  Serial numbers are not
guaranteed unique between vendors, or even between product lines -- I think
at minimum you should include the vendor id and product code in the index.
Also, possibly this should have a #define config guard to disable this
functionality in case the master will be used at a site with pathological
slaves (eg. multiple slaves with identical non-zero vendor/product/serial
triplets, since *technically* they're not guaranteed unique at all --
although any slave vendor who does this deserves a kick).

I haven't had a chance to test things locally yet, but at least everything
is compiling ok with these patches. :)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: frank_04-string-download.patch
Type: application/octet-stream
Size: 1123 bytes
Desc: not available
URL: <http://lists.etherlab.org/pipermail/etherlab-dev/attachments/20150209/4b608f22/attachment-0004.obj>