Warning fixes. #356

kjassmann-renesas · 2021-06-18T20:55:43Z

Warning fixes in timers.c and tasks.c

Description

Fixes the following warnings:

arm-none-eabi-gcc -mcpu=cortex-m4 -mthumb -mfloat-abi=hard -mfpu=fpv4-sp-d16 -O2 -fmessage-length=0 -fsigned-char -ffunction-sections -fdata-sections -Wunused -Wuninitialized -Wall -Wextra -Wmissing-declarations -Wconversion -Wpointer-arith -Wshadow -Wlogical-op -Waggregate-return -Wfloat-equal  -g -D_RENESAS_RA_ -I"/e2_studio/workspace/freertos/src" -I"/e2_studio/workspace/freertos/ra/fsp/inc" -I"/e2_studio/workspace/freertos/ra/fsp/inc/api" -I"/e2_studio/workspace/freertos/ra/fsp/inc/instances" -I"/e2_studio/workspace/freertos/ra/fsp/src/rm_freertos_port" -I"/e2_studio/workspace/freertos/ra/aws/amazon-freertos/freertos_kernel/include" -I"/e2_studio/workspace/freertos/ra/arm/CMSIS_5/CMSIS/Core/Include" -I"/e2_studio/workspace/freertos/ra_gen" -I"/e2_studio/workspace/freertos/ra_cfg/fsp_cfg/bsp" -I"/e2_studio/workspace/freertos/ra_cfg/fsp_cfg" -I"/e2_studio/workspace/freertos/ra_cfg/aws" -std=c99 -MMD -MP -MF"ra/aws/amazon-freertos/freertos_kernel/timers.d" -MT"ra/aws/amazon-freertos/freertos_kernel/timers.o" -c -o "ra/aws/amazon-freertos/freertos_kernel/timers.o" -x c "../ra/aws/amazon-freertos/freertos_kernel/timers.c"
../ra/aws/amazon-freertos/freertos_kernel/timers.c: In function 'vTimerSetReloadMode':
../ra/aws/amazon-freertos/freertos_kernel/timers.c:456:38: warning: conversion from 'int' to 'uint8_t' {aka 'unsigned char'} may change value [-Wconversion]
  456 |                 pxTimer->ucStatus &= ~tmrSTATUS_IS_AUTORELOAD;
      |                                      ^
../ra/aws/amazon-freertos/freertos_kernel/timers.c: In function 'prvProcessExpiredTimer':
../ra/aws/amazon-freertos/freertos_kernel/timers.c:542:34: warning: conversion from 'int' to 'uint8_t' {aka 'unsigned char'} may change value [-Wconversion]
  542 |             pxTimer->ucStatus &= ~tmrSTATUS_IS_ACTIVE;
      |                                  ^
../ra/aws/amazon-freertos/freertos_kernel/timers.c: In function 'prvProcessReceivedCommands':
../ra/aws/amazon-freertos/freertos_kernel/timers.c:840:46: warning: conversion from 'int' to 'uint8_t' {aka 'unsigned char'} may change value [-Wconversion]
  840 |                         pxTimer->ucStatus &= ~tmrSTATUS_IS_ACTIVE;
      |                                              ^
../ra/aws/amazon-freertos/freertos_kernel/timers.c:879:54: warning: conversion from 'int' to 'uint8_t' {aka 'unsigned char'} may change value [-Wconversion]
  879 |                                 pxTimer->ucStatus &= ~tmrSTATUS_IS_ACTIVE;
      |                                                      ^
