can: fix RTR ioctl and support timeout

drivers/can/can.c

@@ -49,6 +49,7 @@
 #include <stdbool.h>
 #include <unistd.h>
 #include <string.h>
+#include <time.h>
 #include <fcntl.h>
 #include <assert.h>
 #include <poll.h>
@@ -56,6 +57,7 @@
 #include <debug.h>
 
 #include <nuttx/arch.h>
+#include <nuttx/clock.h>
 #include <nuttx/signal.h>
 #include <nuttx/fs/fs.h>
 #include <nuttx/can/can.h>
@@ -141,7 +143,7 @@ static ssize_t        can_read(FAR struct file *filep, FAR char *buffer,
 static int            can_xmit(FAR struct can_dev_s *dev);
 static ssize_t        can_write(FAR struct file *filep,
                                 FAR const char *buffer, size_t buflen);
-static inline ssize_t can_rtrread(FAR struct can_dev_s *dev,
+static inline ssize_t can_rtrread(FAR struct file *filep,
                                   FAR struct canioc_rtr_s *rtr);
 static int            can_ioctl(FAR struct file *filep, int cmd,
                                 unsigned long arg);
@@ -916,12 +918,15 @@ return_with_irqdisabled:
  *
  ****************************************************************************/
 
-static inline ssize_t can_rtrread(FAR struct can_dev_s *dev,
+static inline ssize_t can_rtrread(FAR struct file *filep,
-                                  FAR struct canioc_rtr_s *rtr)
+                                  FAR struct canioc_rtr_s *request)
 {
+  FAR struct can_dev_s *dev = filep->f_inode->i_private;
   FAR struct can_rtrwait_s *wait = NULL;
+  struct timespec           abstimeout;
   irqstate_t                flags;
   int                       i;
+  int                       sval;
   int                       ret = -ENOMEM;
 
   /* Disable interrupts through this operation */
@@ -933,26 +938,79 @@ static inline ssize_t can_rtrread(FAR struct can_dev_s *dev,
   for (i = 0; i < CONFIG_CAN_NPENDINGRTR; i++)
     {
       FAR struct can_rtrwait_s *tmp = &dev->cd_rtr[i];
-      if (!rtr->ci_msg)
+
+      ret = nxsem_get_value(&tmp->cr_sem, &sval);
+
+      if (ret < 0)
         {
-          tmp->cr_id  = rtr->ci_id;
+          continue;
-          tmp->cr_msg = rtr->ci_msg;
+        }
+
+      if (sval == 0)
+        {
+          /* No one is waiting on RTR transaction; take it. */
+
+          tmp->cr_msg     = request->ci_msg;
           dev->cd_npendrtr++;
-          wait        = tmp;
+
+          wait            = tmp;
           break;
         }
     }
 
   if (wait)
     {
-      /* Send the remote transmission request */
+      /* Send the remote transmission request with the "old method" unless
+       * the lower-half driver indicates otherwise.
+       */
+
+      if (dev->cd_ops->co_remoterequest != NULL)
+        {
+          if (request->ci_msg->cm_hdr.ch_id < CAN_MAX_STDMSGID
+#ifdef CONFIG_CAN_EXTID
+              && !request->ci_msg->cm_hdr.ch_extid
+#endif
+            )
+            {
+              ret = dev_remoterequest(dev,
+                                (uint16_t)(request->ci_msg->cm_hdr.ch_id));
+            }
+          else
+            {
+              ret = -EINVAL;
+            }
+        }
+      else
+        {
+#ifdef CONFIG_CAN_USE_RTR
+          /* Temporarily set the RTR bit, then send the remote transmission
+           * request message with the lower-half driver's regular function.
+           */
+
+          request->ci_msg->cm_hdr.ch_rtr = 1;
+          ret = can_write(filep,
+                          request->ci_msg,
+                          CAN_MSGLEN(request->ci_msg->cm_hdr.ch_dlc));
+          request->ci_msg->cm_hdr.ch_rtr = 0;
+#else
+          canerr("Error: Driver needs CONFIG_CAN_USE_RTR.\n");
+          ret = -ENOSYS;
+#endif
+        }
 
-      ret = dev_remoterequest(dev, wait->cr_id);
       if (ret >= 0)
         {
           /* Then wait for the response */
 
-          ret = can_takesem(&wait->cr_sem);
+          ret = clock_gettime(CLOCK_REALTIME, &abstimeout);
+
+          if (ret >= 0)
+            {
+              clock_timespec_add(&abstimeout,
+                                 &request->ci_timeout,
+                                 &abstimeout);
+              ret = nxsem_timedwait(&wait->cr_sem, &abstimeout);
+            }
         }
     }
 
@@ -984,7 +1042,8 @@ static int can_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
        */
 
       case CANIOC_RTR:
-        ret = can_rtrread(dev, (FAR struct canioc_rtr_s *)((uintptr_t)arg));
+        ret = can_rtrread(filep,
+                          (FAR struct canioc_rtr_s *)((uintptr_t)arg));
         break;
 
       /* Not a "built-in" ioctl command.. perhaps it is unique to this
@@ -1195,7 +1254,6 @@ int can_register(FAR const char *path, FAR struct can_dev_s *dev)
 
       nxsem_init(&dev->cd_rtr[i].cr_sem, 0, 0);
       nxsem_set_protocol(&dev->cd_rtr[i].cr_sem, SEM_PRIO_NONE);
-      dev->cd_rtr[i].cr_msg = NULL;
     }
 
   /* Initialize/reset the CAN hardware */
@@ -1237,6 +1295,8 @@ int can_receive(FAR struct can_dev_s *dev, FAR struct can_hdr_s *hdr,
   int                      nexttail;
   int                      errcode = -ENOMEM;
   int                      i;
+  int                      sval;
+  int                      ret;
 
   caninfo("ID: %" PRId32 " DLC: %d\n", (uint32_t)hdr->ch_id, hdr->ch_dlc);
 
@@ -1256,35 +1316,44 @@ int can_receive(FAR struct can_dev_s *dev, FAR struct can_hdr_s *hdr,
 
       for (i = 0; i < CONFIG_CAN_NPENDINGRTR; i++)
         {
-          FAR struct can_rtrwait_s *rtr = &dev->cd_rtr[i];
+          FAR struct can_rtrwait_s *wait = &dev->cd_rtr[i];
-          FAR struct can_msg_s     *msg = rtr->cr_msg;
+          FAR struct can_msg_s     *waitmsg = wait->cr_msg;
 
-          /* Check if the entry is valid and if the ID matches.  A valid
+          /* Check if the entry is in use and whether the ID matches */
-           * entry has a non-NULL receiving address
-           */
 
-          if (msg && hdr->ch_id == rtr->cr_id)
+          ret = nxsem_get_value(&wait->cr_sem, &sval);
+
+          if (ret < 0)
+            {
+              continue;
+            }
+
+          else if (sval < 0
+#ifdef CONFIG_CAN_ERRORS
+                && hdr->ch_error == false
+#endif
+#ifdef CONFIG_CAN_EXTID
+                && waitmsg->cm_hdr.ch_extid == hdr->ch_extid
+#endif
+                && waitmsg->cm_hdr.ch_id == hdr->ch_id)
             {
               int nbytes;
 
               /* We have the response... copy the data to the user's buffer */
 
-              memcpy(&msg->cm_hdr, hdr, sizeof(struct can_hdr_s));
+              memcpy(&waitmsg->cm_hdr, hdr, sizeof(struct can_hdr_s));
 
               nbytes = can_dlc2bytes(hdr->ch_dlc);
-              for (i = 0, dest = msg->cm_data; i < nbytes; i++)
+              for (i = 0, dest = waitmsg->cm_data; i < nbytes; i++)
                 {
                   *dest++ = *data++;
                 }
 
-              /* Mark the entry unused */
-
-              rtr->cr_msg = NULL;
               dev->cd_npendrtr--;
 
-              /* And restart the waiting thread */
+              /* Restart the waiting thread and mark the entry unused */
 
-              can_givesem(&rtr->cr_sem);
+              can_givesem(&wait->cr_sem);
             }
         }
     }
@@ -1305,7 +1374,6 @@ int can_receive(FAR struct can_dev_s *dev, FAR struct can_hdr_s *hdr,
       if (nexttail != fifo->rx_head)
         {
           int nbytes;
-          int sval;
 
           /* Add the new, decoded CAN message at the tail of the FIFO.
            *

include/nuttx/can/can.h

@@ -47,6 +47,7 @@
 #include <sys/types.h>
 #include <stdint.h>
 #include <stdbool.h>
+#include <time.h>
 
 #include <nuttx/list.h>
 #include <nuttx/fs/fs.h>
@@ -113,8 +114,23 @@
 /* Ioctl commands supported by the upper half CAN driver.
  *
  * CANIOC_RTR:
- *   Description:  Send the remote transmission request and wait for the response.
+ *   Description:    Send the given message as a remote request. On sucessful
- *   Argument:     A reference to struct canioc_rtr_s
+ *                   return, the passed message structure is updated with
+ *                   the contents of the received message; i.e. the message
+ *                   ID and the standard/extended ID indication bit stay the
+ *                   same, but the DLC and data bits are updated with the
+ *                   contents of the received message. If no response is
+ *                   received after the specified timeout, ioctl will return.
+ *
+ *                   Note: Lower-half drivers that do not implement
+ *                         CONFIG_CAN_USE_RTR and implement co_remoterequest
+ *                         will result in EINVAL if this ioctl is called
+ *                         with an extended-ID message.
+ *
+ *   Argument:       A pointer to struct canioc_rtr_s
+ *   Returned Value: Zero (OK) is returned on success. Otherwise, -1 (ERROR)
+ *                   is returned with the errno variable set to indicate the
+ *                   nature of the error (for example, ETIMEDOUT)
  *
  * Ioctl commands that may or may not be supported by the lower half CAN driver.
  *
@@ -502,8 +518,7 @@ struct can_txfifo_s
 
 struct can_rtrwait_s
 {
-  sem_t         cr_sem;                  /* Wait for RTR response */
+  sem_t         cr_sem;                  /* Wait for response/is the cd_rtr entry available */
-  uint16_t      cr_id;                   /* The ID that is waited for */
   FAR struct can_msg_s *cr_msg;          /* This is where the RTR response goes */
 };
 
@@ -547,7 +562,13 @@ struct can_ops_s
 
   CODE int (*co_ioctl)(FAR struct can_dev_s *dev, int cmd, unsigned long arg);
 
-  /* Send a remote request */
+  /* Send a remote request. Lower-half drivers should NOT implement this if
+   * they support sending RTR messages with the regular send function
+   * (i.e. CONFIG_CAN_USE_RTR). Instead, they should mention CAN_USE_RTR
+   * in their Kconfig help and set this to NULL to indicate that the normal
+   * send function should be used instead. Lower-half drivers must implement
+   * either this or CONFIG_CAN_USE_RTR to support CANIOC_RTR.
+   */
 
   CODE int (*co_remoterequest)(FAR struct can_dev_s *dev, uint16_t id);
 
@@ -611,8 +632,21 @@ struct can_dev_s
 
 struct canioc_rtr_s
 {
-  uint16_t              ci_id;           /* The 11-bit ID to use in the RTR message */
+  /* How long to wait for the response */
-  FAR struct can_msg_s *ci_msg;          /* The location to return the RTR response */
+
+  struct timespec       ci_timeout;
+
+  /* The location to return the RTR response. The arbitration fields
+   * (i.e. message ID and extended ID indication, if applicable) should be
+   * set to the values the driver will watch for. On return from the ioctl,
+   * the DLC and data fields will be updated by the received message.
+   *
+   * The block of memory must be large enough to hold an message of size
+   * CAN_MSGLEN(CAN_MAXDATALEN) even if a smaller DLC is requested, since
+   * the response DLC may not match the requested one.
+   */
+
+  FAR struct can_msg_s *ci_msg;
 };
 
 /* CANIOC_GET_BITTIMING/CANIOC_SET_BITTIMING: