[PATCH] cpukit/include/dev/can: Disabled debug prints in CAN Framework

[Prashanth S]

>On Mon, Nov 28, 2022 at 8:46 PM Prashanth S <fishesprashanth at gmail.com>
wrote:
>>

>> ---

>>  cpukit/include/dev/can/can.h     |  2 +-

>>  testsuites/libtests/can01/init.c | 14 ++++++++------

>>  2 files changed, 9 insertions(+), 7 deletions(-)

>>

>> diff --git a/cpukit/include/dev/can/can.h b/cpukit/include/dev/can/can.h

>> index 9e55395039..27afd00088 100644

>> --- a/cpukit/include/dev/can/can.h

>> +++ b/cpukit/include/dev/can/can.h

>> @@ -53,7 +53,7 @@

>>        printf(str, ##__VA_ARGS__);
      \
>>      } while (false);

>>

>> -#define CAN_DEBUG(str, ...) DEBUG(str, ##__VA_ARGS__)

>> +#define CAN_DEBUG(str, ...) //DEBUG(str, ##__VA_ARGS__)

>

>This is not the right way to disable this, for several reasons.

>

>I should have caught this DEBUG printing stuff earlier. My guess is

>that you should just remove it all from the production code, or it

>should be controlled at a higher level with -DDEBUG kind of flag.

>However, I agree that the current implementation is overly verbose in

>debug mode. I'd just strip it all out.


I will update the debug prints implementation. I will add -DCAN_DEBUG
option to be used
if debug prints are required.

>

>>  #define CAN_DEBUG_BUF(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)

>>  #define CAN_DEBUG_ISR(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)

>>  #define CAN_DEBUG_LOCK(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)

>> diff --git a/testsuites/libtests/can01/init.c
b/testsuites/libtests/can01/init.c
>> index 0675fe606f..c19d45602e 100644

>> --- a/testsuites/libtests/can01/init.c

>> +++ b/testsuites/libtests/can01/init.c

>> @@ -97,8 +97,10 @@ static void test_task(rtems_task_argument data)

>>

>>    rtems_test_assert(fd >= 0);

>>

>> +  printf("test_task %u\n", task_num);

>> +

