[etherlab-dev] Multiple mailbox protocols and other issues

[Knud Baastrup]

Thanks Gavin, see my inline comments below!

BR, Knud

-----Original Message-----
From: Gavin Lambert [mailto:gavinl at compacsort.com] 
Sent: 9. februar 2015 08:44
To: Knud Baastrup
Cc: etherlab-dev at etherlab.org
Subject: RE: [etherlab-dev] Multiple mailbox protocols and other issues

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.
[Knud] I fully agree, I will update and try to avoid in future patches!

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.
[Knud] I fully agree, I will update and try to avoid in future patches!

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.)
[Knud] I have the patches structured with one commit per patch so using git-format-patch will work fine. I will apply.

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.
[Knud] Yes, we just faced some cases where the application developers did not include the necessary locking, which have quite severe impact for the complete system. We have not used RTAI, but I am not sure I understand why the extra locks become a problem for RTAI?

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.
[Knud] I will talk with my colleague Per and check if we can revert this patch and instead use the one provided by Frank.

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).
[Knud] Yes sorry, my mistake with the temporary file. I can only agree with you that vendor and productcode should be included in the index in order for this patch to be used in large scale, I will add this. I can also agree with the #define guard.

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