Use IPM for cdc-acm #552

bigdinotech · 2017-05-18T23:17:09Z

-use the IPM instead of shared memory for passing data between ARC
and x86 cores for cdc-acm.

bigdinotech · 2017-05-18T23:18:01Z

eriknyquist · 2017-05-18T23:28:17Z

cores/arduino/CDCSerialClass.cpp

@@ -73,7 +100,9 @@ void CDCSerialClass::init(const uint32_t dwBaudRate, const uint8_t modeReg)
     * Empty the Rx buffer but don't touch Tx buffer: it is drained by the
     * LMT one way or another */
    _rx_buffer->tail = _rx_buffer->head;
-
+
+    mailbox_register(6, cdc_mbox_isr);


Channel 6 is exposed by the CurieMailbox library. If you use this channel, the Mailbox library and Serial will not work simultaneously.

Can we use channel 7 for both RX and TX?

From my initial tests using channel 7 for both was not stable. It freezes when we are sending and receiving on both cores at a high rate

Throughput would be vastly improved if we fill all 16 bytes of the mailbox channel, instead of sending 1 message per character. Let's focus on that first, and then see if it's stable enough to use one channel for RX/TX

eriknyquist · 2017-05-18T23:34:54Z

cores/arduino/CDCSerialClass.cpp

+        {
+            if(((uint8_t)dword) != '\0')
+            {
+                new_head = (Serial._rx_buffer->head +1) % CDCACM_BUFFER_SIZE;


Surely there's some kind of 'ringbuffer_add' method? we should not be re-implementing this logic for each write to the buffer.

there is none implemented for the cdc-acm ring buffer in shared memory

let's implement one, then.

eriknyquist · 2017-05-18T23:38:40Z

cores/arduino/CDCSerialClass.cpp

@@ -36,6 +36,33 @@ extern void CDCSerial_Handler(void);
 extern void serialEventRun1(void) __attribute__((weak));
 extern void serialEvent1(void) __attribute__((weak));

+static void cdc_mbox_isr(CurieMailboxMsg msg)


This function looks really complex for such a simple task. Can't you just do something like this;

unsigned char *ptr = (unsigned char*) msg.data; /* Continue until 16 chars, or until NULL is seen */ for (unsigned i = 0; i < 16 && ptr[i]; ++i) { /* Assuming we have some kind of 'put' method for ring buffer... */ ringbuffer_put(ptr[i]); }

eriknyquist · 2017-05-18T23:39:58Z

cores/arduino/CDCSerialClass.cpp

-    uint32_t retries = 2;
-
+    CurieMailboxMsg cdcacm_msg;
+    


trailing whitespace

eriknyquist · 2017-05-18T23:42:38Z

cores/arduino/CDCSerialClass.cpp

-    } while (retries--);
-
+    cdcacm_msg.channel = 7;
+    cdcacm_msg.data[0] = (uint32_t)uc_data;


Why the cast to uint32_t? not needed at all. remove please

eriknyquist · 2017-05-18T23:43:28Z

cores/arduino/CDCSerialClass.cpp

-        }
-    } while (retries--);
-
+    cdcacm_msg.channel = 7;


Give it a name. You know the drill, by now I hope.....

eriknyquist · 2017-05-22T17:53:47Z

cores/arduino/CDCSerialClass.cpp

@@ -73,7 +100,9 @@ void CDCSerialClass::init(const uint32_t dwBaudRate, const uint8_t modeReg)
     * Empty the Rx buffer but don't touch Tx buffer: it is drained by the
     * LMT one way or another */
    _rx_buffer->tail = _rx_buffer->head;
-
+
+    mailbox_register(6, cdc_mbox_isr);


Throughput would be vastly improved if we fill all 16 bytes of the mailbox channel, instead of sending 1 message per character. Let's focus on that first, and then see if it's stable enough to use one channel for RX/TX

eriknyquist

Still un-addressed comments in the first revision, and some new things to be addressed in the most recent revision.

eriknyquist · 2017-06-05T17:49:40Z

cores/arduino/CDCSerialClass.cpp


+size_t CDCSerialClass::write(const uint8_t *buffer, size_t size)


This function is super complicated.... again the nested for-loops and bit shifting is unnecessary. Also, you forgot to return the number of bytes written at the end.

Here's a different version of this same function, which is much shorter (about 50% of it is comments anyway) and easier to read, only needs a single loop, and handles the missing return as well...

/* Size in bytes of a mailbox message payload */ #define MBOX_BYTES (CHANNEL_DATA_WORDS * sizeof(uint32_t)) ... size_t CDCSerialClass::write(const uint8_t *buffer, size_t size) { CurieMailboxMsg cdcacm_msg; unsigned int i, step, remainder; cdcacm_msg.channel = CDC_MAILBOX_TX_CHANNEL; if (!_shared_data->device_open || !_shared_data->host_open) return(0); do { /* Distance to next 16-byte boundary */ remainder = i % MBOX_BYTES; if (i > 0 && (remainder == 0 || !buffer[i] || i == size) { /* Either we are copying a full 16 bytes (standard condition), or from * the current position to the last 16-byte boundary (end-of-input condition)*/ step = (buffer[i] || i == size) ? MBOX_BYTES : remainder; /* Copy data into mailbox message */ memset(cdcacm_msg.data, 0, sizeof(cdcacm_msg.data)); memcpy(cdcacm_msg.data, buffer + (i - step), step); /* Write to mailbox*/ mailbox_write(cdcacm_msg); } /* Continue until a null character, or max. write size, is reached*/ } while (buffer[i] && size > i++); /* Return no. of bytes written */ return i; }

eriknyquist · 2017-06-05T17:49:42Z

cores/arduino/CDCSerialClass.cpp

+    if (!_shared_data->device_open || !_shared_data->host_open)
+        return(0);
+
+    int msg_len = size;


msg_len never changes, why is it needed at all? just use size directly.

-use the IPM instead of shared memory for passing data between ARC and x86 cores for cdc-acm.

done

eriknyquist suggested changes May 18, 2017

View reviewed changes

eriknyquist suggested changes May 22, 2017

View reviewed changes

bigdinotech force-pushed the cdc-mbox branch from d36a0ba to 8a3521c Compare May 27, 2017 01:12

eriknyquist previously requested changes Jun 5, 2017

View reviewed changes

Use IPM for cdc-acm

ef13768

-use the IPM instead of shared memory for passing data between ARC and x86 cores for cdc-acm.

bigdinotech force-pushed the cdc-mbox branch from 8a3521c to ef13768 Compare June 5, 2017 22:37

bigdinotech merged commit 7bc16b6 into arduino:master Aug 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use IPM for cdc-acm #552

Use IPM for cdc-acm #552

bigdinotech commented May 18, 2017

bigdinotech commented May 18, 2017

eriknyquist May 18, 2017

bigdinotech May 19, 2017

eriknyquist May 22, 2017

eriknyquist May 18, 2017

bigdinotech May 22, 2017

eriknyquist May 22, 2017

eriknyquist May 18, 2017

eriknyquist May 18, 2017

eriknyquist May 18, 2017

eriknyquist May 18, 2017

eriknyquist May 22, 2017

eriknyquist left a comment

eriknyquist Jun 5, 2017 •

edited

Loading

eriknyquist Jun 5, 2017


		size_t CDCSerialClass::write(const uint8_t *buffer, size_t size)

Use IPM for cdc-acm #552

Use IPM for cdc-acm #552

Conversation

bigdinotech commented May 18, 2017

bigdinotech commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknyquist left a comment

Choose a reason for hiding this comment

eriknyquist Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknyquist Jun 5, 2017 •

edited

Loading