michallenc commented on code in PR #17694:
URL: https://github.com/apache/nuttx/pull/17694#discussion_r2648599793
##########
drivers/can/Kconfig:
##########
@@ -134,6 +134,15 @@ config CAN_TXPRIORITY
---help---
Prioritize sending based on canid.
+config CAN_TXCANCEL
+ bool "Implement tx cancel ability"
+ default n
+ depends on CAN_TXPRIORITY
Review Comment:
Do these have to be separate configuration options? I would say strict TX
priority ordering is useless without the ability to abort the frame from
hardware buffer.
##########
drivers/can/can.c:
##########
@@ -595,6 +595,25 @@ static int can_xmit(FAR struct can_dev_s *dev)
}
}
+#ifdef CONFIG_CAN_TXCANCEL
+ if (TX_PENDING(&dev->cd_sender) && can_txneed_cancel(&dev->cd_sender))
+ {
+ if (can_cancel_mbmsg(dev))
Review Comment:
Shouldn't this be in a while loop? What if you need to cancel more messages?
Let's say there are two CAN frames with IDs 1 and 2 already stored in HW
buffers and you want to add CAN frame with ID 0. The current design, as I
understand it, would have to abort both of these frames.
##########
drivers/can/can.c:
##########
@@ -595,6 +595,25 @@ static int can_xmit(FAR struct can_dev_s *dev)
}
}
+#ifdef CONFIG_CAN_TXCANCEL
+ if (TX_PENDING(&dev->cd_sender) && can_txneed_cancel(&dev->cd_sender))
Review Comment:
I don't think we need to cancel the messages unless hardware buffers are
actually full, we just need to reorganize the order in which they are sent to
the bus (not possible with all CAN controllers, but same can do it). If I am
not mistaken, this approach could lead to the following scenario:
- write CAN frame with ID 5 to first HW buffer
- write CAN frame with ID 0, we check for abort
- CAN frame with ID 5 is aborted and returned back to the software FIFO
- CAN frame 0 is inserted to first HW buffer
- CAN frame 5 is inserted to second HW buffer
The ideal scenario would be something like this
- write CAN frame with ID 5 to first HW buffer
- write CAN frame with ID 0 to second HW buffer
- change the order of HW buffers so the HW buffer 2 is sent before HW buffer
1
We avoid a lot of list operations and unnecessary aborts. This becomes even
more significant if more hardware buffers are used and you need to abort more
frames. Yes, the implementation is more complicated and it passes some of the
logic to the lower half, because `dev_send` has to check the priority of the
newly added frame and correctly change the order in which the buffers are sent
to the bus. Also the cancel has to be handled a bit differently, because now
it's the lower half that decides what frame is returned back to the software
FIFO.
I am not forcing the change here as this is a lot of work to do, just
passing some thoughts and ideas about possible future enhancements. We used
this approach in [RTEMS CAN
stack](https://docs.rtems.org/docs/main/bsp-howto/can.html) in [my diploma
thesis](https://wiki.control.fel.cvut.cz/mediawiki/images/c/cc/Dp_2024_lenc_michal.pdf)
actually -> multiple priority queues, lower half reorganizes the sending order
and returns the frame back to SW FIFO. This is something I'd like to see in
NuttX in the future as well. It's probably a bit different from your needs,
because you are doing strict CAN ID ordering. The problem with that is that
some protocols need to keep the sending order even for different identifiers,
that's where you need multiple priority classes... you filter ID ranges to
these classes and then ensures the highest priority class is sent first, but
the order within one class is still kept.
##########
drivers/can/can_sender.c:
##########
@@ -245,3 +245,86 @@ void can_send_done(FAR struct can_txcache_s *cd_sender)
}
#endif
}
+
+/****************************************************************************
+ * Name: can_txneed_cancel
+ *
+ * Description:
+ * Compare the msgID between tx_sending and tx_pending's head when
+ * dev_txready return false and tx_pending is not empty, preserve the node
+ * with largest msgID in tx_sending into *callbackmsg_node. return true if
+ * the msgID in tx_pending's head < the smallest msgID in tx_sending.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_CAN_TXCANCEL
+bool can_txneed_cancel(FAR struct can_txcache_s *cd_sender)
+{
+ FAR struct can_msg_node_s *msg_node;
+ FAR struct can_msg_node_s *tmp_node;
+
+ /* acquire min msgID from tx_sending and compare it with masgID
+ * in tx_pending list's head.
+ */
+
+ if (SENDING_COUNT(cd_sender) == 0)
+ {
+ return false;
+ }
+
+ msg_node = list_first_entry(&cd_sender->tx_pending,
+ struct can_msg_node_s, list);
+ tmp_node = list_first_entry(&cd_sender->tx_sending,
+ struct can_msg_node_s, list);
+ if (msg_node->msg.cm_hdr.ch_id < tmp_node->msg.cm_hdr.ch_id)
+ {
+ return true;
+ }
+
+ return false;
+}
+#endif
+
+/****************************************************************************
+ * Name: can_cancel_mbmsg
+ *
+ * Description:
+ * cancel the msg with the largest msgID in the mailbox and
+ * return true if success.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_CAN_TXCANCEL
+bool can_cancel_mbmsg(FAR struct can_dev_s *dev)
+{
+ FAR struct can_msg_node_s *tmp_node;
+ FAR struct can_msg_node_s *callbackmsg_node;
+ FAR struct can_txcache_s *cd_sender = &dev->cd_sender;
+
+ callbackmsg_node = list_last_entry(&cd_sender->tx_sending,
+ struct can_msg_node_s, list);
+ if (dev->cd_ops->co_cancel != NULL &&
+ dev_cancel(dev, &callbackmsg_node->msg))
Review Comment:
Maybe add assert above the statement ensuring the lower half actually has
`dev_cancel` to simplify subsequent debugging.
Or just return false as the lower half is not capable of aborting the frame.
I think that's even better than assertion.
##########
include/nuttx/can/can.h:
##########
@@ -815,6 +816,9 @@ struct can_ops_s
*/
CODE bool (*co_txempty)(FAR struct can_dev_s *dev);
+
+ CODE bool (*co_cancel)(FAR struct can_dev_s *dev,
Review Comment:
I think this deserves some comment in the code. I suppose cancel should
abort the `msg` that is passed as an argument, but should it also reorganize
hardware buffers sending order if they are sent to the bus in FIFO order?
##########
drivers/can/Kconfig:
##########
@@ -134,6 +134,15 @@ config CAN_TXPRIORITY
---help---
Prioritize sending based on canid.
+config CAN_TXCANCEL
+ bool "Implement tx cancel ability"
+ default n
+ depends on CAN_TXPRIORITY
+ ---help---
+ Enabling this feature adds support for the can cancel ability.
+ this ability can cancel the msg with the largest msgID in the
Review Comment:
Fix indentation
--
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]
