Screen set frames update #4043

slash-bit · 2024-06-05T09:34:45Z

Thank you for sending in a pull request, here's some tips to get started!

This PR fixes #4038
every time setFrame run the page reverts back to Message or Module frame , but not the same frame that was viewed by user before, which could be annoying having to manually switch back to say Status frame.
So I have added lines where it would keep the same Frame number that user was one prior to reloading the frames. If that frame still available.
There may be some more enhancements needed in the future to accommodate when total number of frames changes ( like enabling/disabling modules or resetting Node DB.
For now, testing on T-Echo seems reasonably stable and stays on the same frame while device runs.

CLAassistant · 2024-06-05T09:34:51Z

All committers have signed the CLA.

CLAassistant · 2024-06-05T09:34:53Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

caveman99

Just something else to consider. We have pop-up screens especially in the Audio and Canned Messages module (PTT; Message Status, Delivery report and so on...), but also for functionality that is enabled with keypresses. These screens still need to take fcocus, but of course should return to the last screen shown and not the first one

caveman99 · 2024-06-05T09:48:14Z

src/graphics/Screen.cpp

+
+    if (currentFrameNum > numframes - 1) { // If we were on a frame that no longer exists
+        ui->switchToFrame(numframes - 2); // Attempt to return to last frame
+        } 


What if that one doesn't exist as well? i'd rather return to the first frame, there's always one frame.

Ah I think I might have caused some of this confusion. I made a (probably unclear) suggestion that it would be good to store numFrames at the end of the method, so that next time setFrames() is called, we can detect whether we were previously on the log buffer / settings frames, and then move to either (numFrames - 1) or (numFrames - 2), which would handle changes to numFrames.

todd-herbert · 2024-06-05T10:22:48Z

Just something else to consider. We have pop-up screens especially in the Audio and Canned Messages module (PTT; Message Status, Delivery report and so on...), but also for functionality that is enabled with keypresses. These screens still need to take fcocus, but of course should return to the last screen shown and not the first one

I'm not familiar enough with the modules to know exactly what needs to be accommodated, but I can certainly imagine the sort of attention to detail it'd take to catch all those little cases.

Now that you mention this, I wonder if rather than trying to catch every case, it might be better to just target one:

If we changed the signature of setFrames() tosetFrames(bool holdPosition=false), we could apply this new behavior only to "node status updates", in Screen::handleStatusUpdate(), avoiding status updates for text messages, input events, and UI events (from modules)?

Added code to attempt to revert back to the same frame that user was on prior to setFrame reload.

Make screen to revert to Frame 0 if the originally displayed frame is no longer there.

Inserted boolean holdPosition into setFrames to indicate the requirement to stay on the same frame ( if =true) or else it will switch to new frame . Only Screen::handleStatusUpdate calls with setFrame(true). ( For Node Updates) All other types of updates call as before setFrame(), so it will change focus as needed.

slash-bit · 2024-06-12T10:46:58Z

Hi guys, sorry attention drifted. I got your points and thanks @caveman99 and @todd-herbert for attention to detail.
I think I got it . This code is not tested yet, will test it tonight. Will try to enable various modules and send some messages to ensure the focus is changing to the new message. Just wanted your verification of code in a meantime. Thanks
master...slash-bit:meshtastic-echo-echo:screen-setFrames-update

todd-herbert · 2024-06-12T10:53:00Z

Hi guys, sorry attention drifted

Hey no problem, really excited to see some action back on this one!

todd-herbert

I'll send through a PR to slash-bit/meshtastic-echo-echo with some fixes I had handy for this. It also catches one extra case, where the frame changes when user sends a text message.

I don't think these extra changes should disrupt any other modules, but if you were going to do some testing there, that'd be really helpful too.

There's one more spot I'd like to tackle tomorrow, where Screen::setScreensaverFrames() tries to do the same job of returning to the same frame (not as nicely as the code we've been working on here though!). It can totally just use setFrames(true) now instead; I'll test that out on my E-Ink stuff here.

todd-herbert · 2024-06-12T10:59:30Z

src/graphics/Screen.cpp

+    if (holdPosition) {
+        ui->switchToFrame(currentFrameNum); // Attempt to return to same frame after rebuilding the frames,
+        // if holdPosition is true (currently only Screen::handleStatusUpdate calls this
+    } else {
+        continue; // We leave the displayed frame as it is or chnage focuse to new frame


I did have a bit of a play with this a few days ago, and I think there might be a sneaky issue here: if you're trying to stay on the "log buffer" frame, and the number of frames increases, switchToFrame(currentFrameNum) won't put you back in the same place.

todd-herbert · 2024-06-12T11:03:31Z

src/graphics/Screen.cpp

@@ -2054,9 +2054,11 @@ void Screen::setScreensaverFrames(FrameCallback einkScreensaver)
 #endif

 // restore our regular frame list
-void Screen::setFrames()
+void Screen::setFrames(bool holdPosition = false)


Don't forget the one declaration in Screen.h either!
(because of the order of Screen.cpp, I also think you'll want to set that =false default argument in Screen.h instead)

slash-bit · 2024-06-12T15:20:09Z

Great stuff, I think we are getting there!
I have left one comment regarding assignment of oldNumFrames, please have a look.
Will try code on the T-Echo this evening.

Collaboration on meshtastic#4043

slash-bit · 2024-06-13T19:51:46Z

Hey all, I have done some tests on the latest branch.
The code works, but there might be more tweaks needed to polish it.
-The frame holds after status update.

increasing number of frames, ( because new nodes found ( when total number of nodes is below 5) still holds.
Enabling additional module ( Telemetry for example), still holds. Obviously reboot knocks it to frame 0 , but after reboot and frame chosen. It stays.
What can be improved:
For some reason when deleting a node from DB manually, the frame reverts back to 0. Don't quite understand why , as it should be using the same Screen::handleStatusUpdate as with new node added.
Also, when new incoming Message arrives (Screen::handleTextMessage) the screen jumps to frame 0.
The frame 0 could be an additional Module ( like Telemetry) , therefore frame 0 isn't always new message.
I quickly switched to official 2.3.12 firmware and it has the same behavior, so that seems an existing issue.
Secondly, when New Way Point arrives it is suppose to switch to waypoint frame, but it jumps to frame 0.
We can probably fix those issues as well. If that's OK, I can have a go and try to work on setFrames() further , the send you new PR.

todd-herbert · 2024-06-14T11:29:47Z

Just wanted to send a quick reply so I didn't leave you hanging here.
Well spotted with those two new cases! I'd never tried out removing a node manually, or working with waypoints; I bet I would have missed it completely.

I see you've got a fix out for those two. I haven't double checked, but I'm pretty sure devicestate.has_rx_waypoint and devicestate.has_rx_text_message values remain true long after the message has arrived. Any time setFrames() is called, it'll keep returning to those frames as if there was a new message. I feel like someone might want to see new telemetry frames as they arrive though and this might disrupt that. (Edit: hmm, maybe not, but is there any potential for weirdness?)

I looked into it today too, and apparently manually removing a node isnt being done by Screen::handleStatusUpdate(); it's actually being removed using an admin message, in AdminModule.cpp. This isn't triggering the redraw though.. turns out that particular redraw is coming from a bug in the canned messages module (the code for handling canned message ACKs is getting triggered by the ACK for that "remove node" admin message). I think we can probably fix that bug at its source, but I'd like to get some advice tomorrow first from someone who's worked on that module before.

slash-bit · 2024-06-14T14:07:24Z

Yeah, thanks. I suspected something wasn't right with devicestate.has_rx_text_message during limited testing that I have done . Will revert. Then other possible solution maybe to use handleTextMessage as a starting point to trigger switch to a message frame.

firmware/src/graphics/Screen.cpp

Line 2658 in 39c9f92

int Screen::handleTextMessage(const meshtastic_MeshPacket *packet)

I am also not a user of the Waypoint or Telemetry in fact, but just reading commented lines in the code, I understand that's the intention to switch to those frames.
Yes, it ll be nice to clarify about canned message , so we may get that sorted as well. I crack on with looking for a solution, and better stay away from the heated debate in the discord :-)

todd-herbert · 2024-06-14T14:16:10Z

I think if we can mark when a new waypoint arrives (and when a node is being removed manually from the db), we can probably move to the right frames at that point, without having to worry about what other modules might be doing. I've just been working on some code to pass this info through to setFrames, but I haven't written any code inside setFrames to make use of this new info. Do you think if I get this side sorted, at least as a draft version, you'd be able to solve the second part of the puzzle?

Edit: The way I was handling it was over-thinking the whole thing. I don't have this code done tonight sorry, but if you want to have a go at doing the first part too, I was thinking maybe with two new Observables, then passing a "TargetFrame" enum to setFrames, instead of the bool.

todd-herbert · 2024-06-14T16:40:26Z

(Whoops, accidentally pushed here by mistake, when I meant to send it as a PR)

todd-herbert · 2024-06-14T16:58:37Z

One more thing I spotted: I don't think the firmware code handles "deleting a waypoint" yet, at least not on the device.
There's no record in the firmware code of what changes are being made to the waypoint packet to indicate that it has been deleted. I imagine we can look in the app code though and see which fields it's using to trigger this.

slash-bit · 2024-06-14T18:09:15Z

he way I was handling it was over-thinking the whole thing. I don't have this code done tonight sorry, but if you want to have a go at doing the first part too, I was thinking maybe with two new Observables, then passing a "TargetFrame" enum to setFrames, instead of the bool.

:-) Aha , I was thinking the same , pass enum instead of boolean. But then I got distracted by .has_rx_text_message.
Yeah, let me try this. Yes, it will need couple of new variables defined, like txtFrame and wpFrame
they will derive from the amount of modules activated ( I believe modules frames always go first) , so every new module activated will shift txtFrame up by one.

slash-bit · 2024-06-14T18:29:03Z

One more thing I spotted: I don't think the firmware code handles "deleting a waypoint" yet, at least not on the device. There's no record in the firmware code of what changes are being made to the waypoint packet to indicate that it has been deleted. I imagine we can look in the app code though and see which fields it's using to trigger this.

This can be interesting one to find you.

todd-herbert · 2024-06-14T19:10:51Z

Actually, maybe it'd even work to record the value of numframes just before drawTextMessageFrame etc are called?

slash-bit marked this pull request as draft June 5, 2024 09:38

slash-bit mentioned this pull request Jun 5, 2024

[Bug]: Screen reverting back to last message after setFrame reload #4038

Open

caveman99 requested changes Jun 5, 2024

View reviewed changes

caveman99 requested review from thebentern and todd-herbert June 5, 2024 09:50

slash-bit added 3 commits June 12, 2024 09:47

Updated setFrames in Screen.cpp

4409057

Added code to attempt to revert back to the same frame that user was on prior to setFrame reload.

Space added Screen.cpp

38a9f33

Update Screen.cpp

cbd49ed

Make screen to revert to Frame 0 if the originally displayed frame is no longer there.

caveman99 force-pushed the screen-setFrames-update branch from 67f3fcf to cbd49ed Compare June 12, 2024 07:47

todd-herbert added 2 commits June 12, 2024 23:28

Hold position, even if number of frames increases

3f3db83

Hold position, if handling an outgoing text message

64d8cdf

todd-herbert reviewed Jun 12, 2024

View reviewed changes

Merge branch 'master' into screen-setFrames-update

da32505

todd-herbert mentioned this pull request Jun 12, 2024

Collaboration on https://github.com/meshtastic/firmware/pull/4043 slash-bit/meshtastic-echo-echo#1

Merged

Merge pull request #1 from todd-herbert/screen-setFrames-update

3b2febb

Collaboration on meshtastic#4043

Update Screen.cpp

de6e8a1

Reverted chnages related to devicestate.has_rx_text_message

1243987

todd-herbert force-pushed the screen-setFrames-update branch from 984f070 to 1243987 Compare June 14, 2024 16:39

todd-herbert mentioned this pull request Jun 14, 2024

Give setFrames more info slash-bit/meshtastic-echo-echo#2

Closed

todd-herbert mentioned this pull request Jun 16, 2024

Show compass on waypoint frame; clear when waypoint deleted #4116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Screen set frames update #4043

Screen set frames update #4043

slash-bit commented Jun 5, 2024

CLAassistant commented Jun 5, 2024 •

edited

Loading

CLAassistant commented Jun 5, 2024

caveman99 left a comment

caveman99 Jun 5, 2024

todd-herbert Jun 5, 2024 •

edited

Loading

todd-herbert commented Jun 5, 2024 •

edited

Loading

slash-bit commented Jun 12, 2024

todd-herbert commented Jun 12, 2024

todd-herbert left a comment

todd-herbert Jun 12, 2024

todd-herbert Jun 12, 2024

slash-bit commented Jun 12, 2024

slash-bit commented Jun 13, 2024

todd-herbert commented Jun 14, 2024 •

edited

Loading

slash-bit commented Jun 14, 2024 •

edited

Loading

todd-herbert commented Jun 14, 2024 •

edited

Loading

todd-herbert commented Jun 14, 2024 •

edited

Loading

todd-herbert commented Jun 14, 2024

slash-bit commented Jun 14, 2024

slash-bit commented Jun 14, 2024

todd-herbert commented Jun 14, 2024

Screen set frames update #4043

Are you sure you want to change the base?

Screen set frames update #4043

Conversation

slash-bit commented Jun 5, 2024

Thank you for sending in a pull request, here's some tips to get started!

CLAassistant commented Jun 5, 2024 • edited Loading

CLAassistant commented Jun 5, 2024

caveman99 left a comment

Choose a reason for hiding this comment

caveman99 Jun 5, 2024

Choose a reason for hiding this comment

todd-herbert Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

todd-herbert commented Jun 5, 2024 • edited Loading

slash-bit commented Jun 12, 2024

todd-herbert commented Jun 12, 2024

todd-herbert left a comment

Choose a reason for hiding this comment

todd-herbert Jun 12, 2024

Choose a reason for hiding this comment

todd-herbert Jun 12, 2024

Choose a reason for hiding this comment

slash-bit commented Jun 12, 2024

slash-bit commented Jun 13, 2024

todd-herbert commented Jun 14, 2024 • edited Loading

slash-bit commented Jun 14, 2024 • edited Loading

todd-herbert commented Jun 14, 2024 • edited Loading

todd-herbert commented Jun 14, 2024 • edited Loading

todd-herbert commented Jun 14, 2024

slash-bit commented Jun 14, 2024

slash-bit commented Jun 14, 2024

todd-herbert commented Jun 14, 2024

CLAassistant commented Jun 5, 2024 •

edited

Loading

todd-herbert Jun 5, 2024 •

edited

Loading

todd-herbert commented Jun 5, 2024 •

edited

Loading

todd-herbert commented Jun 14, 2024 •

edited

Loading

slash-bit commented Jun 14, 2024 •

edited

Loading

todd-herbert commented Jun 14, 2024 •

edited

Loading

todd-herbert commented Jun 14, 2024 •

edited

Loading