Collaboration on https://github.com/meshtastic/firmware/pull/4043 #1
Conversation
| @@ -2150,13 +2151,23 @@ void Screen::setFrames(bool holdPosition = false) | |||
|
|
|||
| setFastFramerate(); // Draw ASAP | |||
|
|
|||
| // In some situations, we'd like to return to the same frame | |||
| static size_t oldNumFrames = numframes; | |||
There was a problem hiding this comment.
I am not sure of assigning oldNumFrames = numframes here. As we have already reset the value of numframes to 0 in row 2085, calculated new value in 2093 ?
I think we just declare oldNumFrames. And assign value of numframes at the end in row 2169 , to use it next time ?
There was a problem hiding this comment.
Actually, I do see what you're getting at there.
Just to be clear: because it's static, it will only be declared and initialized the very first time that method is called. Every other time, it will preserve the value from the end of the previous call.
The very first time that method is called though, you're right, it will be initialized and then set again with the same value. In theory, the first setFrames call should always be setFrames(false), so it would probably be okay to declare it with no initial value, and wait for it to be set at the end of that first call.
That being said, it is nice that it's being initialized before that if(holdPosition) block, just in case the very first call to setFrames is somehow setFrames(true). There's probably no reason it couldn't be initialised with some fixed value like 2 instead. In theory that initial value will never be used; it'd just be insurance against some unexpected weirdness.
Honestly, I didn't put this much thought into it when I initially wrote that line. I think I probably moved it from somewhere else without realizing that it's a bit redundant now.
If you wanted to be really diligent, you could even initialize it as 0, and then assert(oldNumFrames) at the start of that if(holdPosition) block, to throw an error if it does somehow get used before it's meant to, although that might be overkill.
Whatever suits your sense of design!
| ui->switchToFrame(oldFrame); // Otherwise, go back to the same frame | ||
| } | ||
| oldNumFrames = numframes; // Store how many frames we have, in case we want to "restore position" next time | ||
| } |
There was a problem hiding this comment.
That logic is clear now .
Will flash all to my T-Echo tonight and test.
(b^_^)b
A few possible fixes to try out for meshtastic#4043 (review)