Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an hour option to the timer application #2243

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JustScott
Copy link
Contributor

This adds an hours column to the timer app.

Addresses: #1678

Technical Details:

I thought this would be a simple task until I found that xTimerChangePeriod doesn't support periods longer than an hour and 10ish minutes (1hr 9m 54s is what I found). So my solution to this was to divide the hours (referred to as intervals in the code) out of the duration given when the timer is started and store it in timerOverflowIntervals, then set the period in xTimerChangePeriod to the leftover duration (an hour). Then when Messages::TimerDone: is triggered, it checks if any hours/intervals are stored, if there are then it starts a new hour timer and subtracts an hour/interval from timerOverflowIntervals. This allows for setting very large timers (256 hours) with the timer component without making the system hold large tick/millisecond values (however the timer app only allows up to 100 hours).

I may also have missed something simple with xTimerChangePeriod and way over engineered this, in which case this was a great learning exercise. Anyways, let me know what you think. To code reviewers: I'm a C++ novice so please go easy :).

Not sure why the GIF appears sped up, but the timer counts down seconds normally in the simulator.
show_timer

Copy link

github-actions bot commented Feb 5, 2025

Build size and comparison to main:

Section Size Difference
text 373376B 352B
data 948B 0B
bss 22544B 8B

Run in InfiniEmu

@mark9064
Copy link
Member

mark9064 commented Feb 7, 2025

Thanks for the feature! I have a feeling xTimerChangePeriod works fine for any tick count, but using pdMS_TO_TICKS isn't suitable as it causes an integer overflow. This macro is defined as #define pdMS_TO_TICKS( xTimeInMs ) ( ( TickType_t ) ( ( ( TickType_t ) ( xTimeInMs ) * ( TickType_t ) configTICK_RATE_HZ ) / ( TickType_t ) 1000 ) ), which minus the awkward type casts is (xTimeInMs*configTICK_RATE_HZ)/1000, and the tick rate is 1024Hz. The time you found as the maximum working is 4194000ms, which when multiplied by 1024 is 4294656000 - almost exactly 2^32! The solution to this is to not use pdMS_TO_TICKS as you have an integer number of seconds. To convert seconds to ticks, multiply the number of seconds by configTICK_RATE_HZ. This way the longest possible timer is 2^32-1 ticks which equals 4194304s, which is 48 days. Should be enough :)

@mark9064 mark9064 added the enhancement Enhancement to an existing app/feature label Feb 7, 2025
@JustScott
Copy link
Contributor Author

@mark9064 After passing the product of duration.count() in seconds multiplied by configTICK_RATE_HZ to xTimerChangePeriod, the timer is still only setting to a maximum of 1h 9m 54s, I even tried just hard coding it xTimerChangePeriod(timer, 4199 * configTICK_RATE_HZ, 0); but this just overflowed as well... which leads me to believe xTimerChangePeriod is the problem, unless there's something wrong with TickType_t remainingTime = xTimerGetExpiryTime(timer) - xTaskGetTickCount(); in GetTimeRemaining, because I checked the ticks returned by this and its showing the same issue there as well (I even tried using uint32_t and uint64_t instead of TickType_t incase that was the issue). I tried looking around the FreeRTOS kernel but was struggling to understand it well enough to find the issue. Perhaps even if we can find a way to set this to go for the 48 hours you mentioned above, it would still be better to go with an approach like mine of restarting the timer according to a number of intervals, allowing us to set much longer timers (easily increasing the limit by changing timerOverflowIntervals from uint8_t).

@mark9064
Copy link
Member

mark9064 commented Feb 7, 2025

Well in GetTimeRemaining it also multiplies by 1000 which will cause overflow. Basically all code paths involving it will need to be refactored to make sure they don't multiply tick counts beyond the range of uint32. To fix this one, you could split building the time into milliseconds and seconds (seconds with tick count / tick rate, milliseconds with tick count % tick rate * 1000 / tick rate)

@codeguylevi
Copy link

I asked about the same thing!

@JustScott
Copy link
Contributor Author

JustScott commented Feb 18, 2025

I asked about the same thing!

@codeguylevi About what, the timer limit issue I'm having?

@codeguylevi
Copy link

I asked about the same thing!

@codeguylevi About what, the timer limit issue I'm having?

Yes.

@mark9064
Copy link
Member

Yes.

You mean the timer currently being limited to 59:59 right?

@mark9064
Copy link
Member

Also Scott if you'd like me to take a look at the overflow issue I'd be happy to - if so let me know exactly what you've tried so far re removing pdMS_TO_TICKS etc.

@codeguylevi
Copy link

Yes.

You mean the timer currently being limited to 59:59 right?

Yup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants