Re: [etherlab-dev] [PATCH] Default branch patchset re-applied on 5a70ff

[Gavin Lambert]

Thanks.

 

Just as an FYI, I've discovered a memory leak in patch 0042 in more recent
kernel versions, and I decided that it makes more sense to lift the
file-loading part to a separate source file, so I'm in the process of
rewriting patches 0041 and 0042 again.  (Mainly rewriting 0042, but in the
next version I'll fold them together as it will make things more readable.)

 

I'm still interested in any feedback on their current implementation, but
the new version is going to look much cleaner, I think.  I'm hoping to have
a new version of the patch ready in a few hours, but it might take a little
longer. :) 

 

Regarding patch 0034, two potential improvements that occurred to me after
looking through it:

* A successful SII write should update the SII cache, so that an SII
read will read it back without a rescan.

* It might be useful to have an option to sii_read that does cause
it to read the actual device SII without a full rescan; particularly for
devices that store calibration or other custom settings in the SII.  Or
alternately a way to force a rescan to not use the cached SII.  (Having said
that, it's reasonably easy to clear the cache via a "service ethercat
restart", which should suffice in most cases.)

These go beyond the scope of the original patch, of course; they're just
ideas, and I don't mean to imply any sort of obligation.  Just some thoughts
if you're rewriting it anyway. :) 

 

From: Knud Baastrup [mailto:k...@deif.com] 
Sent: Wednesday, 15 June 2016 00:51
To: Gavin Lambert ; Florian Pose ;
'David Page' 
Cc: etherlab-dev@etherlab.org
Subject: RE: [PATCH] Default branch patchset re-applied on 5a70ff

 

Gavin, we really appreciate the time you put into reviewing and improving
the patches.

 

I have done some more testing with focus on the EoE part and in one setup I
have now observed that the EtherCAT-EoE thread can end up in an forever
uninterruptable sleep. I can reproduce this in both the patch serie from
20160502/20160610 and in the newest patch serie from 20160613. I will
investigate this further.

 

A few comments to below patches:

 

Patch 0037:

The versioning is a bit tricky and to be safe we have to use version 3.17
where the detect_deadlock argument for sure is dropped. If using the RT
kernel patch, the argument must be dropped from 3.14.34-rt32, but it is not
possible to do a check on the rt32 version part that is just given by a tag.
I will update the patch to use version 3.17 and maybe also allow the
detect_deadlock argument to be dropped via configure.ac so we can support
the Linux Real time versions from 3.14.34-rt32.

 

Patch 0032:

Yes, now I recall that we discussed this about a year ago and it is actually
the commit message that is a bit wrong as we do actually enter INIT + ERROR
after the Master has requested PREOP. I will update the commit message to
make this a bit more clear.

 

Patch 0034:

Yes, I should have known that this of cause is unnecessary for the read part
(as I have previously worked with the SII cache part). I will update the
patch and remove this part again.

 

Thanks,

 

Knud

 

From: Gavin Lambert [mailto:gav...@compacsort.com] 
Sent: 13. juni 2016 08:34
To: Knud Baastrup; Florian Pose; 'David Page'
Cc: etherlab-dev@etherlab.org  
Subject: RE: [PATCH] Default branch patchset re-applied on 5a70ff

 

Ok, I've looked through the new patches now.  Attached is my refresh of
them.  Mostly it's just inclusion of a series file and cleaning up the
commit messages to be HG-safe (HG doesn't quite like some of the things that
git adds, such as diffstats and a few other artefacts).  Other changes
include:

 

* Adopted the filenames from Knud's set.

* Replaced
0037-Breaking-change-rt_mutx_lock_interruptible-calls-for.patch with a
version that isn't a breaking change (assuming the version numbers in the
commit message were correct; I haven't verified this).  This could probably
be folded into patch 0004, but I left it separate for clarity.

* Added 0040-rescan-check-revision.patch.  This modifies patch 0013
in four ways:

1.   The SII cache-and-reuse behaviour can be disabled via
-disable-sii-cache at configure time, rather than requiring modifying a
header file.

2.   The revision number is also verified before using the cached
version (this resolves some issues when the device firmware is upgraded).

3.   If both the alias and serial are read as 0, it will no longer
bother reading the vendor/product/revision, as it is now known that the SII
is not in the cache.

4.   Several similar states are consolidated into one.

* Added 0041-load-sii-from-file.patch, from Graeme Foot's recent
patch; but I've made the following modifications:

1.   The functionality is disabled by default.

2.   At configure time, you can use -enable-sii-override to activate it,
using the standard 

Re: [etherlab-dev] [PATCH] Default branch patchset

[Graeme Foot]

Hi,

Jesper originally needed the patch due to his module requiring a 3kb SII (which 
he generated from the TwinCAT xml), but the module only had 2kb of memory.  The 
patch loads the first 32 bytes of the xml:
- alias
- vendor_id
- product_code
- revision
- serial number

from this it tries to find first a revision based file, then a generic product 
file
- 1st: ethercat/ec_%08x_%08x_%08x.bin(vendor_id, product_code, revision)
- 2nd: ethercat/ec_%08x_%08x. bin(vendor_id, product_code)

The alias is read from SII, so that is fine.  Revision specific SII's is also 
covered.  If a file can't be found then the remaining SII information is read.


I needed this patch due to our early Yaskawa amps having a faulty SII.  The amp 
also did not allow new SII's to be uploaded.  Downloading the SII from a newer 
amp and reading it from file for the older amp worked well.


So the cases where it is needed is if the SII memory is not big enough, or the 
SII memory is faulty and the module does not allow new SII information to be 
uploaded.


I have attached the patch.


Regards,
Graeme.


-Original Message-
From: Gavin Lambert [mailto:gav...@compacsort.com] 
Sent: Wednesday, 4 May 2016 11:13 a.m.
To: Graeme Foot
Subject: RE: [etherlab-dev] [PATCH] Default branch patchset

On Wednesday, 4 May 2016 10:55, quoth Graeme Foot:
> Are you interested in a patch that can read the SII information for a
slave from
> disk (if the slave has SII problems)?
> 
> It can look up the SII "firmware" using the hotplug/udev framework
(original
> patch from Jesper Smith) or directly from a given directory via a 
> couple
of
> defines (my changes).

It's an interesting idea, certainly.  It's probably not something I'd make use 
of myself (all the slaves I'm using have good SII data) but I know there are 
slaves out there where the vendor has forgotten to program the EEPROM fully.

The naming convention bothers me, though; I would expect the SII data needs to 
differ between different revisions of the slave as well (it certainly does for 
my slaves).

Another problem is that without real SII data on the slaves themselves, they 
can't hold unique aliases, which is something else that I consider mandatory on 
my networks, at least.  So I'm inclined to think that the "correct"
reaction is to reprogram such slaves with their correct SII rather than trying 
to patch it at runtime.

Although at least part of the SII must be working, otherwise you wouldn't be 
able to get the vendor/product ids (unless you're using an ENI file like 
TwinCAT, but that's a massive departure from the Etherlab model).

I'm not opposed to including it anyway (if for no other reason than keeping 
handy patches in one place makes them less likely to get lost in the mists of 
time) but I'm curious what your motivating cases were?




2635-graemef-load_SII_from_file.patch
Description: 2635-graemef-load_SII_from_file.patch
___
etherlab-dev mailing list
etherlab-dev@etherlab.org
http://lists.etherlab.org/mailman/listinfo/etherlab-dev