You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The current Quick Settings lv_task period is 5000 msecs.
With a screen timeout of 5 seconds, you only just see the charge icon appear in the status bar at the top of the watchface.
This PR reduces the period to 100 msecs so as soon as you put it on the charger, the charge icon appears. And when you take it off the charger, it quickly disappears.
There are a couple of other screens which this charging UX is poor (it's just not "snappy"):
Tile.cpp
BatteryInfo.cpp
If you're feeling up to it, I think a review of every screen not using LV_DISP_DEF_REFR_PERIOD would be good step forward in terms of improving responsiveness. There's probably a reason for the slow refreshes in some places - maybe performance of other things on the screen, maybe designs that don't use DirtyValue and therefore redraw things every frame, maybe bugs which have long been fixed, who knows! In most cases I think updating every refresh should be fine as there should be no drawing to do if nothing has changed (but some refactoring may be needed to allow this e.g if DirtyValue needs to be added)
I've been getting my head around the drawing of each screen and the lv_task callbacks and will try to have a further look at a general review.
There appears to be an intention in doc/code/Apps.md but no code, that I can see, uses Refresh() as the callback.
Sounds good, feel free to ask any questions (here or the pinetime-dev chat channel). I'm happy to explain general things too if an overview on a part would be useful
How's it going? I'm happy to look at reviewing just this change for now and equally happy with waiting if you're planning on reviewing more places where the refresh time is long, just let me know :)
How's it going? I'm happy to look at reviewing just this change for now and equally happy with waiting if you're planning on reviewing more places where the refresh time is long, just let me know :)
Hi @mark9064 I've not been able to progress this to look at all of the code. It is on my mind that I really should but it won't be for a while.
To better understand the code, I reviewed every lv_task_create and found the following:
There is a mixture of taskUpdate = lv_task_create, taskRefresh = lv_task_create and refreshTask = lv_task_create. Suggestion is to refactor all to the most common which is taskRefresh = lv_task_create.
There are a number of instances where the Refresh() method is called immediately after the refresh task is created. Where the refresh period is 20msec, it's pretty pointless performing this Refresh() and it should be removed. eg.
Where possible, modify instances where a magic number is used instead of LV_DISP_DEF_REFR_PERIOD. As above, this may require the use of the dirty value utility.
Some apps do not follow the design pattern used for lv_task_create(RefreshTaskCallback, which uses the Refresh() method. Can these apps be updated to the pattern all other apps use?
Thanks a lot for going through and reviewing them all :)
sounds great!
id like to investigate this actually. does calling Refresh in the initialisation mean that all of the displayed content is correct immediately rather than having placeholder values while the screen scrolls into view? i have a suspicion this is the case but i haven't tested it. if my hunch is right, i think it makes sense to always call Refresh before creating the refresh task - maybe we could bake that in somehow (or just have it as convention if inconvenient)
sounds good to me
yeah agree, having just one design pattern makes sense
id like to investigate this actually. does calling Refresh in the initialisation mean that all of the displayed content is correct immediately rather than having placeholder values while the screen scrolls into view? i have a suspicion this is the case but i haven't tested it. if my hunch is right, i think it makes sense to always call Refresh before creating the refresh task - maybe we could bake that in somehow (or just have it as convention if inconvenient)
I'd not thought about that. I'll run a quick test on a few watchfaces to see the impact on the UX.
id like to investigate this actually. does calling Refresh in the initialisation mean that all of the displayed content is correct immediately rather than having placeholder values while the screen scrolls into view? i have a suspicion this is the case but i haven't tested it. if my hunch is right, i think it makes sense to always call Refresh before creating the refresh task - maybe we could bake that in somehow (or just have it as convention if inconvenient)
Tested on the simulator as it's pretty slow on my machine so would exaggerate any UX problems and you're absolutely right @mark9064 the Refresh() is required to fully update the WatchFaces, ready for when they scroll into view. I moved the Refresh() before the refresh task is created and it didn't appear to make any difference to the UX, so is ok to be left as it is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current Quick Settings lv_task period is 5000 msecs.
With a screen timeout of 5 seconds, you only just see the charge icon appear in the status bar at the top of the watchface.
This PR reduces the period to 100 msecs so as soon as you put it on the charger, the charge icon appears. And when you take it off the charger, it quickly disappears.
There are a couple of other screens which this charging UX is poor (it's just not "snappy"):
Tile.cpp
BatteryInfo.cpp