You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "Cynerd (via GitHub)" <gi...@apache.org> on 2025/11/21 10:56:44 UTC
[PR] drivers/can: message alignment for read and write operations [nuttx]
Cynerd opened a new pull request, #17360:
URL: https://github.com/apache/nuttx/pull/17360
## Summary
This is a follow-up on https://github.com/apache/nuttx/pull/17283. The @ppisa comments there sent me in a different direction altogether, and thus I am creating a new PR with a different approach.
## Impact
This preserves the old behavior for reading and writing for default message alignment value, but allows more versatile usage such as reading of a single message and reading to the array of messages.
## Testing
Tested on a custom SAMv7 board.
```c
int fd = open("/dev/can0", O_RDWR);
if (fd < 0) {
printf("Failed to open can: %s", strerror(errno));
return -1;
}
unsigned msgalign = 0;
ioctl(fd, CANIOC_SET_MSGALIGN, &msgalign);
while (true) {
sleep(1); /* Give us some time to use cansend multiple times*/
char buf[200];
size_t n = read(fd, buf, 200);
printf("Read %d", n);
size_t offset = 0;
while (offset < n) {
struct can_msg_s *msg = (void*)&buf[offset];
printf(" id%d", msg->cm_hdr.ch_id);
offset += CAN_MSGLEN(can_dlc2bytes(msg->cm_hdr.ch_dlc));
if (msgalign)
offset = roundup(offset, msgalign);
}
printf("\n");
}
```
```bash
for i in {0..4}; do cansend can0 "00$i##0"; done
```
For `msgalign == 0`:
```
Read 4 id0
Read 4 id1
Read 4 id2
Read 4 id3
Read 4 id4
```
For `msgalign == 1`:
```
Read 4 id0
Read 16 id1 id2 id3 id4
```
(Note: the first message is handled before the rest are received so that is why it is separate)
For `msgalign == 3`::
```
Read 6 id0
Read 24 id1 id2 id3 id4
```
The write tests follow. They are in pair of C code and candump log.
```c
int fd = open("/dev/can0", O_RDWR);
unsigned msgalign = 1;
ioctl(fd, CANIOC_SET_MSGALIGN, &msgalign);
char buf[9];
struct can_msg_s *msg1 = (void*)&buf[0];
msg1->cm_hdr = (struct can_hdr_s) {.ch_id = 1, .ch_dlc = 0};
struct can_msg_s *msg2 = (void*)&buf[4];
msg2->cm_hdr = (struct can_hdr_s) {.ch_id = 2, .ch_dlc = 0};
size_t n = write(fd, buf, sizeof buf);
printf("Written %d\n", n); // Written 8
```
```
can0 001 [0] ''
can0 002 [0] ''
```
(The same behavior is for `msgalign == 0`, I tested it but the output is the same so not posting it here.)
```c
int fd = open("/dev/can0", O_RDWR);
unsigned msgalign = sizeof(struct can_msg_s);
ioctl(fd, CANIOC_SET_MSGALIGN, &msgalign);
struct can_msg_s msgs[2];
msgs[0].cm_hdr = (struct can_hdr_s) {.ch_id = 1, .ch_dlc = 0};
msgs[1].cm_hdr = (struct can_hdr_s) {.ch_id = 2, .ch_dlc = 0};
size_t n = write(fd, msgs, sizeof msgs);
printf("Written %d\n", n); // Written 136
```
```
can0 001 [0] ''
can0 002 [0] ''
```
The write size is 136 because I have CAN-FD enabled, and thus `struct can_msg_s` has space for 64 bytes of data. That makes it 68 times two.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550459364
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
Review Comment:
> It is added to provide a complete set of macros that BSD provides (https://man.freebsd.org/cgi/man.cgi?query=roundup&sektion=9&manpath=NetBSD+8.0). Adding only some of them doesn't feel right to me.
Okay that sounds good. I think we should have a short docstring for each macro. Thanks!
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "ppisa (via GitHub)" <gi...@apache.org>.
ppisa commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550306318
##########
drivers/can/can.c:
##########
@@ -702,6 +717,13 @@ static ssize_t can_write(FAR struct file *filep, FAR const char *buffer,
/* Increment the number of bytes that were sent */
nsent += msglen;
+
+ if (msgalign > 1)
+ {
+ nsent = powerof2(msgalign)
Review Comment:
Even on powerful platforms as is x86, the integer division has significant latency, really significant.
For Zen5 ([https://www.agner.org/optimize/instruction_tables.pdf](https://www.agner.org/optimize/instruction_tables.pdf)) you read for 32-bit division (our case) if we do not switch to size_t which would be 64-bit and even worse, you read
|Instruction |Operands |Ops |Latency |Reciprocal throughput|
| -----------------|---------------|-------|---------|-------------------------------|
|DIV | r32/m32 |2 | 11-14 | 6 |
|IDIV | r32/m32 |2 | 11-14 | 6 |
This means that the latency for the availability of result is up to 14 cycles and only the single division can be issued once per 6 cycles. If you compare it with 64-bit multiplication which has 3 cycles latency and can be started once per cycle or even shifts (SHL, SHR, SAR) which has latency 1 cycle and two can be started per cycle then the division is horrible. The mispredicted branch has penalty equivalent up to around 100 sequentially executed simple instructions but it would be kept in branch predictor for this case and the latency is masked. So I think that the check for power of two and branch should be there.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550500677
##########
include/sys/param.h:
##########
@@ -53,6 +53,39 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding.
+ *
+ * These macros accept value to be rounded or checked as 'a' and base as 'b'.
+ * They are consistent with those provided by some BSDs.
+ *
Review Comment:
Suggestion:
```suggestion
* howmany() calculates how many b fit into a.
*
* rounddown() and roundup() round a to the next b.
*
* rounddown2() and roundup2() are for powers of 2 only.
*
* powerof2() returns 1 if a is a power of 2, 0 otherwise.
```
Thoughts?
##########
include/sys/param.h:
##########
@@ -53,6 +53,39 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding.
+ *
+ * These macros accept value to be rounded or checked as 'a' and base as 'b'.
+ * They are consistent with those provided by some BSDs.
+ *
+ * Warning: rounddown2 and roundup2 can be used only if 'b' is power of 2,
+ * otherwise they will produce wrong result.
+ */
Review Comment:
```suggestion
*
* In all of these macros, outcome is undefined if b is 0.
*
* All parameters are assumed to be unsigned integers.
*/
```
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2549916009
##########
drivers/can/can.c:
##########
@@ -702,6 +717,13 @@ static ssize_t can_write(FAR struct file *filep, FAR const char *buffer,
/* Increment the number of bytes that were sent */
nsent += msglen;
+
+ if (msgalign > 1)
+ {
+ nsent = powerof2(msgalign)
Review Comment:
That is absolutely true on platforms that do have ALU with division, but I would bet that on platforms without it (such as cortex-m0) this should be faster over emulation of division as the emulation could take hunderts of cycles (consider that there are actually multiplication and division in `roundup`).
Thus, my reasoning is:
- It is either a very tiny MCU without division, and this branching could be beneficial.
- It is larger MCU that will have division bult in, but it will also be in general faster, thus removing the condition would be a micro-optimization, where I don't think that this is hot code for that.
Depending on the compiler optimization it could also detect that code that is part of `powerof2` is actually `stdc_has_single_bit` and `roundup2` `stdc_bit_ceil`. The macro could even replace these with an appropriate compiler builtin if that would be available. Thus making the happy path of "power of 2" very fast, of course, depending on architecture, if those builtins are converted to single instruction or not.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "ppisa (via GitHub)" <gi...@apache.org>.
ppisa commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550306318
##########
drivers/can/can.c:
##########
@@ -702,6 +717,13 @@ static ssize_t can_write(FAR struct file *filep, FAR const char *buffer,
/* Increment the number of bytes that were sent */
nsent += msglen;
+
+ if (msgalign > 1)
+ {
+ nsent = powerof2(msgalign)
Review Comment:
Even on powerful platforms as is x86, the integer division has significant latency, really significant.
For Zen5 ([https://www.agner.org/optimize/instruction_tables.pdf](https://www.agner.org/optimize/instruction_tables.pdf)) you read for 32-bit division (our case) if we do not switch to size_t which would be 64-bit and even worse, you read
|Instruction |Operands |Ops |Latency |Reciprocal throughput|
| -----------------|---------------|-------|---------|-------------------------------|
|DIV | r32/m32 |2 | 11-14 | 6 |
|IDIV | r32/m32 |2 | 11-14 | 6 |
This means that the latency for the availability of result is up to 14 cycles and only the single division can be issued once per 6 cycles. If you compare it with 64-bit multiplication which has 3 cycles latency and can be started once per cycle or even shifts (SHL, SHR, SAR) which has latency 1 cycle and two can be started per cycle then the division is horrible. The misdirected branch has penalty equivalent up to around 100 sequentially executed simple instructions but it would be kept in branch predictor for this case and the latency is masked. So I think that the check for power of two and branch should be there.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "linguini1 (via GitHub)" <gi...@apache.org>.
linguini1 commented on PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#issuecomment-3568080674
> Hi, does anyone know why "unable to merge"? This PR has 4 approving reviews. Thanks!
It is because there are outstanding comments
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2551052666
##########
include/sys/param.h:
##########
@@ -53,6 +53,39 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding.
+ *
+ * These macros accept value to be rounded or checked as 'a' and base as 'b'.
+ * They are consistent with those provided by some BSDs.
+ *
Review Comment:
It is commited now. I have mentioned you in the commit message.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2549916009
##########
drivers/can/can.c:
##########
@@ -702,6 +717,13 @@ static ssize_t can_write(FAR struct file *filep, FAR const char *buffer,
/* Increment the number of bytes that were sent */
nsent += msglen;
+
+ if (msgalign > 1)
+ {
+ nsent = powerof2(msgalign)
Review Comment:
That is true on platforms that do have ALU with division, but I would bet that on platforms without it (such as cortex-m0) this should be faster over emulation of division as the emulation could take hunderts of cycles (consider that there are actually multiplication and division in `roundup`).
Thus, my reasoning is:
- It is either a very tiny MCU without division, and this branching could be beneficial.
- It is larger MCU that will have division bult in, but it will also be in general faster, thus removing the condition would be a micro-optimization, where I don't think that this is hot code for that.
Depending on the compiler optimization it could also detect that code that is part of `powerof2` is actually `stdc_has_single_bit` and `roundup2` `stdc_bit_ceil`. The macro could even replace these with an appropriate compiler builtin if that would be available. Thus making the happy path of "power of 2" very fast, of course, depending on architecture, if those builtins are converted to single instruction or not.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2551106908
##########
include/sys/param.h:
##########
@@ -53,6 +53,39 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding.
+ *
+ * These macros accept value to be rounded or checked as 'a' and base as 'b'.
+ * They are consistent with those provided by some BSDs.
+ *
Review Comment:
Thank you!
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2553974606
##########
Documentation/guides/reading_can_msgs.rst:
##########
@@ -2,61 +2,89 @@
Reading CAN Messages
====================
-.. warning::
- Migrated from:
- https://cwiki.apache.org/confluence/display/NUTTX/Reading+CAN+Messages
+NuttX's CAN driver's default behavior is to return multiple messages for single
+``read`` operation, if they fit. If your code (especially if migrated from
+SocketCAN) doesn't count with it, then you will most likely encounter seemingly
+lost frames. You have two options: Either implement your code to support his
Review Comment:
@Cynerd please fix this typo to let us to merge it
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550054041
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
Review Comment:
Yes. It is added to provide a complete set of macros what BSD provides (https://man.freebsd.org/cgi/man.cgi?query=roundup&sektion=9&manpath=NetBSD+8.0). Adding only some of them doesn't feel right to me.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2551042523
##########
include/sys/param.h:
##########
@@ -53,6 +53,39 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding.
+ *
+ * These macros accept value to be rounded or checked as 'a' and base as 'b'.
+ * They are consistent with those provided by some BSDs.
+ *
Review Comment:
I like it. Thank you.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2549785021
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+#define howmany(a, b) (((a) + ((b) - 1)) / (b))
Review Comment:
indent in all places
##########
drivers/can/can.c:
##########
@@ -702,6 +717,13 @@ static ssize_t can_write(FAR struct file *filep, FAR const char *buffer,
/* Increment the number of bytes that were sent */
nsent += msglen;
+
+ if (msgalign > 1)
+ {
+ nsent = powerof2(msgalign)
Review Comment:
what's benifit to runtime check align? this check/jump may already consume the similar overhead of divide
##########
include/nuttx/can/can.h:
##########
@@ -796,13 +823,14 @@ 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 'msgalign' to '1'.
*/
struct can_reader_s
{
struct list_node list;
struct can_rxfifo_s fifo; /* Describes receive FIFO */
+ unsigned msgalign;
Review Comment:
unsigned int
##########
drivers/can/can.c:
##########
@@ -595,9 +608,11 @@ static int can_xmit(FAR struct can_dev_s *dev)
static ssize_t can_write(FAR struct file *filep, FAR const char *buffer,
size_t buflen)
{
- FAR struct inode *inode = filep->f_inode;
- FAR struct can_dev_s *dev = inode->i_private;
- FAR struct can_txcache_s *sender = &dev->cd_sender;
+ FAR struct inode *inode = filep->f_inode;
+ FAR struct can_dev_s *dev = inode->i_private;
+ FAR struct can_txcache_s *sender = &dev->cd_sender;
+ FAR struct can_reader_s *reader = filep->f_priv;
+ int msgalign = reader->msgalign;
Review Comment:
unsigned int
##########
drivers/can/can.c:
##########
@@ -380,6 +382,7 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
{
FAR struct can_reader_s *reader;
FAR struct can_rxfifo_s *fifo;
+ int msgalign;
Review Comment:
unsigned int
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550055588
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
+
+#ifndef rounddown
+# define rounddown(a, b) (((a) / (b)) * (b))
+#endif /* rounddown */
+
+#ifndef rounddown2
+# define rounddown2(a, b) ((a) & ~((b) - 1))
+#endif /* rounddown2 */
+
+#ifndef roundup
+# define roundup(a, b) ((((a) + ((b) - 1)) / (b)) * (b))
+#endif /* roundup */
+
+#ifndef roundup2
+# define roundup2(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
+#endif /* roundup2 */
Review Comment:
Yeah, that is a good idea.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550022751
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
+
+#ifndef rounddown
+# define rounddown(a, b) (((a) / (b)) * (b))
+#endif /* rounddown */
+
+#ifndef rounddown2
+# define rounddown2(a, b) ((a) & ~((b) - 1))
+#endif /* rounddown2 */
Review Comment:
rounddown() and rounddown2() also appear unused, but I think it's OK to have them as counterparts of roundup() and roundup2().
Same comment as roundup2(): Can we have a docstring to explain that rounddown2() is for rounding down of powers of 2 only? Thanks!
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
+
+#ifndef rounddown
+# define rounddown(a, b) (((a) / (b)) * (b))
+#endif /* rounddown */
+
+#ifndef rounddown2
+# define rounddown2(a, b) ((a) & ~((b) - 1))
+#endif /* rounddown2 */
+
+#ifndef roundup
+# define roundup(a, b) ((((a) + ((b) - 1)) / (b)) * (b))
+#endif /* roundup */
+
+#ifndef roundup2
+# define roundup2(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
+#endif /* roundup2 */
Review Comment:
Can we have a docstring to explain that roundup2() is for rounding up powers of 2 only? It took me a while to understand what this is for. Thanks!
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
Review Comment:
howmany() macro does not appear to be used anywhere?
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2551050814
##########
include/sys/param.h:
##########
@@ -53,6 +53,39 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding.
+ *
+ * These macros accept value to be rounded or checked as 'a' and base as 'b'.
+ * They are consistent with those provided by some BSDs.
+ *
Review Comment:
Feel free to commit those (suggested) docstrings or edit to your liking :-)
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#issuecomment-3568038563
Hi, does anyone know why "unable to merge"? This PR has 4 approving reviews. Thanks!
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #17360:
URL: https://github.com/apache/nuttx/pull/17360
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2554264377
##########
Documentation/guides/reading_can_msgs.rst:
##########
@@ -2,61 +2,89 @@
Reading CAN Messages
====================
-.. warning::
- Migrated from:
- https://cwiki.apache.org/confluence/display/NUTTX/Reading+CAN+Messages
+NuttX's CAN driver's default behavior is to return multiple messages for single
+``read`` operation, if they fit. If your code (especially if migrated from
+SocketCAN) doesn't count with it, then you will most likely encounter seemingly
+lost frames. You have two options: Either implement your code to support his
Review Comment:
Done
Sorry for the delay. I was away for a weekend.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2554264377
##########
Documentation/guides/reading_can_msgs.rst:
##########
@@ -2,61 +2,89 @@
Reading CAN Messages
====================
-.. warning::
- Migrated from:
- https://cwiki.apache.org/confluence/display/NUTTX/Reading+CAN+Messages
+NuttX's CAN driver's default behavior is to return multiple messages for single
+``read`` operation, if they fit. If your code (especially if migrated from
+SocketCAN) doesn't count with it, then you will most likely encounter seemingly
+lost frames. You have two options: Either implement your code to support his
Review Comment:
Done
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550054041
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
Review Comment:
Yes. It is added to provide a complete set of macros what BSD provides (gi/man.cgi?query=roundup&sektion=9&manpath=NetBSD+8.0). Adding only some of them doesn't feel right to me.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550054041
##########
include/sys/param.h:
##########
@@ -53,6 +53,32 @@
# define nitems(_a) (sizeof(_a) / sizeof(0[(_a)]))
#endif /* nitems */
+/* Macros for counting and rounding. */
+
+#ifndef howmany
+# define howmany(a, b) (((a) + ((b) - 1)) / (b))
+#endif /* howmany */
Review Comment:
It is added to provide a complete set of macros that BSD provides (https://man.freebsd.org/cgi/man.cgi?query=roundup&sektion=9&manpath=NetBSD+8.0). Adding only some of them doesn't feel right to me.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "ppisa (via GitHub)" <gi...@apache.org>.
ppisa commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2550318312
##########
Documentation/components/drivers/character/can.rst:
##########
@@ -31,11 +31,45 @@ Files supporting CAN can be found in the following locations:
``can_hdr_s`` structure depends on ``CONFIG_CAN_TIMESTAMP`` and
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*...
-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:
+The upper half driver supports the following ``ioctl`` commands:
+
+- **CANIOC_RTR**: Send the given message (passed as ``ioctl`` argument) as a
+ remote request. On successful return, the passed message structure is updated
+ with the contents of the received message; i.e. the message ID and the
+ standard/extended ID indication bit stay the same, but the DLC and data bits
+ are updated with the contents of the received message. If no response is
+ received after the specified timeout, ioctl will return.
+- **CANIOC_GET_BITTIMING**: Return the current bit timing settings.
+- **CANIOC_SET_BITTIMING**: Set new current bit timing values.
+- **CANIOC_ADD_STDFILTER**: Add an address filter for a standard 11 bit address.
+- **CANIOC_ADD_EXTFILTER**: Add an address filter for a extended 29 bit address.
+- **CANIOC_DEL_STDFILTER**: Remove an address filter for a standard 11 bit
+ address.
+- **CANIOC_DEL_EXTFILTER**: Remove an address filter for a standard 29 bit
+ address.
+- **CANIOC_GET_CONNMODES**: Get the current bus connection modes.
+- **CANIOC_SET_CONNMODES**: Set new bus connection modes values.
+- **CANIOC_BUSOFF_RECOVERY**: Initiates the BUS-OFF recovery sequence.
+- **CANIOC_SET_NART**: Enable/Disable NART (No Automatic Retry).
+- **CANIOC_SET_ABOM**: Enable/Disable ABOM (Automatic Bus-off Management).
+- **CANIOC_IFLUSH**: Flush data received but not read.
+- **CANIOC_OFLUSH**: Flush data written but not transmitted.
+- **CANIOC_IOFLUSH**: Flush data received but not read and data written but not
+ transmitted.
+- **CANIOC_SET_STATE**: Set specific can controller state.
+- **CANIOC_GET_STATE**: Get specific can controller state.
+- **CANIOC_SET_TRANSVSTATE**: Set specific can transceiver state.
+- **CANIOC_GET_TRANSVSTATE**: Get specific can transceiver state.
+- **CANIOC_SET_MSGALIGN**: Set messages alignment. Read and written messages can
+ be configured to be aligned to multiple of given bytes by this. The default
+ value is 1. The alignment affects both read and write operation. The value 0
+ has a special meaning where write behaves the same way as with 1, but read
Review Comment:
I would keep the same behavior as for the read. If it is zero, you send only single message even if you provide whole buffer. It would make lot of code easier.
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "Cynerd (via GitHub)" <gi...@apache.org>.
Cynerd commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2551117281
##########
Documentation/components/drivers/character/can.rst:
##########
@@ -31,11 +31,45 @@ Files supporting CAN can be found in the following locations:
``can_hdr_s`` structure depends on ``CONFIG_CAN_TIMESTAMP`` and
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*...
-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:
+The upper half driver supports the following ``ioctl`` commands:
+
+- **CANIOC_RTR**: Send the given message (passed as ``ioctl`` argument) as a
+ remote request. On successful return, the passed message structure is updated
+ with the contents of the received message; i.e. the message ID and the
+ standard/extended ID indication bit stay the same, but the DLC and data bits
+ are updated with the contents of the received message. If no response is
+ received after the specified timeout, ioctl will return.
+- **CANIOC_GET_BITTIMING**: Return the current bit timing settings.
+- **CANIOC_SET_BITTIMING**: Set new current bit timing values.
+- **CANIOC_ADD_STDFILTER**: Add an address filter for a standard 11 bit address.
+- **CANIOC_ADD_EXTFILTER**: Add an address filter for a extended 29 bit address.
+- **CANIOC_DEL_STDFILTER**: Remove an address filter for a standard 11 bit
+ address.
+- **CANIOC_DEL_EXTFILTER**: Remove an address filter for a standard 29 bit
+ address.
+- **CANIOC_GET_CONNMODES**: Get the current bus connection modes.
+- **CANIOC_SET_CONNMODES**: Set new bus connection modes values.
+- **CANIOC_BUSOFF_RECOVERY**: Initiates the BUS-OFF recovery sequence.
+- **CANIOC_SET_NART**: Enable/Disable NART (No Automatic Retry).
+- **CANIOC_SET_ABOM**: Enable/Disable ABOM (Automatic Bus-off Management).
+- **CANIOC_IFLUSH**: Flush data received but not read.
+- **CANIOC_OFLUSH**: Flush data written but not transmitted.
+- **CANIOC_IOFLUSH**: Flush data received but not read and data written but not
+ transmitted.
+- **CANIOC_SET_STATE**: Set specific can controller state.
+- **CANIOC_GET_STATE**: Get specific can controller state.
+- **CANIOC_SET_TRANSVSTATE**: Set specific can transceiver state.
+- **CANIOC_GET_TRANSVSTATE**: Get specific can transceiver state.
+- **CANIOC_SET_MSGALIGN**: Set messages alignment. Read and written messages can
+ be configured to be aligned to multiple of given bytes by this. The default
+ value is 1. The alignment affects both read and write operation. The value 0
+ has a special meaning where write behaves the same way as with 1, but read
Review Comment:
Ok. I changed the behavior to be consistent between read and write. 👍
--
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
Re: [PR] drivers/can: message alignment for read and write operations [nuttx]
Posted by "ppisa (via GitHub)" <gi...@apache.org>.
ppisa commented on code in PR #17360:
URL: https://github.com/apache/nuttx/pull/17360#discussion_r2551150146
##########
Documentation/guides/reading_can_msgs.rst:
##########
@@ -2,61 +2,89 @@
Reading CAN Messages
====================
-.. warning::
- Migrated from:
- https://cwiki.apache.org/confluence/display/NUTTX/Reading+CAN+Messages
+NuttX's CAN driver's default behavior is to return multiple messages for single
+``read`` operation, if they fit. If your code (especially if migrated from
+SocketCAN) doesn't count with it, then you will most likely encounter seemingly
+lost frames. You have two options: Either implement your code to support his
Review Comment:
s/his/this
--
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