[etherlab-dev] [PATCH] Default branch patchset re-applied on 5a70ff
Gavin Lambert
gavinl at compacsort.com
Wed Jun 15 10:08:50 CEST 2016
And here we go. Changes in this patchset from the previous one:
* 0013-Improved-EtherCAT-rescan-performance.patch:
Fixed some minor whitespace (indentation) errors that crept in somewhere.
* 0041-load-sii-from-file.patch:
Rewrote to eliminate some bugs and make the code considerably tidier.
Incorporates the async behaviour as standard. (As before, the feature as a
whole is disabled by default, but can be enabled at configure time.)
* 0042-load-sii-from-file-async.patch:
Dropped, as it's now folded into the prior patch.
Since 0041 is new, it hasn't had extensive testing yet - however I have
verified that all the combinations of configure parameters compile and that
the cache and overrides behave as expected in a small network, so I don't
expect any issues.
From: Gavin Lambert
Sent: Wednesday, 15 June 2016 11:19
To: 'Knud Baastrup' <kba at deif.com>
Cc: etherlab-dev at etherlab.org
Subject: Re: [etherlab-dev] [PATCH] Default branch patchset re-applied on
5a70ff
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:kba at deif.com]
Sent: Wednesday, 15 June 2016 00:51
To: Gavin Lambert <gavin.lambert at compacsort.com
<mailto:gavin.lambert at compacsort.com> >; Florian Pose <fp at igh.de
<mailto:fp at igh.de> >; 'David Page' <dave.page at gleeble.com
<mailto:dave.page at gleeble.com> >
Cc: etherlab-dev at etherlab.org <mailto:etherlab-dev at 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:gavinl at compacsort.com]
Sent: 13. juni 2016 08:34
To: Knud Baastrup; Florian Pose; 'David Page'
Cc: etherlab-dev at etherlab.org <mailto:etherlab-dev at 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 udev/hotplug lookup process.
3. At configure time, you can use -enable-sii-override=/lib/firmware
(or another path) to activate it using a direct loading method.
4. A bug where it would fail to load 6 words of the SII when not
loading from a file has been corrected.
5. Several places where it exited the state prematurely have been
corrected.
6. It will cooperate as expected with patch 0013, although note that
it's not as efficient as it could be (it will reload some of the values that
patch 0013 already read when checking the SII cache; but trying to improve
this would make the code really awkward).
* Added 0042-load-sii-from-file-async.patch, which makes the above
firmware loading asynchronous so it doesn't block the master thread, since
that makes me uncomfortable. Again this could be folded into the prior
patch but I left it separate so that the differences are apparent and easier
to review. I'm also pondering whether the firmware loading stuff (or
perhaps even the whole SII loading stuff including the reuse cache) should
be broken out into a separate sub-FSM file. It's not really complicated but
it's a bit wordy, particularly with the direct-loading support included.
Open to suggestions here.
* (Note these additions aren't breaking changes, but I didn't want
to renumber the patches just because of that. They should be able to apply
prior to the breaking change patches if you want to try it that way.)
Some other thoughts about the new patches (I haven't modified anything for
these):
* 0032-Correction-to-Clear-slave-mailboxes-after-a-re-scan.patch:
As previously mentioned, the stated circumstances should never happen, as a
slave is not permitted to refuse to enter INIT. But the patch itself is
probably harmless.
* 0034-Tool-Withdraw-EEPROM-control-for-SII-read-write.patch:
This seems like it should have some conditioning on EC_SII_ASSIGN, since
unless that is enabled the SII should always be assigned to ECAT anyway.
Additionally the changes to CommandSiiRead.cpp are completely unnecessary
(or don't work as intended), as this doesn't access the device - it reads
the SII contents as cached during scanning (even before patch 0013
happened). I also wonder if perhaps the mode switch on writing should be
done in the state machines instead, so that it can be switched to ECAT only
just prior to the write and then restored afterwards.
Please let me know if you have any questions or suggestions, or if there's a
different way I can present these that would help them get merged faster. :)
From: Gavin Lambert
Sent: Sunday, 12 June 2016 01:25
To: Knud Baastrup < <mailto:kba at deif.com> kba at deif.com>; Florian Pose <
<mailto:fp at igh.de> fp at igh.de>; 'David Page' < <mailto:dave.page at gleeble.com>
dave.page at gleeble.com>
Subject: Re: [PATCH] Default branch patchset re-applied on 5a70ff
Heh, looks like you beat me to it - I was in the process of rebasing to the
same point (along with some additional tweaks - in particular I'm testing
the loadable SII patch Graeme provided a few weeks ago. Sadly it has some
bugs but I think I have it nearly ready).
I'm away from my test setup ATM but I'll have a look at these next week.
Sent from my Samsung Galaxy smartphone.
-------- Original message --------
From: Knud Baastrup < <mailto:kba at deif.com> kba at deif.com>
Date: 11/06/2016 00:55 (GMT+12:00)
To: Florian Pose < <mailto:fp at igh.de> fp at igh.de>, 'David Page' <
<mailto:dave.page at gleeble.com> dave.page at gleeble.com>, Gavin Lambert <
<mailto:gavin.lambert at compacsort.com> gavin.lambert at compacsort.com>
Cc: <mailto:etherlab-dev at etherlab.org> etherlab-dev at etherlab.org
Subject: [PATCH] Default branch patchset re-applied on 5a70ff
Hi all,
I have re-applied the patch serie from Gavin Lambert on the latest commit on
default branch (5a70ff from 2016-05-04) and as well added some additional
(minor) patches (see the short explanation in the patch it-selves). I have
placed the "Breaking change" patches in the end and done some extensive
testing including all patches up to and including patch 0037. The tests have
involved a lot of EoE traffic tests that all passed successfully.
The patches are created with GIT (and named according to the commit message)
but I believe they can easily be applied on HG as well. I have as well
cleaned the patches for all white-space related issues.
The patch serie is quite long and a bit cumbersome to maintain so I hope we
soon can get more of them included on the default branch.
* 0001-Distributed-Clock-fixes-and-helpers.patch (same as
0001-graemef-dc_fixes_and_helpers.patch)
* 0002-Distributed-Clock-fixes-from-Jun-Yuan.patch (same as
0002-junyuan-dc_sync_issues.patch)
* 0003-Do-not-reuse-the-index-of-a-pending-datagram.patch (same as
0003-frank-index-reuse.patch)
* 0004-If-enable-rtmutex-use-rtmutexes-instead-of-semaphore.patch
(same as 0004-gavinl-Semaphores-replaced-with-mutexes.patch)
* 0005-Support-for-multiple-mailbox-protocols.patch (same as
0005-knud-Support-for-multiple-mailbox-protocols.patch)
* 0006-Await-SDO-dictionary-to-be-fetched.patch (same as
0006-knud-Await-SDO-dictionary-to-be-fetched.patch)
* 0007-Protection-of-external-datagram-queue.patch (same as
0007-knud-Protection-of-external-datagram-queue.patch)
* 0008-Clear-slave-mailboxes-after-a-re-scan.patch (same as
0008-knud-Clear-slave-mailboxes-after-a-re-scan.patch)
* 0009-Skip-output-statistics-during-re-scan.patch (same as
0009-knud-Skip-output-statistics-during-re-scan.patch)
* 0010-Sdo-directory-now-only-fetched-on-request.patch (same as
0010-knud-Sdo-directory-now-only-fetched-on-request.patch)
* 0011-Master-locks-to-avoid-corrupted-datagram-queue.patch (same as
0011-knud-Master-locks-to-avoid-corrupted-datagram-queue.patch)
* 0012-Ignore-mailbox-settings-if-corrupted-sii-file.patch (same as
0012-knud-Ignore-mailbox-settings-if-corrupted-sii-file.patch)
* 0013-Improved-EtherCAT-rescan-performance.patch (same as
0013-knud-Improved-EtherCAT-rescan-performance.patch)
* 0014-Fix-NOHZ-local_softirq_pending-08-warning.patch (same as
0016-knud-Fix-NOHZ-local_softirq_pending-08-warning.patch)
* 0015-EoE-processing-is-now-only-allowed-in-state-PREOP.patch (same
as 0017-knud-EoE-processing-is-now-only-allowed-in-state-PREOP-SA.patch)
* 0016-Prevent-abandoning-the-mailbox-state-machines-early-.patch
(same as 0101-gavinl-mbox-finish-sm.patch)
* 0017-Make-busy-logging-a-little-less-irritating.patch (same as
0102-gavinl-sdo_logging_verbosity.patch)
* 0018-Clear-configuration-on-deactivate-even-if-not-activa.patch
(same as 0103-gavinl-clear_on_deactivate.patch)
* 0019-After-a-comms-interruption-allow-SAFEOP-OP-directly-.patch
(same as 0104-gavinl-quick_op_watchdog.patch)
* 0020-Adding-some-more-state-to-avoid-calling-the-more-exp.patch
(same as 0105-gavinl-more_state.patch)
* 0021-Detect-bypassed-ports-timestamp-not-updated.patch (same as
0106-gavinl-topology_bypass.patch)
* 0022-Calculate-most-likely-upstream-port-for-each-slave.patch
(same as 0107-gavinl-topology_upstream.patch)
* 0023-Print-redundancy-device-name-with-ring-positions-as-.patch
(same as0108-gavinl-redundant_device.patch)
* 0024-RX-does-not-reset-watchdog-breaks-redundancy.patch (same as
0109-gavinl-e1000e_watchdog.patch)
* 0025-Add-command-to-request-hardware-reboot-for-slaves-th.patch
(same as 0110-gavinl-reboot.patch)
* 0026-Add-register-read-write-support.patch (same as
0111-gavinl-reg_readwrite.patch)
* 0027-Display-more-info-about-the-register-requests-in-pro.patch
(same as 0112-gavinl-reg_req_info.patch)
* 0028-Support-SDO-upload-via-complete-access.patch (same as
0113-gavinl-sdo_complete_upload.patch)
* 0029-Disable-DC-SYNC-before-updating-a-running-slave-s-sy.patch
(same as 0116-gavinl-dc-sync-vs-sys-time.patch)
* 0030-Use-call-back-functions.patch (NEW)
* 0031-Added-newline-to-syslog-message-MAC-address-derived.patch
(NEW)
* 0032-Correction-to-Clear-slave-mailboxes-after-a-re-scan.patch
(NEW)
* 0033-Corrected-debug-level-for-Aborting-dictionary.patch (NEW)
* 0034-Tool-Withdraw-EEPROM-control-for-SII-read-write.patch (NEW)
* 0035-Add-support-for-bringup-up-network-interface-when-st.patch
(NEW)
* 0036-Reduced-printing-to-avoid-syslog-spam.patch (NEW)
* 0037-Breaking-change-rt_mutx_lock_interruptible-calls-for.patch
(NEW)
* 0038-Breaking-change-require-data-size-to-be-specified-in.patch
(same as 0114-gavinl-sdo_write_size.patch)
* 0039-Breaking-change-Support-for-SDO-complete-access-in-e.patch
(same as 0115-gavinl-sdo_async_complete.patch)
Venlig hilsen / Best regards,
Knud Baastrup
DEIF Wind Power Technology
SW Developer
Direct.: +45 9614 8458
E-mail: <mailto:kba at deif.com> kba at deif.com
---------------------------------------------------------------
DEIF A/S
Frisenborgvej 33
DK-7800 Skive
Tel.: +45 9614 9614
Fax: +45 9614 9615
<http://www.deifwindpower.com/> www.deifwindpower.com
"This email and any files transmitted with it are confidential and intended
solely for the use of the individual or entity to whom they are addressed.
Please notify the sender immediately by e-mail if you have received this
e-mail by mistake and delete this e-mail from your system. If you are not
the intended recipient you are notified that disclosing, copying,
distributing or taking any action in reliance on the contents of this
information is strictly prohibited"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.etherlab.org/pipermail/etherlab-dev/attachments/20160615/0e7debd5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches-gavinl-20160615.zip
Type: application/x-zip-compressed
Size: 114438 bytes
Desc: not available
URL: <http://lists.etherlab.org/pipermail/etherlab-dev/attachments/20160615/0e7debd5/attachment-0001.bin>
More information about the etherlab-dev
mailing list