arm-none-eabi-gcc -mcpu=cortex-m4 -mthumb -mfloat-abi=hard -mfpu=fpv4-sp-d16 -O2 -fmessage-length=0 -fsigned-char -ffunction-sections -fdata-sections -Wunused -Wuninitialized -Wall -Wextra -Wmissing-declarations -Wconversion -Wpointer-arith -Wshadow -Wlogical-op -Waggregate-return -Wfloat-equal  -g -D_RENESAS_RA_ -I"/e2_studio/workspace/freertos/src" -I"/e2_studio/workspace/freertos/ra/fsp/inc" -I"/e2_studio/workspace/freertos/ra/fsp/inc/api" -I"/e2_studio/workspace/freertos/ra/fsp/inc/instances" -I"/e2_studio/workspace/freertos/ra/fsp/src/rm_freertos_port" -I"/e2_studio/workspace/freertos/ra/aws/amazon-freertos/freertos_kernel/include" -I"/e2_studio/workspace/freertos/ra/arm/CMSIS_5/CMSIS/Core/Include" -I"/e2_studio/workspace/freertos/ra_gen" -I"/e2_studio/workspace/freertos/ra_cfg/fsp_cfg/bsp" -I"/e2_studio/workspace/freertos/ra_cfg/fsp_cfg" -I"/e2_studio/workspace/freertos/ra_cfg/aws" -std=c99 -MMD -MP -MF"ra/aws/amazon-freertos/freertos_kernel/tasks.d" -MT"ra/aws/amazon-freertos/freertos_kernel/tasks.o" -c -o "ra/aws/amazon-freertos/freertos_kernel/tasks.o" -x c "../ra/aws/amazon-freertos/freertos_kernel/tasks.c"
../ra/aws/amazon-freertos/freertos_kernel/tasks.c: In function 'prvDeleteTCB':
../ra/aws/amazon-freertos/freertos_kernel/tasks.c:3932:39: warning: unused parameter 'pxTCB' [-Wunused-parameter]
 3932 |     static void prvDeleteTCB( TCB_t * pxTCB )
      |                               ~~~~~~~~^~~~~

Test Steps

Build using GCC (gcc-arm-none-eabi-9-2020-q2-update) with all warnings enabled.

Related Issue

See above. I have not made a GitHub issue, but I will if you request it.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

mingyue86010 · 2021-06-18T21:22:58Z

tasks.c

@@ -3945,6 +3945,10 @@ static void prvCheckTasksWaitingTermination( void )
         * want to allocate and clean RAM statically. */
        portCLEAN_UP_TCB( pxTCB );

+        /* Remove compiler warning about unused variables when portCLEAN_UP_TCB
+         * is not being used. */
+        ( void ) pxTCB;


Suggested change

( void ) pxTCB;

#if !defined( portCLEAN_UP_TCB ) || ( portCLEAN_UP_TCB == 0 )

( void ) pxTCB;

#endif

Thank you for the review.

This won't work because portCLEAN_UP_TCB is defined. It's just defined to nothing.

#define portCLEAN_UP_TCB( pxTCB )

Perhaps the comment wording could be improved. I'm open to suggestions.

yeah thank you for pointing out. I updated the changes here.

The updated changes also will not work. portCLEAN_UP_TCB is defined to nothing, it is not defined to 0.

Does this default definition not take care of this? https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/include/FreeRTOS.h#L314

It would, except that portCLEAN_UP_TCB is already defined in many ports provided in the repository. Examples:

https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/ARMv8M/non_secure/portable/GCC/ARM_CM33/portmacro.h#L269

https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/portable/IAR/ARM_CM33/non_secure/portmacro.h#L269

I could fix/remove these instances of portCLEAN_UP_TCB instead if you prefer.

cobusve

I am not convinced that we have to intoduce the clear defines here, let me get some more inputs

codecov · 2021-06-18T23:25:43Z

Codecov Report

Merging #356 (cf557a1) into main (1d86b97) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #356   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files           4        4           
  Lines        1272     1272           
  Branches      342      342           
=======================================
  Hits         1172     1172           
  Misses         53       53           
  Partials       47       47

