FreeRTOS / FreeRTOS-Kernel

FreeRTOS kernel files only, submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos.
https://www.FreeRTOS.org
MIT License
2.76k stars 1.12k forks source link

Add pdTICKS_TO_MS macro #866

Closed Dazza0 closed 11 months ago

Dazza0 commented 1 year ago

Description

This PR adds a simple pdTICKS_TO_MS() convenience macro to convert time in ticks to time in milliseconds.

Test Steps

Checklist:

Related Issue

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

htibosch commented 1 year ago

I just tested my proposed macro on a 32-bit platform. It works as expected, although for a complete test, you'd have to adapt pdMS_TO_TICKS() as well:

-    #define pdMS_TO_TICKS( xTimeInMs )    ( ( TickType_t ) ( ( ( TickType_t ) ( xTimeInMs )   * ( TickType_t ) configTICK_RATE_HZ ) / ( TickType_t ) 1000U ) )
+    #define pdMS_TO_TICKS( xTimeInMs )    ( ( TickType_t ) ( ( ( uint64_t ) ( xTimeInMs )   * ( uint64_t ) configTICK_RATE_HZ ) / ( uint64_t ) 1000U ) )

I tested with a tick rate of 500 Hz.

Dazza0 commented 12 months ago

@htibosch Thanks for the quick review. I've update both pdTICKS_TO_MS() and pdMS_TO_TICKS() to use a uint64_t cast.

codecov[bot] commented 12 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0640b2e) 93.51% compared to head (acc71c7) 93.51%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #866 +/- ## ======================================= Coverage 93.51% 93.51% ======================================= Files 6 6 Lines 3175 3175 Branches 883 883 ======================================= Hits 2969 2969 Misses 99 99 Partials 107 107 ``` | [Flag](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/866/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/FreeRTOS/FreeRTOS-Kernel/pull/866/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS) | `93.51% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=FreeRTOS#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aggarg commented 11 months ago

Given that we now have a way to make TickType_t 64-bit, do we need this to be explicitly uint64_t?

I think that we do not and TickType_t should be good. Those who need wider TickType_t can do so in their FreeRTOSConfig.h. What do you think @Dazza0, @htibosch ?

htibosch commented 11 months ago

The problem is that it may take a lot of time before one discovers that mentioned macro is overflowing. I am one of them: I couldn't understand why the timestamps in my logging were erroneous.

And I can imagine that someone choses TICK_TYPE_WIDTH_32_BITS and conversion macro's that do not overflow in 1.5 day.

sonarcloud[bot] commented 11 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication