Bug report for WinAVR Mega323 demo

Kernel
1

I’m a first-timer in this forum, so please bear with me if I’m asking about something well known since years, or if this is the completely wrong place at all. I’d like to report a bug within one of the AVR demos that has the potential of exist in every port where the Tick counter is only 16 bits wide (configUSE_16_BIT_TICKS = 1).

Some days ago I started playing around with FreeRTOS on an Mega32 AVR and the AVR_ATMega323_WinAVR demo. After adapting the clock speed and remapping the LEDs from Port B (where the SPI interfase resides) to Port C, most of the tasks did run successfully. The 3-second check routine prvCheckOtherTasksAreStillRunning() however did fail due to xArePollingQueuesStillRunning() failing, because the activity counter xPollingConsumerCount didn’t change within the 3-second test interval. Instead, the consumer task is running only about once a minute.

Further testing showed that the producer task did run fine but experienced a Queue Full condition, a second proof that the consumer task didn’t run at the expected intervals.

Now, both tasks are calling vTaskDelay before looping again with parameters pollqPRODUCER_DELAY and pollqCONSUMER_DELAY, which are constants defined as follows:

#define pollqPRODUCER_DELAY ( pdMS_TO_TICKS( ( TickType_t ) 200 ) )
#define pollqCONSUMER_DELAY ( pollqPRODUCER_DELAY - ( TickType_t ) ( 20 / portTICK_PERIOD_MS ) )

#define portTICK_PERIOD_MS ((TickType_t) 1000 / configTICK_RATE_HZ )
#define pdMS_TO_TICKS(xTimeInMs) ((TickType_t) (((TickType_t) (xTimeInMs) * (TickType_t) configTICK_RATE_HZ) / (TickType_t) 1000U))

Therefore, the Producer task should run every 200 ms, the Consumer task should run every 200 - 20 = 180 ms.

Now, expanding the pollqCONSUMER_DELAY definition does the following 200 * 1000 / 1000 - 20 / 1.
Due to the casts, this is performed in 16bit math:
200 * 1000 mod 65536 = 3392
3392 / 1000 = 3
3 - 20 = -17
-17 in 16bits = 65536 - 17 = 65519
65519 Ticks to wait is just the initially observed “about a minute”.

Changing the definition of pdMS_TO_TICKS as follows resolves the 16 bit truncation error:
#define pdMS_TO_TICKS(xTimeInMs) ((TickType_t) (((uint32_t) (xTimeInMs) * configTICK_RATE_HZ) / 1000U))

Is this something already known to the maintainers of the demos? This part of the demo could have never run with this bug. Plus, as said above, this bug could potentially pop up in any implementation with Ticks being held in a 16bit variable.

Please excuse if this was a bit long-winded, but I wanted to provide a conclusive error analysis and solution. And if this is all old news, just feel free to delete this post completely.

Thanks,
– Thilo

2

Thanks for taking the time for such a detailed report.

There is unfortunately not a single way of defining pdMS_TO_TICKS that covers all cases without generating compiler warnings so FreeRTOS just provides a default definition. The default only gets used if you don’t define your own: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/include/projdefs.h#L39 If you need to override the default to make it correct for your implementation then add your own definition into FreeRTOSConfig.h.

It does look like the WinAVR demo is delivered with configUSE_16_BIT_TICKS set to 1, so that needs to be corrected by either setting it to 0 or providing a different pdMS_TO_TICKS definition - so please open a bug report here: https://github.com/FreeRTOS/FreeRTOS/issues - you can link to this forum post from the bug report.

3

Hello Thilo and Richard,

In my case, GNURL78 compiler, Renesas CC-RL compiler, recent IAR ICCRL78 compiler also cause the same issue. (On the other hand, it seems that very old IAR ICCRL78 compiler didn’t cause it according to the official FreeRTOS source code. Or, in the past, the definition of pdMS_TO_TICKS might be different from now.) I avoid it by adding the following definition to FreeRTOSConfig.h which is basically the same definition as yours.

#if (configUSE_16_BIT_TICKS == 1)
#define pdMS_TO_TICKS( xTimeInMs ) ( ( TickType_t ) ( ( ( uint32_t ) ( xTimeInMs ) * ( uint32_t ) configTICK_RATE_HZ ) / ( uint32_t ) 1000 ) )
#endif

This report may help you to post an issue to FreeRTOS’s GitHub repository.

Best regards,
NoMaY

4

Thanks both for your very informative replies.
@Rick: well, on a small micro, I wouldn’t want to suffer from configUSE_16_BIT_TICKS setting to 0 just to avoid the 16bit math in the tick calculation. My intention was mainly to fix that bug in the demo, which could be easily done in the changed definition of pdMS_TO_TICKS, as NoMaY suggested (and I used as well).
But I don’t quite get why a generally changed definition of pdMS_TO_TICKS wouldn’t provide an improvement for everyone. Using 32bit math until the final cast to TickType_t applies should at least perform the correct math and end up with a correct tick value. If this comes at the cost of a compiler warning, well, then even better, since the programmer will then get an additional note that he might need to look closer at his code - but you’d guarantee the compiler will calculate correct values.

Does that make sense?

Thanks again to both of you! Nice place here :wink:
– Thilo

5

Generally we try to avoid compiler warnings, but I suppose here there is scope to use the configUSE_16_BIT_TICKS setting to switch between two different pdMS_TO_TICKS() implementations.