<div dir="ltr">>On Mon, Nov 28, 2022 at 8:46 PM Prashanth S <<a href="mailto:fishesprashanth@gmail.com">fishesprashanth@gmail.com</a>> wrote: <br>>> <br>>> --- <br>>> cpukit/include/dev/can/can.h | 2 +- <br>>> testsuites/libtests/can01/init.c | 14 ++++++++------ <br>>> 2 files changed, 9 insertions(+), 7 deletions(-) <br>>> <br>>> diff --git a/cpukit/include/dev/can/can.h b/cpukit/include/dev/can/can.h <br>>> index 9e55395039..27afd00088 100644 <br>>> --- a/cpukit/include/dev/can/can.h <br>>> +++ b/cpukit/include/dev/can/can.h <br>>> @@ -53,7 +53,7 @@ <br>>> printf(str, ##__VA_ARGS__); \<br>>> } while (false); <br>>> <br>>> -#define CAN_DEBUG(str, ...) DEBUG(str, ##__VA_ARGS__) <br>>> +#define CAN_DEBUG(str, ...) //DEBUG(str, ##__VA_ARGS__) <br>> <br>>This is not the right way to disable this, for several reasons. <br>> <br>>I should have caught this DEBUG printing stuff earlier. My guess is <br>>that you should just remove it all from the production code, or it <br>>should be controlled at a higher level with -DDEBUG kind of flag. <br>>However, I agree that the current implementation is overly verbose in <br>>debug mode. I'd just strip it all out. <br><blockquote style="margin:0 0 0 40px;border:none;padding:0px">I will update the debug prints implementation. I will add -DCAN_DEBUG option to be used<br>if debug prints are required.</blockquote><div>> <br>>> #define CAN_DEBUG_BUF(str, ...) CAN_DEBUG(str, ##__VA_ARGS__) <br>>> #define CAN_DEBUG_ISR(str, ...) CAN_DEBUG(str, ##__VA_ARGS__) <br>>> #define CAN_DEBUG_LOCK(str, ...) CAN_DEBUG(str, ##__VA_ARGS__) <br>>> diff --git a/testsuites/libtests/can01/init.c b/testsuites/libtests/can01/init.c<br>>> index 0675fe606f..c19d45602e 100644 <br>>> --- a/testsuites/libtests/can01/init.c <br>>> +++ b/testsuites/libtests/can01/init.c <br>>> @@ -97,8 +97,10 @@ static void test_task(rtems_task_argument data) <br>>> <br>>> rtems_test_assert(fd >= 0); <br>>> <br>>> + printf("test_task %u\n", task_num); <br>>> + <br>>> for (int i = 0; i < NUM_TEST_MSGS; i++) { <br>>> - printf("test_task %u\n", task_num); <br>>> + CAN_DEBUG("test_task %u\n", task_num); <br>>> <br>>> <a href="http://msg.id">msg.id</a> = task_num; <br>>> //FIXME: Implement Test cases for other flags also. <br>> <br>>There shouldn't be // comments anywhere. <br><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div>I will remove that.</div></blockquote></div><div>> <br>>> @@ -111,18 +113,18 @@ static void test_task(rtems_task_argument data) <br>>> <br>>> msg_size = ((char *)&msg.data[msg.len] - (char *)&msg); <br>>> <br>>> - printf("calling write task = %u\n", task_num); <br>>> + CAN_DEBUG("calling write task = %u\n", task_num); <br>> <br>>The general philosophy in our test suite is to only print something if <br><div>>there's a failure. So just remove these debug statements that are <br>>leftover from your development approach. <br></div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div><div>I will remove extra debug prints.</div></div></blockquote><div><div>> <br>>> <br>>> count = write(fd, &msg, sizeof(msg)); <br>>> rtems_test_assert(count == msg_size); <br>>> - printf("task = %u write count = %u\n", task_num, count); <br>>> + CAN_DEBUG("task = %u write count = %u\n", task_num, count); <br>>> <br>>> - printf("calling read task = %u\n", task_num); <br>>> + CAN_DEBUG("calling read task = %u\n", task_num); <br>>> count = read(fd, &msg, sizeof(msg)); <br>>> rtems_test_assert(count > 0); <br>>Since you send the messages, you know their sizes. You should be able <br>>to assert the exact amount received? <br></div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div><div>The CAN framework has only minimal Rx support (CAN Framework sends the latest received CAN message).<br>So only the read success is checked in the test files.<br></div></div></blockquote><div><div>> <br>>> - printf("task = %u read count = %u\n", task_num, count); <br>>> + CAN_DEBUG("task = %u read count = %u\n", task_num, count); <br>>> <br>>> - printf("received message\n"); <br>>> + CAN_DEBUG("received message\n"); <br>>> can_print_msg(&msg); <br>>> <br>>> sleep(1); </div><div> <br></div></div><div>Regards</div><div>Prashanth S</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 29 Nov 2022 at 10:27, Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Prashanth,<br>
<br>
On Mon, Nov 28, 2022 at 8:46 PM Prashanth S <<a href="mailto:fishesprashanth@gmail.com" target="_blank">fishesprashanth@gmail.com</a>> wrote:<br>
><br>
> ---<br>
> cpukit/include/dev/can/can.h | 2 +-<br>
> testsuites/libtests/can01/init.c | 14 ++++++++------<br>
> 2 files changed, 9 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/cpukit/include/dev/can/can.h b/cpukit/include/dev/can/can.h<br>
> index 9e55395039..27afd00088 100644<br>
> --- a/cpukit/include/dev/can/can.h<br>
> +++ b/cpukit/include/dev/can/can.h<br>
> @@ -53,7 +53,7 @@<br>
> printf(str, ##__VA_ARGS__); \<br>
> } while (false);<br>
><br>
> -#define CAN_DEBUG(str, ...) DEBUG(str, ##__VA_ARGS__)<br>
> +#define CAN_DEBUG(str, ...) //DEBUG(str, ##__VA_ARGS__)<br>
<br>
This is not the right way to disable this, for several reasons.<br>
<br>
I should have caught this DEBUG printing stuff earlier. My guess is<br>
that you should just remove it all from the production code, or it<br>
should be controlled at a higher level with -DDEBUG kind of flag.<br>
However, I agree that the current implementation is overly verbose in<br>
debug mode. I'd just strip it all out.<br>
<br>
> #define CAN_DEBUG_BUF(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)<br>
> #define CAN_DEBUG_ISR(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)<br>
> #define CAN_DEBUG_LOCK(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)<br>
> diff --git a/testsuites/libtests/can01/init.c b/testsuites/libtests/can01/init.c<br>
> index 0675fe606f..c19d45602e 100644<br>
> --- a/testsuites/libtests/can01/init.c<br>
> +++ b/testsuites/libtests/can01/init.c<br>
> @@ -97,8 +97,10 @@ static void test_task(rtems_task_argument data)<br>
><br>
> rtems_test_assert(fd >= 0);<br>
><br>
> + printf("test_task %u\n", task_num);<br>
> +<br>
> for (int i = 0; i < NUM_TEST_MSGS; i++) {<br>
> - printf("test_task %u\n", task_num);<br>
> + CAN_DEBUG("test_task %u\n", task_num);<br>
><br>
> <a href="http://msg.id" rel="noreferrer" target="_blank">msg.id</a> = task_num;<br>
> //FIXME: Implement Test cases for other flags also.<br>
<br>
There shouldn't be // comments anywhere.<br>
<br>
> @@ -111,18 +113,18 @@ static void test_task(rtems_task_argument data)<br>
><br>
> msg_size = ((char *)&msg.data[msg.len] - (char *)&msg);<br>
><br>
> - printf("calling write task = %u\n", task_num);<br>
> + CAN_DEBUG("calling write task = %u\n", task_num);<br>
<br>
The general philosophy in our test suite is to only print something if<br>
there's a failure. So just remove these debug statements that are<br>
leftover from your development approach.<br>
<br>
><br>
> count = write(fd, &msg, sizeof(msg));<br>
> rtems_test_assert(count == msg_size);<br>
> - printf("task = %u write count = %u\n", task_num, count);<br>
> + CAN_DEBUG("task = %u write count = %u\n", task_num, count);<br>
><br>
> - printf("calling read task = %u\n", task_num);<br>
> + CAN_DEBUG("calling read task = %u\n", task_num);<br>
> count = read(fd, &msg, sizeof(msg));<br>
> rtems_test_assert(count > 0);<br>
Since you send the messages, you know their sizes. You should be able<br>
to assert the exact amount received?<br>
<br>
> - printf("task = %u read count = %u\n", task_num, count);<br>
> + CAN_DEBUG("task = %u read count = %u\n", task_num, count);<br>
><br>
> - printf("received message\n");<br>
> + CAN_DEBUG("received message\n");<br>
> can_print_msg(&msg);<br>
><br>
> sleep(1);<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div>