ppisa commented on code in PR #18039:
URL: https://github.com/apache/nuttx/pull/18039#discussion_r2708830557
##########
drivers/can/ctucanfd_pci.c:
##########
@@ -641,12 +650,24 @@ static int ctucanfd_chrdev_send(FAR struct can_dev_s *dev,
{
fmt.s.ide = 1;
id.s.id_ext = msg->cm_hdr.ch_id;
+
+ /* Set txbuf record used for txconfirm */
+
+# ifdef CONFIG_CAN_TXCONFIRM
+ priv->tx_idbuf[txidx] = CAN_EFF_FLAG | msg->cm_hdr.ch_id;
+# endif
Review Comment:
The use of `CAN_EFF_FLAG` in the character driver sections is a problem
because it is defined in `include/nuttx/can.h` but that file is not included in
character driver version which leads to
```
CC: environ/env_removevar.c can/ctucanfd_pci.c: In function
‘ctucanfd_chrdev_send’:
can/ctucanfd_pci.c:827:31: error: ‘CAN_EFF_FLAG’ undeclared (first use in
this function)
827 | priv->tx_idbuf[txidx] = CAN_EFF_FLAG | msg->cm_hdr.ch_id;
| ^~~~~~~~~~~~
can/ctucanfd_pci.c:827:31: note: each undeclared identifier is reported only
once for each function it appears in
can/ctucanfd_pci.c: In function ‘ctucanfd_chrdev_txconfirm’:
can/ctucanfd_pci.c:974:29: error: ‘CAN_EFF_FLAG’ undeclared (first use in
this function)
974 | if (priv->tx_idbuf[idx] & CAN_EFF_FLAG)
| ^~~~~~~~~~~~
```
I am not sure if it is good idea to include it.
I would suggest to use
```
struct can_hdr_s tx_idbuf[CTUCANFD_TXBUF_CNT];
```
or something similar/simplified there the character device driver. The
long term goal is to use there something like in the RTEMS driver which allows
multiple priority queues etc.
##########
drivers/can/ctucanfd_pci.c:
##########
@@ -346,6 +365,19 @@ static void ctucanfd_setup(FAR struct ctucanfd_can_s *priv)
regval |= CTUCANFD_MODE_RXBAM;
+ /* Rx Acceptance filter setting */
+
+ /* REVISIT: acceptance filter not used.
+ *
+ * This driver was verified on QEMU with virtual host CAN network,
+ * which doesn't support acceptance filter feature.
+ * For real hardware, this feature can be enabled with:
+ * " regval |= CTUCANFD_MODE_AFM; "
+ *
+ * If this feature is enabled, related registers CTUCANFD_FLTR_x_MSK/
+ * CTUCANFD_FLTR_x_VAL must also be configured !
+ */
+
Review Comment:
Unrelated to the commit description, but only comment so acceptable.
##########
drivers/can/ctucanfd_pci.c:
##########
@@ -641,12 +650,24 @@ static int ctucanfd_chrdev_send(FAR struct can_dev_s *dev,
{
fmt.s.ide = 1;
id.s.id_ext = msg->cm_hdr.ch_id;
+
+ /* Set txbuf record used for txconfirm */
+
+# ifdef CONFIG_CAN_TXCONFIRM
+ priv->tx_idbuf[txidx] = CAN_EFF_FLAG | msg->cm_hdr.ch_id;
+# endif
Review Comment:
Confirmation does not work correctly with the test, because confirmation
messages are recognized as the responses from the other side of the ping-pong
test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
