Hi Gavin,

Thanks for the feed-back, I have in-lined some comments below.

Mvh. Knud

From: Gavin Lambert [mailto:gav...@compacsort.com]
Sent: 25. juni 2014 02:02
To: Knud Baastrup
Cc: etherlab-dev@etherlab.org
Subject: RE: [etherlab-dev] Support for multiple mailbox protocols

Hi Knud,
I haven't reviewed or tested the whole patch yet, but I like the idea in 
concept.  One thing that made me pause though is the way that the check_mbox 
flag is handled.  You have it protected by a semaphore (implying you're 
expecting it to get used concurrently), but you have non-atomic 
test-and-then-set actions which will mean that if concurrent access is 
attempted multiple users might call prepare_check and check_mbox_set (causing 
some of them to be "lost", which may or may not be a problem), and then on 
failure one might call check_mbox_clear while another is still waiting for a 
check to occur (which seems like it would be a problem).
[Knud] We have concurrent mailbox access due to the EoE Handlers running in a 
separate thread. I fully agree that it is a mistake that I have not used a 
test-and-set operation for the check_mbox flag and I will correct this in an 
updated patch.

Again, I haven't followed the logic all the way through yet so possibly this 
isn't a real problem, but it bothers me. :)

If the intent is to have only one concurrent state machine trigger a check 
datagram at a time (which it seems like it is), then you should probably be 
using an atomic test-and-set operation instead.

Additionally it doesn't look like you have any protection against concurrent 
CoE access (which TBH I'm not entirely sure whether this occurs, but Frank's 
patch 27 suggests it does), and I'm definitely not a fan of 
allocating/deallocating memory on each mailbox transfer, which is what it looks 
like you're doing.
[Knud] I believe that the check_mbox flag should work ok for concurrent CoE 
access as well (however, I can as well not see how this can happen?) as the 
check_mbox flag will ensure only one ongoing read request per slave no matter 
which mailbox protocol.
[Knud] Each fetch data datagram is already allocating memory corresponding to 
the mailbox size so allocation memory is already heavily used. In my case, I 
know the payload size of the returned data, which in most cases is 
significantly smaller than the configured mailbox size and that is why I choose 
to allocate. In my test setup I have slaves with both 192 bytes and 1024 bytes 
mailboxes and yes I could as an alternative do a one-time allocation once the 
configured mailbox size is known (i.e. when slave configuration/scanning has 
finalized).

Regards,
Gavin Lambert

From: 
etherlab-dev-boun...@etherlab.org<mailto:etherlab-dev-boun...@etherlab.org> 
[mailto:etherlab-dev-boun...@etherlab.org] On Behalf Of Knud Baastrup
Sent: Wednesday, 25 June 2014 01:13
To: etherlab-dev@etherlab.org<mailto:etherlab-dev@etherlab.org>
Subject: Re: [etherlab-dev] Support for multiple mailbox protocols

Hi !

I just discovered that the provided patch included a hardcoded mailbox size 
that I have now replaced with a dynamic allocated buffer. I have attached a new 
patch (ethercat_152_stable_mailbox_1.patch) that fully replaces the prior patch 
(ethercat_152_stable_mailbox.patch).

Thanks,

Knud Baastrup


From: 
etherlab-dev-boun...@etherlab.org<mailto:etherlab-dev-boun...@etherlab.org> 
[mailto:etherlab-dev-boun...@etherlab.org] On Behalf Of Knud Baastrup
Sent: 23. juni 2014 14:27
To: etherlab-dev@etherlab.org<mailto:etherlab-dev@etherlab.org>
Subject: [etherlab-dev] Support for multiple mailbox protocols

Hello Florian, Gavin, Frank (and others facing the lack of support for multiple 
mailbox protocols)


I have like Frank Heckenbach and Gavin also struggled with the lack of support 
for multiple mailbox protocols and came up with an alternative solution to the 
one provided by Frank in patch 9-10-11.

I have attached the patch that is based on the stable-1.5 branch. The patch 
should support all the mailbox protocols, but has only been tested with CoE, 
EoE and FoE.

I will in few lines try to summarize the patch:
In this patch 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. The data returned by a read datagram is therefore stored in a 
separate buffer for each mailbox protocol instead of the datagram data buffer. 
The mailbox state machines will check and fetch the data from their own buffer 
instead of the datagram buffer (that is no longer used for mailbox read data). 
A check_mbox flag is introduced to track when a given slave has an ongoing 
mailbox read request. In normal case the mailbox state machine will run as 
previously if no mailbox read request is ongoing, but if a mailbox read-request 
is ongoing (check_mbox flag is set) it will check its own mailbox buffer (as 
the ongoing mailbox read request might have returned its data) and otherwise 
wait until the read request is done and it gets the opportunity to reserve the 
mailbox for its own read request.


Venlig hilsen / Best regards,
Knud Baastrup
DEIF Wind Power Technology
SW Developer
Direct.: +45 9614 8458
E-mail: k...@deif.com<mailto:k...@deif.com>
---------------------------------------------------------------
[cid:image002.png@01CF908E.454D66D0]Retrofit your Vestas COTAS controller and 
optimize availability that will improve your annual energy generation, reduce 
service cost and extend the lifetime of your turbine.
V27 V39 V44 V47
Read more about DEIF's solutions to retrofit your turbines on our 
website<http://www.deifwindpower.com/retrofit.aspx?utm_source=Retrofit&utm_medium=email%20signatur&utm_term=Retrofit%2BVestas%2BCOTAS&utm_content=textlink&utm_campaign=Retrofit>

_______________________________________________
etherlab-dev mailing list
etherlab-dev@etherlab.org
http://lists.etherlab.org/mailman/listinfo/etherlab-dev

Reply via email to