Prevents blocking when Infineat watchface is charging #2256

SteveAmor · 2025-02-18T21:13:41Z

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

github-actions · 2025-02-18T21:17:18Z

Build size and comparison to main:

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

Run in InfiniEmu

SteveAmor · 2025-02-18T21:39:17Z

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?

NeroBurner · 2025-02-19T09:16:32Z

src/displayapp/screens/WatchFaceInfineat.cpp

@@ -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


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?

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.

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.

mark9064 · 2025-02-23T11:00:52Z

src/displayapp/screens/WatchFaceInfineat.cpp

@@ -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


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)

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.

mark9064 · 2025-02-23T11:01:31Z

src/displayapp/screens/WatchFaceInfineat.h

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

      private:
        uint32_t savedTick = 0;
+        uint32_t savedAnimationTick = 0;


This would become TickType_t.

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

I think/hope the changes I made address this?

mark9064 · 2025-02-23T17:29:46Z

src/displayapp/screens/WatchFaceInfineat.cpp

-  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


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

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 · 2025-02-23T21:20:56Z

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

src/displayapp/screens/WatchFaceInfineat.cpp

mark9064

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 · 2025-02-24T19:44:20Z

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.

NeroBurner

Looking good. Thanks!

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)

SteveAmor mentioned this pull request Feb 18, 2025

Return to Infineat watch face crashes when charging #2245

Closed

1 task

mark9064 added the bug Something isn't working label Feb 19, 2025

mark9064 self-requested a review February 19, 2025 00:35

NeroBurner added this to the 1.16.0 milestone Feb 19, 2025

NeroBurner reviewed Feb 19, 2025

View reviewed changes

Prevent blocking when Infineat watchface is charging

7198b5b

SteveAmor force-pushed the infineat-ani branch from a1e3292 to 7198b5b Compare February 19, 2025 17:55

mark9064 reviewed Feb 23, 2025

View reviewed changes

Use RTOS timers for annimation

45f1ec7

mark9064 reviewed Feb 24, 2025

View reviewed changes

src/displayapp/screens/WatchFaceInfineat.cpp Show resolved Hide resolved

SteveAmor added 3 commits February 24, 2025 14:51

Adds comment on height of animation

de7238a

Fixes clang formatting error for comment

4c822e8

Fixes comments

9a06545

mark9064 approved these changes Feb 24, 2025

View reviewed changes

NeroBurner approved these changes Feb 25, 2025

View reviewed changes

mark9064 merged commit 728da0f into InfiniTimeOrg:main Feb 26, 2025
7 checks passed

mark9064 mentioned this pull request Feb 26, 2025

Prevent blocking when Infineat watchface is charging #2248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevents blocking when Infineat watchface is charging #2256

Prevents blocking when Infineat watchface is charging #2256

SteveAmor commented Feb 18, 2025 •

edited

Loading

github-actions bot commented Feb 18, 2025 •

edited

Loading

SteveAmor commented Feb 18, 2025 •

edited

Loading

NeroBurner Feb 19, 2025

SteveAmor Feb 19, 2025 •

edited

Loading

SteveAmor Feb 22, 2025

mark9064 Feb 23, 2025

SteveAmor Feb 23, 2025

mark9064 Feb 23, 2025

SteveAmor Feb 23, 2025

mark9064 Feb 23, 2025

SteveAmor Feb 23, 2025 •

edited

Loading

SteveAmor commented Feb 23, 2025

mark9064 left a comment

SteveAmor commented Feb 24, 2025

NeroBurner left a comment

Prevents blocking when Infineat watchface is charging #2256

Prevents blocking when Infineat watchface is charging #2256

Conversation

SteveAmor commented Feb 18, 2025 • edited Loading

github-actions bot commented Feb 18, 2025 • edited Loading

SteveAmor commented Feb 18, 2025 • edited Loading

NeroBurner Feb 19, 2025

Choose a reason for hiding this comment

SteveAmor Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

SteveAmor Feb 22, 2025

Choose a reason for hiding this comment

mark9064 Feb 23, 2025

Choose a reason for hiding this comment

SteveAmor Feb 23, 2025

Choose a reason for hiding this comment

mark9064 Feb 23, 2025

Choose a reason for hiding this comment

SteveAmor Feb 23, 2025

Choose a reason for hiding this comment

mark9064 Feb 23, 2025

Choose a reason for hiding this comment

SteveAmor Feb 23, 2025 • edited Loading

Choose a reason for hiding this comment

SteveAmor commented Feb 23, 2025

mark9064 left a comment

Choose a reason for hiding this comment

SteveAmor commented Feb 24, 2025

NeroBurner left a comment

Choose a reason for hiding this comment

SteveAmor commented Feb 18, 2025 •

edited

Loading

github-actions bot commented Feb 18, 2025 •

edited

Loading

SteveAmor commented Feb 18, 2025 •

edited

Loading

SteveAmor Feb 19, 2025 •

edited

Loading

SteveAmor Feb 23, 2025 •

edited

Loading