-
-
Notifications
You must be signed in to change notification settings - Fork 982
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
Conversation
Build size and comparison to main:
|
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
It's due to |
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a1e3292
to
7198b5b
Compare
@@ -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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@mark9064 let me know if any more changes are required. If not, would you would like me to squash the commits? |
There was a problem hiding this 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?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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)
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)
This PR prevents the blocking issue identified in #2245
This is a better solution than the one proposed in #2248