drivers/can: iwatermark ioctl to control multiple frame read

[Cynerd]

Summary

This adds ioctl to NuttX's CAN that allows control of the read behavior.

Impact

The old behavior of reading multiple frames with a single read is preserved. The new ioctl just adds the ability to limit it. That includes not only disabilities but also more versatile limitations based on the size, which allows for multiple frames to be read.

Testing

Tested on a custom SAMv7 board.

  int fd = open("/dev/can0", O_RDWR);
  if (fd < 0) {
    syslog(LOG_ERR, "Failed to open can: %s", strerror(errno));
    return -1;
  }
  size_t iw = SIZE_MAX;
  ioctl(fd, CANIOC_SET_IWATERMARK, &iw);
  while (true) {
    sleep(1); /* Give us some time to use cansend multiple times*/
    struct can_msg_s frame;
    size_t n = read(fd, &frame, sizeof frame);
    syslog(LOG_INFO, "Read %d id%d", n, frame.cm_hdr.ch_id);
  }

for i in {0..4}; do cansend can0 "00$i##0"; done

For iw == SIZE_MAX:

[1762352028.612600] startup_main: Read 4 id0
[1762352029.613000] startup_main: Read 16 id1

for iw == 0:

[1762352153.185100] startup_main: Read 4 id0
[1762352154.185500] startup_main: Read 4 id1
[1762352155.185900] startup_main: Read 4 id2
[1762352156.186300] startup_main: Read 4 id3
[1762352157.186700] startup_main: Read 4 id4

for iw == 5:

[1762352293.124000] startup_main: Read 4 id0
[1762352294.124400] startup_main: Read 8 id1
[1762352295.124900] startup_main: Read 8 id3

[acassis]

@Cynerd please update the Documentation https://nuttx.apache.org/docs/latest/components/drivers/character/can.html to include infor about this new ioctl

[Cynerd]

@Cynerd please update the Documentation https://nuttx.apache.org/docs/latest/components/drivers/character/can.html to include infor about this new ioctl

I will as part of this PR. I wanted to check initially if this approach is suitable for interested parties.

[linguini1]

I would also mention GET_IWATERMARK.

[acassis]

@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?

[michallenc]

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

[acassis]

@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?

[acassis]

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

[michallenc]

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

[acassis]

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.

[xiaoxiang781216]

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

[ppisa]

@Cynerd, thanks for the CAN chardev interface extension. I think that it simplifies lot of code where elimination of expensive syscalls for really high throughput is not required. In the NutX case multiple messages read in one go could have effect only in MMU or protected case.

As for the name CANIOC_SET_IWATERMARK, I am not sure if it is self-descriptive. It probably originates in termios style of IOCTLs building. But CANIOC_SET_RDMSGLIMIT or CANIOC_SET_RDWATERMARK or... would be more descriptive to me. I am not sure, but my initial read of the observation did not bring to the intended meaning and I have had to read the code and documentation.

[ppisa]

After some parallel discussion there is some alternative approach.

Instead of the read buffer fill size, when the more messages are not transferred into read buffer, there is another may it be more valuable option to specify alignment of read "slots". If the alignment is size of the the whole maximal message and the buffer size is the same then the result is the same as for above option. But if there is interest to minimize syscalls count, then it is possible to offer buffer size in read equal to size of multiple full messages. But there is even another usage, which has not been thought yet probably. It is specifying of the messages alignment required for correct read of ext ID and other header fields. Because on the architectures which do not support unaligned reads, current code requires memcopy of the second and followup messages to some other correctly aligned location to not experience unaligned fault. Even on some RISC-V implementations (i.e. PolarFire) where RISC-V ABI guarantees unaligned accesses, the unaligned reads are emulated by SBI and are extremely slow.

So the suggestion is to change the control IOTL to CANIOC_SET_MSGALIGN and then ensure, that next message is placed into read buffer at location controlled by align. There is problem that power of two alignments have purpose for these cases where only aliment is intended but when whole message buffer is required it is (header size + data size + optional pad to ensure next header alignment). The required elements alignment is automatically ensured when sizeof(struct ...) is used... So there is proposal for processing of alignment which minimizes high divide cost operations. The a is specified alignment, s is size of the header plus data and n is span between current filed message and next message to put into buffer

if (a & (a - 1)) { /* alignment is not power of two */
  if (s <= a) {
    n = a;
  } else {
    n = s + a - 1;
    n = n - (n % a);
  }
} else { /* easy case, power of two */
  n = a - 1;
  n = (s + n) & ~n;
}

This setup seems to me as the most practically usable, but suggestions and alternatives are welcomed.

[ppisa]

When I think about CANIOC_SET_MSGALIGN then the default value for newly open /dev/can should be 1 to match current NuttX CAN chardev API. The value 0 should be used to switch multiple messages per single read or write off completely to make think more clean and switch off multiple messages in the single buffer simpler.

Due to C89 requirements in common headers, the CAN_MSGSIZEALIGN(s, a) has to be macro and has to be implemented by ternary operators.

I like idea of this option to control multiple messages in the single read and or write so much, that I consider to return support of multiple messages per syscall even to our RTEMS CAN FD subsystem implementation which has origin in LinCAN which has originally support for multiple messages per syscall but they have been aligned by full message size. If our RTEMS CAN FD subsystem would be ported to Linux again (which had in past and I believe would have much better RT properties than SocketCAN) then there multiple messages per single syscall would have real value. On RTEMS and NuttX without MMU and real syscall, there would be no big difference. But for NuttX if MMU setup prevails one day it would be advantage as well.