Redesign message handling - Prevent handling in ISR

Author: kbumsik

cores/arduino/VirtIOSerial.cpp

@@ -24,6 +24,8 @@
 #if defined (VIRTIOCON)
 
 #include "VirtIOSerial.h"
+#include "core_debug.h"
+
 #if !defined(VIRTIOSERIAL_NUM)
 #define VIRTIOSERIAL_NUM    1
 #endif
@@ -62,6 +64,9 @@ void VirtIOSerial::begin(void)
   VirtIOSerial_Handle[VirtIOSerial_index] = &_VirtIOSerialObj;
   _VirtIOSerialObj.initialized = true;
   _VirtIOSerialObj.first_message_discarded = false;
+
+  // This will wait for the first message "DUMMY", see rxCallback().
+  OPENAMP_Wait_EndPointready(&_VirtIOSerialObj.handle.ept);
 }
 
 void VirtIOSerial::begin(uint32_t /* baud_count */)
@@ -85,18 +90,21 @@ void VirtIOSerial::end()
 
 int VirtIOSerial::available(void)
 {
+  checkMessageFromISR();
   return virtio_buffer_read_available(&_VirtIOSerialObj.ring);
 }
 
 int VirtIOSerial::availableForWrite()
 {
+  checkMessageFromISR();
   // Just return max length of VIRT_UART_Transmit() can transmit.
   // See VIRT_UART_Transmit().
-  return RPMSG_BUFFER_SIZE - RPMSG_VRING_HEADER_SIZE;
+  return RPMSG_VRING_PAYLOAD_SIZE;
 }
 
 int VirtIOSerial::peek(void)
 {
+  checkMessageFromISR();
   if (virtio_buffer_read_available(&_VirtIOSerialObj.ring) > 0) {
     uint8_t tmp;
     virtio_buffer_peek(&_VirtIOSerialObj.ring, &tmp, 1);
@@ -119,14 +127,11 @@ int VirtIOSerial::read(void)
 
 size_t VirtIOSerial::readBytes(char *buffer, size_t length)
 {
+  checkMessageFromISR();
   uint16_t prev_write_available = virtio_buffer_write_available(&_VirtIOSerialObj.ring);
   const size_t size = virtio_buffer_read(&_VirtIOSerialObj.ring, reinterpret_cast<uint8_t *>(buffer), length);
-
-  if (prev_write_available < RPMSG_VRING_TOTAL_PAYLOAD_SIZE
-      && virtio_buffer_write_available(&_VirtIOSerialObj.ring) >= RPMSG_VRING_TOTAL_PAYLOAD_SIZE) {
-    MAILBOX_Notify_Rx_Buf_Free();
-  }
-
+  // The ring buffer might be available enough to write after reading
+  checkMessageFromISR();
   return size;
 }
 
@@ -137,23 +142,43 @@ size_t VirtIOSerial::write(uint8_t ch)
 }
 
 // Warning: Currently VirtIOSerial implementation is synchronous, blocking
-// until all bytes are sent.
+// until all bytes are sent. But it will be fast enough.
 size_t VirtIOSerial::write(const uint8_t *buffer, size_t size)
 {
-  if (VIRT_UART_Transmit(&_VirtIOSerialObj.handle, const_cast<uint8_t *>(buffer), size) == VIRT_UART_ERROR) {
-    // This error usually happens when rpmsg is not ready for
+  checkMessageFromISR();
+  if (VIRT_UART_Transmit(&_VirtIOSerialObj.handle, const_cast<uint8_t *>(buffer), size) != VIRT_UART_OK) {
+    // This error usually happens when size > 496. See VirtIOSerial::availableForWrite()
+    core_debug("ERROR: VirtIOSerial::write() failed. Check availableForWrite().\n");
     return 0;
   }
+  // It is likely receive "buf free" from the Linux host right after
+  // VIRT_UART_Transmit(). So check it here too.
+  checkMessageFromISR();
   return size;
 }
 
 void VirtIOSerial::flush(void)
 {
+  checkMessageFromISR();
   // write() is blocked until all bytes are sent. So flush() doesn't need to do
   // anything. See rpmsg_send().
   return;
 }
 
+/**
+ * @brief Check if RPMsg message arrived from IPCC ISR
+ * @note  This should called as mush as possible to consume RPMsg ring buffers,
+ *        so that the host processor can send more messages quickly.
+ */
+void VirtIOSerial::checkMessageFromISR(void)
+{
+  OPENAMP_check_for_tx_message();
+  if (virtio_buffer_write_available(&_VirtIOSerialObj.ring) >= RPMSG_VRING_TOTAL_PAYLOAD_SIZE) {
+    // This calls rxCallback() VRING_NUM_BUFFS times at maximum
+    OPENAMP_check_for_rx_message();
+  }
+}
+
 void VirtIOSerial::rxGenericCallback(VIRT_UART_HandleTypeDef *huart)
 {
   VirtIOSerialObj_t *obj = get_VirtIOSerial_obj(huart);
@@ -180,9 +205,6 @@ void VirtIOSerial::rxCallback(VIRT_UART_HandleTypeDef *huart)
   while (size > 0) {
     size -= virtio_buffer_write(&_VirtIOSerialObj.ring, huart->pRxBuffPtr, size);
   }
-  if (virtio_buffer_write_available(&_VirtIOSerialObj.ring) >= RPMSG_VRING_TOTAL_PAYLOAD_SIZE) {
-    MAILBOX_Notify_Rx_Buf_Free();
-  }
 }
 
 /* Aim of the function is to get _VirtIOSerialObj pointer using huart pointer */

cores/arduino/VirtIOSerial.h

@@ -73,6 +73,7 @@ class VirtIOSerial : public Stream {
   private:
     static uint32_t VirtIOSerial_index;
     VirtIOSerialObj_t _VirtIOSerialObj;
+    void checkMessageFromISR(void);
 };
 
 extern VirtIOSerial SerialVirtIO;

cores/arduino/stm32/OpenAMP/mbox_ipcc.c

@@ -36,23 +36,24 @@
 #include "openamp/open_amp.h"
 #include "stm32_def.h"
 #include "openamp_conf.h"
+#include "core_debug.h"
 
 /* Private define ------------------------------------------------------------*/
 #define MASTER_CPU_ID       0
 #define REMOTE_CPU_ID       1
 #define IPCC_CPU_A7         MASTER_CPU_ID
 #define IPCC_CPU_M4         REMOTE_CPU_ID
 
-#define RX_NO_MSG           0
-#define RX_NEW_MSG          1
-#define RX_BUF_FREE         2
+typedef enum {
+  RX_NO_MSG = 0,
+  RX_NEW_MSG = 1,
+  RX_BUF_FREE = 2
+} rx_status_t;
 
 /* Private variables ---------------------------------------------------------*/
 IPCC_HandleTypeDef hipcc;
-extern struct rpmsg_virtio_device rvdev;
-
-
-
+rx_status_t msg_received_ch1 = RX_NO_MSG;
+rx_status_t msg_received_ch2 = RX_NO_MSG;
 
 /* Private function prototypes -----------------------------------------------*/
 void IPCC_channel1_callback(IPCC_HandleTypeDef *hipcc, uint32_t ChannelIndex, IPCC_CHANNELDirTypeDef ChannelDir);
@@ -68,6 +69,7 @@ int MAILBOX_Init(void)
   __HAL_RCC_IPCC_CLK_ENABLE();
   HAL_NVIC_SetPriority(IPCC_RX1_IRQn, IPCC_IRQ_PRIO, IPCC_IRQ_SUBPRIO);
   HAL_NVIC_EnableIRQ(IPCC_RX1_IRQn);
+
   hipcc.Instance = IPCC;
   if (HAL_IPCC_Init(&hipcc) != HAL_OK) {
     Error_Handler();
@@ -89,19 +91,61 @@ int MAILBOX_Init(void)
 }
 
 /**
-  * @brief  Callback function called by OpenAMP MW to notify message processing
-  * @param  VRING id
+ * @brief  Process vring messages received from IPCC ISR
+ * @param vdev: virtio device
+ * @param vring_id: Vring ID
+ * @retval : Operation result
+ */
+int MAILBOX_Poll(struct virtio_device *vdev, uint32_t vring_id)
+{
+  int ret = -1;
+
+  switch (vring_id) {
+    case VRING0_ID:
+      if (msg_received_ch1 == RX_BUF_FREE) {
+        /* This calls rpmsg_virtio_tx_callback(), which actually does nothing. */
+        rproc_virtio_notified(vdev, VRING0_ID);
+        msg_received_ch1 = RX_NO_MSG;
+        ret = 0;
+      }
+      break;
+    case VRING1_ID:
+      if (msg_received_ch2 == RX_NEW_MSG) {
+        /**
+         * This calls rpmsg_virtio_rx_callback(), which calls virt_uart rx callback
+         * RING_NUM_BUFFS times at maximum.
+         */
+        rproc_virtio_notified(vdev, VRING1_ID);
+        msg_received_ch2 = RX_NO_MSG;
+
+        /* The OpenAMP framework does not notify for free buf: do it here */
+        rproc_virtio_notified(NULL, VRING1_ID);
+        ret = 0;
+      }
+      break;
+    default:
+      break;
+  }
+
+  return ret;
+}
+
+/**
+  * @brief  Callback function called by OpenAMP MW to notify message processing (aka. kick)
+  * @note   This callback is called by virtqueue_kick() in rpmsg_virtio_send_offchannel_raw().
+  *         Therefore, it is only called while tx, but not rx.
+  * @param  Vring id
   * @retval Operation result
   */
-int MAILBOX_Notify(void *priv, uint32_t id)
+int MAILBOX_Notify(void *priv, uint32_t vring_id)
 {
   uint32_t channel;
   (void)priv;
 
   /* Called after virtqueue processing: time to inform the remote */
-  if (id == VRING0_ID) {
+  if (vring_id == VRING0_ID) {
     channel = IPCC_CHANNEL_1;
-  } else if (id == VRING1_ID) {
+  } else if (vring_id == VRING1_ID) {
     /* Note: the OpenAMP framework never notifies this */
     channel = IPCC_CHANNEL_2;
     return -1;
@@ -121,37 +165,25 @@ int MAILBOX_Notify(void *priv, uint32_t id)
   return 0;
 }
 
-/**
-  * @brief  Notify Rx buffer is free to Master
-  */
-void MAILBOX_Notify_Rx_Buf_Free()
-{
-  HAL_IPCC_NotifyCPU(&hipcc, IPCC_CHANNEL_2, IPCC_CHANNEL_DIR_RX);
-}
-
 /* Private function  ---------------------------------------------------------*/
 /* Callback from IPCC Interrupt Handler: Master Processor informs that there are some free buffers */
 void IPCC_channel1_callback(IPCC_HandleTypeDef *hipcc,
                             uint32_t ChannelIndex, IPCC_CHANNELDirTypeDef ChannelDir)
 {
+  msg_received_ch1 = RX_BUF_FREE;
+
   /* Inform A7 that we have received the 'buff free' msg */
   HAL_IPCC_NotifyCPU(hipcc, ChannelIndex, IPCC_CHANNEL_DIR_RX);
-  rproc_virtio_notified(rvdev.vdev, VRING0_ID);
 }
 
 /* Callback from IPCC Interrupt Handler: new message received from Master Processor */
 void IPCC_channel2_callback(IPCC_HandleTypeDef *hipcc,
                             uint32_t ChannelIndex, IPCC_CHANNELDirTypeDef ChannelDir)
 {
-  (void) hipcc;
-  (void) ChannelIndex;
-  (void) ChannelDir;
-  /* Don't inform A7 here, do it when the buffer has more than RPMSG_BUFFER_SIZE.
-   * See MAILBOX_Notify_Rx_Buf_Free() and VirIOSerial.cpp.
-   */
-  rproc_virtio_notified(rvdev.vdev, VRING1_ID);
-  /* The OpenAMP framework does not notify for free buf: do it here */
-  rproc_virtio_notified(NULL, VRING1_ID);
+  msg_received_ch2 = RX_NEW_MSG;
+
+  /* Don't inform A7 here */
+  HAL_IPCC_NotifyCPU(hipcc, ChannelIndex, IPCC_CHANNEL_DIR_RX);
 }
 
 /**

cores/arduino/stm32/OpenAMP/mbox_ipcc.h

@@ -30,13 +30,9 @@
 #define IPCC_IRQ_SUBPRIO    0
 #endif
 
-/* Includes ------------------------------------------------------------------*/
-/* Exported types ------------------------------------------------------------*/
-/* Exported constants --------------------------------------------------------*/
-/* Exported functions ------------------------------------------------------- */
 int MAILBOX_Init(void);
+int MAILBOX_Poll(struct virtio_device *vdev, uint32_t vring_id);
 int MAILBOX_Notify(void *priv, uint32_t id);
-void MAILBOX_Notify_Rx_Buf_Free(void);
 
 #endif /* VIRTIOCON */
 #endif /* MBOX_IPCC_H_ */

cores/arduino/stm32/OpenAMP/openamp.c

@@ -161,9 +161,39 @@ int OPENAMP_create_endpoint(struct rpmsg_endpoint *ept, const char *name,
                           unbind_cb);
 }
 
+/**
+ * @brief Check and process any received messages from tx channel
+ * @note  Don't call this in an ISR.
+ */
+void OPENAMP_check_for_tx_message(void)
+{
+  MAILBOX_Poll(rvdev.vdev, VRING0_ID);
+}
+
+/**
+ * @brief Check and process any received messages from rx channel
+ * @note  Don't call this in an ISR.
+ */
+void OPENAMP_check_for_rx_message(void)
+{
+  MAILBOX_Poll(rvdev.vdev, VRING1_ID);
+}
+
+/**
+ * @brief Wait loop on rpmsg endpoint (VirtIOSerial) ready to send a message.
+ *        (until message dest address is known)
+ * @note  This will wait until the first message arrives from the Linux host.
+ *        If we send a rpmsg message before this it will fail.
+ * @note  Don't call this in an ISR.
+ * @see   VirtIOSerial::rxCallback
+ * @param rp_ept: virtio rpmsg endpoint
+ */
 void OPENAMP_Wait_EndPointready(struct rpmsg_endpoint *rp_ept)
 {
-  while (!is_rpmsg_ept_ready(rp_ept));
+  while (!is_rpmsg_ept_ready(rp_ept)) {
+    MAILBOX_Poll(rvdev.vdev, VRING0_ID);
+    MAILBOX_Poll(rvdev.vdev, VRING1_ID);
+  }
 }
 
 #endif /* VIRTIOCON */

cores/arduino/stm32/OpenAMP/openamp.h

@@ -48,6 +48,9 @@ int OPENAMP_create_endpoint(struct rpmsg_endpoint *ept, const char *name,
                             uint32_t dest, rpmsg_ept_cb cb,
                             rpmsg_ns_unbind_cb unbind_cb);
 
+/* Check for new rpmsg reception */
+void OPENAMP_check_for_message(void);
+
 /* Wait loop on endpoint ready ( message dest address is know)*/
 void OPENAMP_Wait_EndPointready(struct rpmsg_endpoint *rp_ept);