Flag	Coverage Δ
unittests	`92.13% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d86b97...cf557a1. Read the comment docs.

kjassmann-renesas · 2021-06-21T15:18:16Z

I am not convinced that we have to intoduce the clear defines here, let me get some more inputs

I don't mind using the existing defines, but they would need to be typecast to avoid the warning. The bitwise not operation (~) appears to convert all literals to signed integers in GCC. The new defines seemed cleaner to me than typecasting the existing define that already includes a typecast.

Example of the equivalent fix with the existing define:

pxTimer->ucStatus &= (uint8_t) ~tmrSTATUS_IS_AUTORELOAD;

jefftenney · 2021-06-21T17:34:26Z

Another option is to change the type of ucStatus from uint8_t to int8_t. And update the existing defines to match. So ucStatus becomes cStatus.

aggarg · 2021-06-21T18:05:42Z

timers.c

+    #define tmrSTATUS_CLEAR_ACTIVE                  ( ( uint8_t ) ~0x01 )
+    #define tmrSTATUS_CLEAR_STATICALLY_ALLOCATED    ( ( uint8_t ) ~0x02 )
+    #define tmrSTATUS_CLEAR_AUTORELOAD              ( ( uint8_t ) ~0x04 )


Suggested change

#define tmrSTATUS_CLEAR_ACTIVE ( ( uint8_t ) ~0x01 )

#define tmrSTATUS_CLEAR_STATICALLY_ALLOCATED ( ( uint8_t ) ~0x02 )

#define tmrSTATUS_CLEAR_AUTORELOAD ( ( uint8_t ) ~0x04 )

#define tmrSTATUS_CLEAR_ACTIVE ( ( uint8_t ) ~tmrSTATUS_IS_ACTIVE )

#define tmrSTATUS_CLEAR_STATICALLY_ALLOCATED ( ( uint8_t ) ~tmrSTATUS_IS_STATICALLY_ALLOCATED )

#define tmrSTATUS_CLEAR_AUTORELOAD ( ( uint8_t ) ~tmrSTATUS_IS_AUTORELOAD )

The above is just a suggestion if we decide to add new defines.

kjassmann-renesas · 2021-06-25T16:32:07Z

Another option is to change the type of ucStatus from uint8_t to int8_t. And update the existing defines to match. So ucStatus becomes cStatus.

I do think that would make things cleaner, but it might not be worth the compatibility break (in my opinion).

kjassmann-renesas · 2021-06-25T16:39:22Z

Please let me know when you have made a decision on how you prefer to fix these warnings and if you would like me to make any changes.

jefftenney · 2021-06-25T16:40:18Z

I do think that would make things cleaner, but it might not be worth the compatibility break (in my opinion).

I don't think it would cause any compatibility issues -- what am I missing?

kjassmann-renesas · 2021-06-25T16:43:20Z

I do think that would make things cleaner, but it might not be worth the compatibility break (in my opinion).

I don't think it would cause any compatibility issues -- what am I missing?

My mistake, I didn't notice the xTIMER struct is limited in scope to timers.c. I will make this change.

jefftenney · 2021-06-25T16:47:52Z

My mistake, I didn't notice the xTIMER struct is limited in scope to timers.c. I will make this change.

Might be best to wait for feedback from @cobusve or others from FreeRTOS. Using a signed field may run afoul of overzealous static checkers due to bitwise operations, even though flag operations on a field that always gets promoted are safe.

kjassmann-renesas · 2021-06-25T17:39:31Z

My mistake, I didn't notice the xTIMER struct is limited in scope to timers.c. I will make this change.

Might be best to wait for feedback from @cobusve or others from FreeRTOS. Using a signed field may run afoul of overzealous static checkers due to bitwise operations, even though flag operations on a field that always gets promoted are safe.

Fair point, thanks. I will make this change if requested.

kjassmann-renesas · 2021-07-19T20:55:20Z

@cobusve, have you made a decision regarding the CLEAR defines yet? I can make changes if you have any specific request.

aggarg · 2021-07-19T21:54:45Z

@cobusve, have you made a decision regarding the CLEAR defines yet? I can make changes if you have any specific request.

Yes, apologies for the delay in response. Please make the following changes:

Please remove the new #defines and add the cast in place:

pxTimer->ucStatus &= ( ( uint8_t ) ~tmrSTATUS_IS_ACTIVE );

Instead of changing tasks.c, please remove the empty definitions of portCLEAN_UP_TCB from portmacro.h files so that the default one from FreeRTOS.h is used.

Please let us know if you would want us to make these changes.

Thank you for your contribution.

…OCATE_SECURE_CONTEXT( ulSecureStackSize ) from ports. When these are undefined, the default empty definition is defined in FreeRTOS.h.

aggarg · 2021-07-20T18:23:31Z

timers.c

@@ -462,7 +462,7 @@
            }
            else
            {
-                pxTimer->ucStatus &= ~tmrSTATUS_IS_AUTORELOAD;
+                pxTimer->ucStatus &= ( (uint8_t) ~tmrSTATUS_IS_AUTORELOAD );


Suggested change

pxTimer->ucStatus &= ( (uint8_t) ~tmrSTATUS_IS_AUTORELOAD );

pxTimer->ucStatus &= ( ( uint8_t ) ~tmrSTATUS_IS_AUTORELOAD );

aggarg · 2021-07-20T18:26:47Z

timers.c

@@ -550,7 +550,7 @@
        }
        else
        {
-            pxTimer->ucStatus &= ~tmrSTATUS_IS_ACTIVE;
+            pxTimer->ucStatus &= ( (uint8_t) ~tmrSTATUS_IS_ACTIVE );
