search

meshtastic / firmware

Meshtastic device firmware
https://meshtastic.org
GNU General Public License v3.0
2.97k stars 706 forks source link

Screen set frames update #4043

Open slash-bit opened 3 weeks ago

slash-bit commented 3 weeks ago

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 commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 3 weeks ago

CLA assistant check
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.

todd-herbert commented 3 weeks ago

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)?

slash-bit commented 2 weeks ago

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 https://github.com/meshtastic/firmware/compare/master...slash-bit:meshtastic-echo-echo:screen-setFrames-update

todd-herbert commented 2 weeks ago

Hi guys, sorry attention drifted

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

slash-bit commented 2 weeks ago

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.

slash-bit commented 1 week ago

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.

todd-herbert commented 1 week ago

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 commented 1 week ago

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. https://github.com/meshtastic/firmware/blob/39c9f92c6e7ee1639051a967d761ed5d642f4fcd/src/graphics/Screen.cpp#L2658 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 commented 1 week ago

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 commented 1 week ago

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

todd-herbert commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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