[etherlab-dev] Multiple mailbox protocols and other issues

[Knud Baastrup]

Thanks, attached updated patches. See inline comments.

BR, Knud

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

On 27 February 2015 21:44, quoth Knud Baastrup:
> 
> Added one additional patch
(0015-Internal-SDO-requests-now-synchronized-with-
> external.patch) that solves an issue with input/output errors when
executing
> the ethercat sdos command (that now fetch directory) while configured 
> SDO requests are executed from user application. Can also be observed 
> with ethercat upload/download from EtherCAT Tool together with the 
> execution of configured SDO requests. See the documentation in the 
> patch it selves for more information.

I just noticed that patch "17_remove_prints_to_avoid_syslog_spam.patch" from the 02022015 patch series appears to have vanished from later series.  I assume this was intentional, as I don't recall seeing the spam it referred to, but I thought I'd mention it just in case.
[Knud] Yes, it was attentional. I believe our observed syslog spam is provoked due wrong usage of the master library.


Also, some compiler warnings are still present from patch 0013:

master/fsm_slave_scan.c: In function 'ec_fsm_slave_scan_enter_attach_sii':
master/fsm_slave_scan.c:494:17: warning: format '%zu' expects argument of type 'size_t', but argument 5 has type 'int' [-Wformat=]
                 EC_SLAVE_DBG(slave, 1, "Slave can re-use SII image data stored."
                 ^
master/fsm_slave_scan.c:502:17: warning: format '%zu' expects argument of type 'size_t', but argument 5 has type 'uint32_t' [-Wformat=]
                 EC_SLAVE_DBG(slave, 1, "Slave can re-use SII image data stored."
                 ^
master/fsm_slave_scan.c:502:17: warning: format '%zu' expects argument of type 'size_t', but argument 6 has type 'uint32_t' [-Wformat=]
master/fsm_slave_scan.c:502:17: warning: format '%zu' expects argument of type 'size_t', but argument 7 has type 'uint32_t' [-Wformat=]
master/fsm_slave_scan.c: In function 'ec_fsm_slave_scan_state_sii_alias':
master/fsm_slave_scan.c:721:5: warning: format '%zu' expects argument of type 'size_t', but argument 5 has type 'int' [-Wformat=]
     EC_SLAVE_DBG(slave, 1, "Alias: %zu\n", slave->effective_alias);
     ^
master/fsm_slave_scan.c: In function 'ec_fsm_slave_scan_state_sii_serial':
master/fsm_slave_scan.c:759:5: warning: format '%zu' expects argument of type 'size_t', but argument 5 has type 'uint32_t' [-Wformat=]
     EC_SLAVE_DBG(slave, 1, "Serial Number: %zu\n",
slave->effective_serial_number);
     ^
master/fsm_slave_scan.c: In function 'ec_fsm_slave_scan_state_sii_vendor':
master/fsm_slave_scan.c:792:5: warning: format '%zu' expects argument of type 'size_t', but argument 5 has type 'uint32_t' [-Wformat=]
     EC_SLAVE_DBG(slave, 1, "Vendor ID: %zu\n", slave->effective_vendor_id);
     ^
master/fsm_slave_scan.c: In function 'ec_fsm_slave_scan_state_sii_product':
master/fsm_slave_scan.c:825:5: warning: format '%zu' expects argument of type 'size_t', but argument 5 has type 'uint32_t' [-Wformat=]
     EC_SLAVE_DBG(slave, 1, "Product code: %zu\n",
slave->effective_product_code);
     ^

The ones complaining about "int" probably need casts to "unsigned" (or uint32_t if you prefer) due to default parameter extension, and the %zu should just be %u for all of them.

Also vendor ids and product codes are usually printed in hex.  Not sure about serial numbers, but "ethercat slaves -v" displays those in hex too, so that seems reasonable.
[Knud] I believe the compiler warnings should be fixed now. I did however not get any Warnings in my build environment despite the usage of -Wall.

On a somewhat related note, I'm not sure "effective_serial" etc are good variable names.  "Effective alias" is phrased that way because there are several different kinds of alias, but this contains the one that's currently in use (eg. see the EC_REGALIAS code, which allows the effective alias to come from a register rather than the SII); but that isn't really true for the vendor/product/serial values.  This is just a minor quibble of course.
:)
[Knud] Yes the naming can be argued, but I will keep the terminology effective_serial_number for now as e.g. the term serial_number has already been used for sii_image->sii.serial_number and you can say that the effective_serial number is effective in the sense that it is the one used when matching sii_images after a rescan.

Although speaking of the EC_REGALIAS code, if that's enabled and if the register 0x0012 alias is different from the SII alias, then this patch might malfunction (it should probably skip reading the SII alias and go straight for the register).  Having said that, normally the two should be the same, unless someone is in the process of changing the alias (in which case rebooting the slave afterwards should "fix" everything).  There might be some odd slaves out there though, which could be why EC_REGALIAS was added in the first place..?
[Knud] The patch should not be malfunctioning, but yes if alias (or a serial number) is updated after a re-scan the stored sii_image cannot be matched in the coming re-scan and a new sii_image will be created for that particular module.

Finally, this is one of those "probably not strictly necessary but it makes things tidier just in case" changes, but I recommend adding the following hunk to patch 0005:

--- a/master/datagram.c
+++ b/master/datagram.c
@@ -586,6 +586,9 @@
         case EC_DATAGRAM_ERROR:
             printk("error");
             break;
+        case EC_DATAGRAM_INVALID:
+            printk("invalid");
+            break;
         default:
             printk("???");
     }
[Knud] Agree - I have added the hunk into the patch.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches-knud-02032015.zip
Type: application/x-zip-compressed
Size: 57073 bytes
Desc: patches-knud-02032015.zip
URL: <http://lists.etherlab.org/pipermail/etherlab-dev/attachments/20150302/1092f1d5/attachment-0001.bin>