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

Prevents blocking when Infineat watchface is charging #2256

Merged
merged 5 commits into from
Feb 26, 2025

Conversation

SteveAmor
Copy link
Contributor

@SteveAmor SteveAmor commented Feb 18, 2025

This PR prevents the blocking issue identified in #2245
This is a better solution than the one proposed in #2248

Copy link

github-actions bot commented Feb 18, 2025

Build size and comparison to main:

Section Size Difference
text 373088B 64B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

@SteveAmor
Copy link
Contributor Author

SteveAmor commented Feb 18, 2025

I'm all for clean code and keeping things tidy but I don't like the way clang wants to right justify my comment, about the magic number 3, with the previous charging battery animation comment. The comment // 100% / pine_small height = 3 is pushed very far to the right of the code is it providing the comment for.

if (batteryController.IsCharging() && (uint32_t(lv_tick_get() - savedAnimationTick) > 150)) { // Charging battery animation
chargingBatteryPercent += 3; // 100% / pine_small height = 3

It's due to AlignTrailingComments = true
But, if I have to make the change I will, perhaps this works well for some IDEs?

@mark9064 mark9064 added the bug Something isn't working label Feb 19, 2025
@mark9064 mark9064 self-requested a review February 19, 2025 00:35
@NeroBurner NeroBurner added this to the 1.16.0 milestone Feb 19, 2025
@@ -434,12 +434,13 @@ void WatchFaceInfineat::Refresh() {

batteryPercentRemaining = batteryController.PercentRemaining();
isCharging = batteryController.IsCharging();
if (batteryController.IsCharging()) { // Charging battery animation
chargingBatteryPercent += 1;
if (batteryController.IsCharging() && (lv_tick_get() - savedAnimationTick > 150)) { // Charging battery animation
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when the tick counter wrapps around. Then the tick count is smaller than the saved tick count. What would be the resulting behaviour?

Copy link
Contributor Author

@SteveAmor SteveAmor Feb 19, 2025

Choose a reason for hiding this comment

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

Good point. I need to add a cast to fix this (uint32_t(lv_tick_get() - savedAnimationTick) > 150)
Now, wrap around shouldn't be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I need to add a cast to fix this (uint32_t(lv_tick_get() - savedAnimationTick) > 150) Now, wrap around shouldn't be a problem.

I tested on the simulator with and without the uint32_t and the compiler applies the same modulo arithmetic for unsigned ints to either version with no change in the complied flash and ram sizes. However, it's probably safer to leave uint32_t in.

@@ -434,12 +434,13 @@ void WatchFaceInfineat::Refresh() {

batteryPercentRemaining = batteryController.PercentRemaining();
isCharging = batteryController.IsCharging();
if (batteryController.IsCharging()) { // Charging battery animation
chargingBatteryPercent += 1;
if (batteryController.IsCharging() && (uint32_t(lv_tick_get() - savedAnimationTick) > 150)) { // Charging battery animation
Copy link
Member

Choose a reason for hiding this comment

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

Though they're the same, for timing it's recommended to use the RTOS tick with xTaskGetTickCount(). Additionally no type casts are needed, the overflow semantics of unsigned integers are well defined and subtraction works as expected. i.e just xTaskGetTickCount() - savedAnimationTick > pdMS_TO_TICKS(150) (ms to ticks is recommended as the tick rate may change in the future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @mark9064. I have made the changes and it works as expected in terms of the same timings as the original code and as per the first commit in this PR.

@@ -46,6 +46,7 @@ namespace Pinetime {

private:
uint32_t savedTick = 0;
uint32_t savedAnimationTick = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This would become TickType_t.

I think it would be better if the variable name was specific that it's the charging animation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think/hope the changes I made address this?

if (batteryController.IsCharging()) { // Charging battery animation
chargingBatteryPercent += 1;
if (batteryController.IsCharging() && (uint32_t(lv_tick_get() - savedAnimationTick) > 150)) { // Charging battery animation
chargingBatteryPercent += 3; // 100% / pine_small height = 3
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use 100 / lv_obj_get_height(logoPine)?
I think it'd be worth checking that LVGL does report what you'd expect
This would give a constant fillrate of 1 pixel per 150ms regardless of the pine cone size

Copy link
Contributor Author

@SteveAmor SteveAmor Feb 23, 2025

Choose a reason for hiding this comment

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

This does appear to provide the expected value. I didn't know if I should type cast the result to an uint8_t? OK, no need given the division of two integer type values will result in an integer and chargingBatteryPercent is a uint8_t

@SteveAmor
Copy link
Contributor Author

@mark9064 let me know if any more changes are required. If not, would you would like me to squash the commits?

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Thanks for the report and fix!

I think this is good to go, my only criticism would be is that when it's almost full the battery indicator might flicker in some way. But I think it did this before, so it's not a regression. How has it looked in your testing?

@SteveAmor
Copy link
Contributor Author

I think this is good to go, my only criticism would be is that when it's almost full the battery indicator might flicker in some way. But I think it did this before, so it's not a regression. How has it looked in your testing?

On the PT hardware, it looks identical to the animation in V1.15 and the animation when the battery is almost full is also the same. It's quite tolerable, not flashing the last pixel or two too fast. You can see it's charging when you're at 90+ percent.

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.

Looking good. Thanks!

@mark9064 mark9064 merged commit 728da0f into InfiniTimeOrg:main Feb 26, 2025
7 checks passed
dariusarnold pushed a commit to dariusarnold/InfiniTime that referenced this pull request Mar 2, 2025
Optimise the battery animation to not use 100% CPU (which causes DisplayApp to spin forever with AOD)
(DisplayApp also needs to be fixed in the future so it cannot spin infinitely)
BurninTurtles pushed a commit to BurninTurtles/InfiniTimeBT that referenced this pull request Mar 3, 2025
Optimise the battery animation to not use 100% CPU (which causes DisplayApp to spin forever with AOD)
(DisplayApp also needs to be fixed in the future so it cannot spin infinitely)
BurninTurtles pushed a commit to BurninTurtles/InfiniTimeBT that referenced this pull request Mar 3, 2025
Optimise the battery animation to not use 100% CPU (which causes DisplayApp to spin forever with AOD)
(DisplayApp also needs to be fixed in the future so it cannot spin infinitely)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants