Skip to content

drivers/can: iwatermark ioctl to control multiple frame read #17283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Cynerd wants to merge 2 commits into from
Closed

drivers/can: iwatermark ioctl to control multiple frame read #17283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a391959
nuttx/can: fix two documented CANIOC names
Cynerd Nov 4, 2025
63bbe8e
nuttx/can: add read watermark
Cynerd Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions Documentation/components/drivers/character/can.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ is used to store the timestamp of the CAN message.
**Usage Note**: When reading from the CAN driver multiple messages
may be returned, depending on (1) the size the returned can
messages, and (2) the size of the buffer provided to receive CAN
messages. *Never assume that a single message will be returned*...
messages. *Do not assume that a single message will be returned*...
if you do this, *you will lose CAN data* under conditions where
your read buffer can hold more than one small message. Below is an
example about how you should think of the CAN read operation:
your read buffer can hold more than one small message. This
behavior can be controlled by using
``ioctl(fd, CANIOC_SET_IWATERMARK, &(size_t)0)`` that limits the
Copy link
Contributor

@linguini1 linguini1 Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also mention GET_IWATERMARK.

Copy link
Contributor

@acassis acassis Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cynerd maybe is better to create a subsection to ioctls and include the ioctls renaming. Also since you are renaming ioctls, should we add breaking changes in the PR?

Copy link
Contributor

@michallenc michallenc Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acassis it's true CAN documentation is trash, but I wouldn't force @Cynerd to fix it here. We should rewrite CAN docs completely to add frame structures, all ioctls and so on. Similar to ADC or PWM docs. But that's a different task to do. I've had it in my TODO list for some time, but didn't have time for it yet.

Btw, the ioctl rename is only in comment, so no breaking change if I look at it correctly.

Copy link
Contributor

@acassis acassis Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michallenc that is true, I'm not forcing him and I didn't Request a Change. But Documentation/ is something we need to worry to improve. That is something that everybody pointed in the workshop. So, I will request Documentation for all contributions that include new features or that modify existing things. I know nobody like to write documentation, but if the developer doesn't do it how will do?

Copy link
Contributor

@acassis acassis Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoxiang781216 sometime ago you told me that Xiaomi was hiring someone to help to write Documentation. Do you have some news about it?

Copy link
Contributor

@michallenc michallenc Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acassis I replied on the mailing list, just a short citation here

I think I maybe phrased my comment badly there, I am not against enhancing the documentation with new ioctl in that pull request, but I don't think we should require the committer to write the complete documentation about CAN interface (or all ioctls). It should be limited just to the newly added features and changes.

Copy link
Contributor

@acassis acassis Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @michallenc don't worry, I think you was honest in our worry about my request. But I think even if the current Documentation is not good, if we ask each contributor to improve it a little bit, we went up with a better documentation. If we wait for a good and complete Documentation it never will happen.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoxiang781216 sometime ago you told me that Xiaomi was hiring someone to help to write Documentation. Do you have some news about it?

Here is it: https://doc.openvela.com/document?id=198&version=trunk&language=en
NuttX is just part of it.

last message to be placed at most zero bytes since buffer start
(thus only one message is provided).

**Examples**: ``drivers/can/mcp2515.c``.
19 changes: 18 additions & 1 deletion drivers/can/can.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ static FAR struct can_reader_s *init_can_reader(FAR struct file *filep)
DEBUGASSERT(reader != NULL);

nxsem_init(&reader->fifo.rx_sem, 0, 0);
reader->rxwatermark = SIZE_MAX;
filep->f_priv = reader;

return reader;
Expand Down Expand Up @@ -489,7 +490,7 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
fifo->rx_head = 0;
}
}
while (fifo->rx_head != fifo->rx_tail);
while (fifo->rx_head != fifo->rx_tail && ret < reader->rxwatermark);

if (fifo->rx_head != fifo->rx_tail)
{
Expand Down Expand Up @@ -964,6 +965,22 @@ static int can_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
}
break;

/* Set read watermark */

case CANIOC_SET_IWATERMARK:
{
reader->rxwatermark = *(FAR size_t *)arg;
}
break;

/* Set read watermark */

case CANIOC_GET_IWATERMARK:
{
*(FAR size_t *)arg = reader->rxwatermark;
}
break;

/* Not a "built-in" ioctl command.. perhaps it is unique to this
* lower-half, device driver.
*/
Expand Down
35 changes: 31 additions & 4 deletions include/nuttx/can/can.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@
* nature of the error.
* Dependencies: None
*
* CANIOC_SET_TRANSV_STATE
* CANIOC_SET_TRANSVSTATE
* Description: Set specific can transceiver state
*
* Argument: A pointer to an int type that describes the CAN
Expand All @@ -296,7 +296,7 @@
* nature of the error.
* Dependencies: None
*
* CANIOC_GET_TRANSV_STATE
* CANIOC_GET_TRANSVSTATE
* Description: Get specific can transceiver state
*
* Argument: A pointer to an int type that describes the CAN
Expand All @@ -305,6 +305,29 @@
* is returned with the errno variable set to indicate the
* nature of the error.
* Dependencies: None
*
* CANIOC_SET_IWATERMARK
* Description: Set read watermark. This watermark limits number of
* bytes multiple frames can take in read. The frame is not
* placed to the provided buffer if it would start after
* the watermark offset (thus value 0 ensures only one
* frame to be placed into the buffer). The default valued
* is SIZE_MAX.
*
* Argument: A pointer to an size_t type with watermark value.
* 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.
* Dependencies: None
*
* CANIOC_GET_IWATERMARK
* Description: Get read watermark.
*
* Argument: A pointer to an size_t type for watermark value.
* 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.
* Dependencies: None
*/

#define CANIOC_RTR _CANIOC(1)
Expand All @@ -326,9 +349,11 @@
#define CANIOC_GET_STATE _CANIOC(17)
#define CANIOC_SET_TRANSVSTATE _CANIOC(18)
#define CANIOC_GET_TRANSVSTATE _CANIOC(19)
#define CANIOC_SET_IWATERMARK _CANIOC(20)
#define CANIOC_GET_IWATERMARK _CANIOC(21)

#define CAN_FIRST 0x0001 /* First common command */
#define CAN_NCMDS 19 /* 20 common commands */
#define CAN_NCMDS 21 /* 21 common commands */

/* User defined ioctl commands are also supported. These will be forwarded
* by the upper-half CAN driver to the lower-half CAN driver via the
Expand Down Expand Up @@ -796,13 +821,15 @@ struct can_ops_s
*
* The elements of 'cd_ops', and 'cd_priv'
*
* The common logic will initialize all semaphores.
* The common logic will initialize all semaphores and set 'watermark' to
* 'SIZE_MAX'.
*/

struct can_reader_s
{
struct list_node list;
struct can_rxfifo_s fifo; /* Describes receive FIFO */
size_t rxwatermark;
FAR struct pollfd *cd_fds;
};

Expand Down
Loading