You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/01/02 15:05:22 UTC

[GitHub] [incubator-nuttx] MTres19 opened a new pull request #2641: can: WIP: Cleanup usage of soft fifo semaphore

MTres19 opened a new pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641


   ## Summary
   
   I got caught by the fact that CAN messages [are packed](https://nuttx.apache.org/docs/latest/components/drivers/character/can.html?highlight=usage%20note) into the buffer passed to read() and thought there was a problem in can_poll(), so I started browsing through the code. Functionally there's nothing _wrong_ with the current code, but I thought it was strange that the poll setup could in theory block on the RX fifo semaphore rather than the designated poll semaphore. This would only happen if there was a read() call blocked in another thread, which I admit is an unusual use-case.
   
   If this looks acceptable I can try to clean up the TX code as well.
   
   ## Impact
   No impact expected.
   
   ## Testing
   Tested with a TM4C123GXL Launchpad using my [WIP driver](https://github.com/MTres19/incubator-nuttx/tree/MTres19/tiva-can). [Test application](https://github.com/MTres19/ETCetera-tools/blob/main/cantest_main.c#L352).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on a change in pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on a change in pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#discussion_r774280728



##########
File path: drivers/can/can.c
##########
@@ -609,24 +609,24 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
 
       fifo = &reader->fifo;
 
-      while (fifo->rx_head == fifo->rx_tail)
+      if (filep->f_oflags & O_NONBLOCK)
+        {
+          ret = nxsem_trywait(&fifo->rx_sem);

Review comment:
       +1 for removing the unnecessary wrappers if no one's opposed.




-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641


   


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-997644888


   Thanks @xiaoxiang781216 Should be good now


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on a change in pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on a change in pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#discussion_r774280528



##########
File path: include/nuttx/can/can.h
##########
@@ -491,7 +491,13 @@ begin_packed_struct struct can_msg_s
 
 struct can_rxfifo_s
 {
-  sem_t         rx_sem;                  /* Counting semaphore */
+  /* Binary semaphore. Indicates whether FIFO is available for reading
+   * AND not empty. Only take this sem inside a critical section to guarantee
+   * exclusive access to both the semaphore and the head/tail FIFO indices.
+   */
+
+  sem_t         rx_sem;

Review comment:
       No; `rx_sem` is not used as a lock because the ISR (which calls `can_receive()`) can edit the FIFO whenever it wants as long as interrupts are enabled. `rx_sem` exists to allow `can_receive()` to wake up a reader thread, but the reader thread then has exclusive access only because it uses a critical section, not because it took the semaphore.
   
   See also my reply above.
   
   Actually, `rx_sem` was a binary semaphore before too, but "0" and "1" didn't have any defined meaning because it was initialized to 1 and then immediately decremented in a loop in `can_read()` and `can_poll()`




-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on pull request #2641: can: WIP: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-762585267


   @acassis Feedback/suggestions? Thinking about the TX side, I wonder whether memcpy-ing everything into a FIFO is really necessary. I know the application passes a packed array but that's not really an issue since the upper-half driver can just point to a particular offset when calling `dev_send()`...


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] acassis commented on pull request #2641: can: WIP: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-756063879


   Hi @MTres19 could you please supply a sample code where the issue with current code will happen? @raiden00pl are you around to help us to review?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on a change in pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on a change in pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#discussion_r774280480



##########
File path: drivers/can/can.c
##########
@@ -609,24 +609,24 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
 
       fifo = &reader->fifo;
 
-      while (fifo->rx_head == fifo->rx_tail)
+      if (filep->f_oflags & O_NONBLOCK)

Review comment:
       Good point; I did it this way so that `rx_sem` would have clearly defined semantics; i.e. 1 = ready to read, 0 = not ready to read. Right now, if I only replaced the `nxsem_trywait()` here, a subsequent call to `can_read()` would return the error "RX FIFO sem posted but FIFO is empty."
   
   Obviously those semantics duplicate the information you get from checking if head == tail. It's just what I thought of at the time. Alternatively, we could only post the semaphore in `can_receive()` if at least one thread is waiting on it (i.e. sem_get_value yields a number < 0). Then `rx_sem` would truly be used exclusively for waiting and not duplicate any information. Maybe that would be better? I'd need to put my CAN setup back together of course to test it, in that case.




-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-997320430


   Hey @acassis I'm going to be getting back into tinkering with NuttX soon; could I get another set of eyes on this or do you think my testing 11 months ago would be enough?


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] MTres19 commented on pull request #2641: can: WIP: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
MTres19 commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-757415989


   Hi @acassis sorry for the delay but [here](https://github.com/MTres19/ETCetera-tools/blob/dab39c7981c71715266e2fc69cc30c7aea86ef10/cantest_main.c#L1105) you go. In the current state, the main thread is unable to set up a poll on stdin because the poll setup for the CAN device file is blocked.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] raiden00pl commented on pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
raiden00pl commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-998587989


   I don't have any CAN setup around at this moment to test this change. But looking only for the code, it looks good.


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#discussion_r773694946



##########
File path: drivers/can/can.c
##########
@@ -609,24 +609,24 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
 
       fifo = &reader->fifo;
 
-      while (fifo->rx_head == fifo->rx_tail)
+      if (filep->f_oflags & O_NONBLOCK)
+        {
+          ret = nxsem_trywait(&fifo->rx_sem);

Review comment:
       should we add can_trysem or remove the null wrapper of can_takesem and can_givesem?

##########
File path: drivers/can/can.c
##########
@@ -609,24 +609,24 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
 
       fifo = &reader->fifo;
 
-      while (fifo->rx_head == fifo->rx_tail)
+      if (filep->f_oflags & O_NONBLOCK)

Review comment:
       the critical section is held at line 568, why not directly check the queue empty directly instead calling nxsem_trywait?

##########
File path: include/nuttx/can/can.h
##########
@@ -491,7 +491,13 @@ begin_packed_struct struct can_msg_s
 
 struct can_rxfifo_s
 {
-  sem_t         rx_sem;                  /* Counting semaphore */
+  /* Binary semaphore. Indicates whether FIFO is available for reading
+   * AND not empty. Only take this sem inside a critical section to guarantee
+   * exclusive access to both the semaphore and the head/tail FIFO indices.
+   */
+
+  sem_t         rx_sem;

Review comment:
       but, the code still use it as the lock, not an indicator?




-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-997774086


   @acassis or @raiden00pl could you review the change? I am not familiar with CAN protocol.:(


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2641: can: Cleanup usage of soft fifo semaphore

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#issuecomment-997542559


   @MTres19 could you fix the style warning?


-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org