>>    for (int i = 0; i < NUM_TEST_MSGS; i++) {

>> -    printf("test_task %u\n", task_num);

>> +    CAN_DEBUG("test_task %u\n", task_num);

>>

>>      msg.id = task_num;

>>      //FIXME: Implement Test cases for other flags also.

>

>There shouldn't be // comments anywhere.


I will remove that.

>

>> @@ -111,18 +113,18 @@ static void test_task(rtems_task_argument data)

>>

>>      msg_size = ((char *)&msg.data[msg.len] - (char *)&msg);

>>

>> -    printf("calling write task = %u\n", task_num);

>> +    CAN_DEBUG("calling write task = %u\n", task_num);

>

>The general philosophy in our test suite is to only print something if
>there's a failure. So just remove these debug statements that are

>leftover from your development approach.


I will remove extra debug prints.

>

>>

>>      count = write(fd, &msg, sizeof(msg));

>>      rtems_test_assert(count == msg_size);

>> -    printf("task = %u write count = %u\n", task_num, count);

>> +    CAN_DEBUG("task = %u write count = %u\n", task_num, count);

>>

>> -    printf("calling read task = %u\n", task_num);

>> +    CAN_DEBUG("calling read task = %u\n", task_num);

>>      count = read(fd, &msg, sizeof(msg));

>>      rtems_test_assert(count > 0);

>Since you send the messages, you know their sizes. You should be able

>to assert the exact amount received?


The CAN framework has only minimal Rx support (CAN Framework sends the
latest received CAN message).
So only the read success is checked in the test files.

>

>> -    printf("task = %u read count = %u\n", task_num, count);

>> +    CAN_DEBUG("task = %u read count = %u\n", task_num, count);

>>

>> -    printf("received message\n");

>> +    CAN_DEBUG("received message\n");

>>      can_print_msg(&msg);

>>

>>      sleep(1);

Regards
Prashanth S

On Tue, 29 Nov 2022 at 10:27, Gedare Bloom <gedare at rtems.org> wrote:

> Hi Prashanth,
>
> On Mon, Nov 28, 2022 at 8:46 PM Prashanth S <fishesprashanth at gmail.com>
> wrote:
> >
> > ---
> >  cpukit/include/dev/can/can.h     |  2 +-
> >  testsuites/libtests/can01/init.c | 14 ++++++++------
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/cpukit/include/dev/can/can.h b/cpukit/include/dev/can/can.h
> > index 9e55395039..27afd00088 100644
> > --- a/cpukit/include/dev/can/can.h
> > +++ b/cpukit/include/dev/can/can.h
> > @@ -53,7 +53,7 @@
> >        printf(str, ##__VA_ARGS__);
>        \
> >      } while (false);
> >
> > -#define CAN_DEBUG(str, ...) DEBUG(str, ##__VA_ARGS__)
> > +#define CAN_DEBUG(str, ...) //DEBUG(str, ##__VA_ARGS__)
>
> This is not the right way to disable this, for several reasons.
>
> I should have caught this DEBUG printing stuff earlier. My guess is
> that you should just remove it all from the production code, or it
> should be controlled at a higher level with -DDEBUG kind of flag.
> However, I agree that the current implementation is overly verbose in
> debug mode. I'd just strip it all out.
>
> >  #define CAN_DEBUG_BUF(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)
> >  #define CAN_DEBUG_ISR(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)
> >  #define CAN_DEBUG_LOCK(str, ...) CAN_DEBUG(str, ##__VA_ARGS__)
> > diff --git a/testsuites/libtests/can01/init.c
> b/testsuites/libtests/can01/init.c
> > index 0675fe606f..c19d45602e 100644
> > --- a/testsuites/libtests/can01/init.c
> > +++ b/testsuites/libtests/can01/init.c
> > @@ -97,8 +97,10 @@ static void test_task(rtems_task_argument data)
> >
> >    rtems_test_assert(fd >= 0);
> >
> > +  printf("test_task %u\n", task_num);
> > +
> >    for (int i = 0; i < NUM_TEST_MSGS; i++) {
> > -    printf("test_task %u\n", task_num);
> > +    CAN_DEBUG("test_task %u\n", task_num);
> >
> >      msg.id = task_num;
> >      //FIXME: Implement Test cases for other flags also.
>
> There shouldn't be // comments anywhere.
>
> > @@ -111,18 +113,18 @@ static void test_task(rtems_task_argument data)
> >
> >      msg_size = ((char *)&msg.data[msg.len] - (char *)&msg);
> >
> > -    printf("calling write task = %u\n", task_num);
> > +    CAN_DEBUG("calling write task = %u\n", task_num);
>
> The general philosophy in our test suite is to only print something if
> there's a failure. So just remove these debug statements that are
> leftover from your development approach.
>
> >
> >      count = write(fd, &msg, sizeof(msg));
> >      rtems_test_assert(count == msg_size);
> > -    printf("task = %u write count = %u\n", task_num, count);
> > +    CAN_DEBUG("task = %u write count = %u\n", task_num, count);
> >
> > -    printf("calling read task = %u\n", task_num);
> > +    CAN_DEBUG("calling read task = %u\n", task_num);
> >      count = read(fd, &msg, sizeof(msg));
> >      rtems_test_assert(count > 0);
> Since you send the messages, you know their sizes. You should be able
> to assert the exact amount received?
>
> > -    printf("task = %u read count = %u\n", task_num, count);
> > +    CAN_DEBUG("task = %u read count = %u\n", task_num, count);
> >
> > -    printf("received message\n");
> > +    CAN_DEBUG("received message\n");
> >      can_print_msg(&msg);
> >
> >      sleep(1);
> > --
> > 2.25.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20221129/b47b31e5/attachment.htm>