Skip to content

Refactors watch faces. Replace lv_tick_get() with xTaskGetTickCount() #2264

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

Merged
merged 1 commit into from
May 23, 2025

Conversation

SteveAmor
Copy link
Contributor

Tested on hardware and all timings are the same.
Also saved some flash memory.

Copy link

Build size and comparison to main:

Section Size Difference
text 373056B -32B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

@mark9064 mark9064 added the maintenance Background work label Mar 1, 2025
@mark9064 mark9064 added this to the 1.16.0 milestone May 16, 2025
@JF002
Copy link
Collaborator

JF002 commented May 19, 2025

@SteveAmor What's the rational behind this change? I'm not sure I understand why we should use xTaskGetTickCount() instead of lv_tick_get() in apps.

@mark9064
Copy link
Member

They're the same tick counter, and I think it's confusing to have two. I think it makes sense to unify on one tick counter API, and the system tick counter is the better choice of the two as it's used everywhere else. Important things to know about the tick counter such as its bit width, signed-ness (and therefore wraparound semantics) and tick rate are well known and defined for the system tick counter

@JF002
Copy link
Collaborator

JF002 commented May 21, 2025

Since apps are mostly based on LVGL, I think that it would make more sense to use LVGL API as much as possible, and reduce the dependencies to other libs as much as possible.
In this case, lv_tick_get() effectively abstract FreeRTOS which makes the code more portable and consistent.

@mark9064
Copy link
Member

I agree, but I think simplicity is more important than perfect coupling

In this case, we will need to implement a millisecond level timer, as LVGL only works in milliseconds and currently we provide it FreeRTOS ticks (1024Hz rather than 1000Hz)

@JF002
Copy link
Collaborator

JF002 commented May 21, 2025

Yes OK, that makes sense 👍 . I guess we can still hide FreeRTOS in a utility class that do all the calculations if we need that kind of abstraction in the future.

Copy link
Contributor

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to use one timing related API makes things clearer. And if we use only one then that is the one that will get used by other contributors.
And for me personally I had more exposure to xTaskGetTickCount() than to lv_tick_get(). So just from a feeling perspective I would go with xTaskGetTickCount as well.

Changes look sane

@mark9064 mark9064 merged commit 9fb35cc into InfiniTimeOrg:main May 23, 2025
7 checks passed
@mark9064 mark9064 mentioned this pull request Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Background work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants