Skip to content
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

Add persistent message storage and message log frame #3784

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

ndnestor
Copy link

@ndnestor ndnestor commented May 4, 2024

Added

  • Persistent message storage
  • Message RAM cache
  • Message log UI frame
  • Message protobuf

Changed

  • Trunk version to latest
  • Trunk linters' versions to latest
  • Touchscreen tap and double tap input events

Removed

  • Text message frame that only showed last received message

Video Demonstration

Here is a video demonstration of changes made. It is slightly outdated.

Recommended Future Changes

  • Replace current ACK UI frame with a symbol that is inline with the ACK'd message
  • Add "default auto-focus state" toggle to settings
  • Add "delete all messages" button to settings
  • Add setting to change how often messages are saved
  • Use ostream for more efficient string building in drawMessageLogFrame
  • Add external storage support (e.g. SD card)
  • Only run through message log frame drawing logic when what is drawn needs to be changed
  • Warn user when running out of storage
  • Delete only the oldest message when running out of storage

@CLAassistant
Copy link

CLAassistant commented May 4, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

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.

@ndnestor ndnestor marked this pull request as draft May 4, 2024 22:47
Copy link
Contributor

@todd-herbert todd-herbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Do you have any plans to implement some of the features (cycle through channel?) for devices which only have a single user-button?

src/graphics/Screen.cpp Show resolved Hide resolved
src/graphics/Screen.cpp Show resolved Hide resolved
@GUVWAF
Copy link
Member

GUVWAF commented May 5, 2024

Very nice.

On which devices/screens has this been tested next to the T-Deck?

src/mesh/NodeDB.cpp Outdated Show resolved Hide resolved
.trunk/trunk.yaml Outdated Show resolved Hide resolved
src/mesh/NodeDB.cpp Outdated Show resolved Hide resolved
@HarukiToreda
Copy link
Contributor

Tested on Heltec V3, wireless Paper, Wireless Tracker. Although the screen on the Tracker is the same size as the V3 i can view less messages per line, this is not an issue since we have the ability to scroll with the arrow keys. However the User button does not work anymore, no matter how many times it is pressed it remains on the same Message screen on all 3 devices. Arrow keys on the CardKB are the only way to go to the next screens.
20240505_192657
20240505_192722
20240505_192735

@meshtastic-bot
Copy link

This pull request has been mentioned on Meshtastic. There might be relevant details there:

https://meshtastic.discourse.group/t/t-deck-2-3-6-firmware-how-to-change-channel/12378/8

@ndnestor
Copy link
Author

ndnestor commented May 9, 2024

Very nice.

On which devices/screens has this been tested next to the T-Deck?

@GUVWAF
I've only tested it on the T-Deck so far since that's all I own.

@todd-herbert
Copy link
Contributor

todd-herbert commented May 9, 2024

Very nice.
On which devices/screens has this been tested next to the T-Deck?

@GUVWAF I've only tested it on the T-Deck so far since that's all I own.

What I might actually suggest is: instead of modifying the Screen class directly, write a new class which derives from Screen, then override what you need. Then, where Screen is instantiated, use a macro to instead use your derived class.

That way, you can focus on implementing the new features for just T-Deck right now, and gradually expand to more devices later on.

@ndnestor
Copy link
Author

Very nice.
On which devices/screens has this been tested next to the T-Deck?

@GUVWAF I've only tested it on the T-Deck so far since that's all I own.

What I might actually suggest is: instead of modifying the Screen class directly, write a new class which derives from Screen, then override what you need. Then, where Screen is instantiated, use a macro to instead use your derived class.

That way, you can focus on implementing the new features for just T-Deck right now, and gradually expand to more devices later on.

The issue I see with making a T-Deck-specific device is that expanding would mean making classes for other models which seems impractical in the long run as new devices come out. For maintainability, one class that can generically deal with everything would be nice.

@todd-herbert
Copy link
Contributor

Very nice.
On which devices/screens has this been tested next to the T-Deck?

@GUVWAF I've only tested it on the T-Deck so far since that's all I own.

What I might actually suggest is: instead of modifying the Screen class directly, write a new class which derives from Screen, then override what you need. Then, where Screen is instantiated, use a macro to instead use your derived class.
That way, you can focus on implementing the new features for just T-Deck right now, and gradually expand to more devices later on.

The issue I see with making a T-Deck-specific device is that expanding would mean making classes for other models which seems impractical in the long run as new devices come out. For maintainability, one class that can generically deal with everything would be nice.

I'm still suggesting using one class for all devices, but use a macro to select the new class. If a variant is built with the macro USE_PERSISTENT_MSG (or whatever suits), it will build with the new class, otherwise it will build with the existing Screen class.

Once it's confirmed that a new device works correctly with the new class, that generic macro can be added to its specific platformio.ini file

Have a look at how the Screen constructor does it. I think something like this might be useful

@ndnestor
Copy link
Author

Very nice.
On which devices/screens has this been tested next to the T-Deck?

@GUVWAF I've only tested it on the T-Deck so far since that's all I own.

What I might actually suggest is: instead of modifying the Screen class directly, write a new class which derives from Screen, then override what you need. Then, where Screen is instantiated, use a macro to instead use your derived class.
That way, you can focus on implementing the new features for just T-Deck right now, and gradually expand to more devices later on.

The issue I see with making a T-Deck-specific device is that expanding would mean making classes for other models which seems impractical in the long run as new devices come out. For maintainability, one class that can generically deal with everything would be nice.

I'm still suggesting using one class for all devices, but use a macro to select the new class. If a variant is built with the macro USE_PERSISTENT_MSG (or whatever suits), it will build with the new class, otherwise it will build with the existing Screen class.

Once it's confirmed that a new device works correctly with the new class, that generic macro can be added to its specific platformio.ini file

Have a look at how the Screen constructor does it. I think something like this might be useful

Oh I see. That makes sense to me. Will do!

@@ -87,6 +87,7 @@
#define HW_VENDOR meshtastic_HardwareModel_TLORA_V2_1_1P8
#elif defined(T_DECK)
#define HW_VENDOR meshtastic_HardwareModel_T_DECK
#define USE_PERSISTENT_MSG
#elif defined(T_WATCH_S3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This define is best moved to the variant file if you really want it only for the t-deck.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caveman99 Would you prefer to try implement it everywhere? I'll withdraw my objections if you do think it'd be better.

Copy link
Contributor

@HarukiToreda HarukiToreda May 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second this, the Tdeck UI will be out someday, but all other devices will be left behind, this is the best option we've gotten so far. Maybe have a new module and the ability to give the choice of turning this on or not. I've been using this on my V3 and so far no issues other than the ability to press the program button. No issues with long messages either cause I can scroll up and down to see them, something I couldn't do before. please don't block this for just the Tdeck

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea initially was to maybe get it released for T-Deck first, and add other devices once they're tested. There's definitely a lot of edge case stuff that would need to be handled, and some devices probably shouldn't get this feature at all imo, but if the devs want to go ahead and try, I'll support the effort.

@caveman99
Copy link
Member

just a general question, since you seem to go to great lengths to implement this. I am not convinced the heavy filesystem operation this relys on will work on the embedded platforms when the number of files grows. You do know realize we have a new UI for the T-Deck ind evelopment that will mimick a phone app more than the current Oled UI? This seems like double effort.

@ndnestor
Copy link
Author

just a general question, since you seem to go to great lengths to implement this. I am not convinced the heavy filesystem operation this relys on will work on the embedded platforms when the number of files grows. You do know realize we have a new UI for the T-Deck ind evelopment that will mimick a phone app more than the current Oled UI? This seems like double effort.

@caveman99 Yes, I've heard about that. When I first asked about this feature on the Discord server, I was told that it would, optimistically, take a year. Based on the progress done so far, I would guess that it's going to take decently longer than that. Given the utility of this feature, I figured it is worth it to have this not-so-fancy version for now and get the fancy version later.

@bobricius
Copy link

What about reserve some array in RAM for cca 10 messages ?

@tropho23
Copy link
Contributor

tropho23 commented May 14, 2024 via email

@todd-herbert
Copy link
Contributor

todd-herbert commented May 14, 2024

Even a rolling window of 10 messages for nrf52 would be great, for now until an external storage solution is developed. Tony Good

This would be a whole lot more straight-forward! (Assuming there's RAM to spare?)

@tropho23
Copy link
Contributor

tropho23 commented May 14, 2024 via email

@ndnestor
Copy link
Author

We've solved the throttling problem by implementing a database file for each "category" instead of individual message files in a file structure. This allows us to save messages in batches which store in a buffer that gets saved every 30 seconds to a single file, limiting our number of writes. We also now execute saving to disk asynchronously which makes the whole process safe from packet-loss.

@ndnestor ndnestor marked this pull request as ready for review June 1, 2024 00:48
@HarukiToreda
Copy link
Contributor

I tried building this to test but it's not building. Missing "message.pb.h"

In file included from src/DisplayFormatters.h:2,
                 from src/DisplayFormatters.cpp:1:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
In file included from src/GPSStatus.h:2,
                 from src/gps/GPS.h:5,
                 from src/ButtonThread.cpp:4:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\DisplayFormatters.cpp.o] Error 1
*** [.pio\build\heltec-v3\src\ButtonThread.cpp.o] Error 1
In file included from src/modules/Telemetry/Sensor/TelemetrySensor.h:7,
                 from src/modules/Telemetry/Sensor/INA219Sensor.h:6,
                 from src/power.h:41,
                 from src/power.cpp:13:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\Power.cpp.o] Error 1
In file included from src/mesh/Default.h:2,
                 from src/PowerFSM.cpp:11:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\PowerFSM.cpp.o] Error 1
In file included from src/RedirectablePrint.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\RedirectablePrint.cpp.o] Error 1
In file included from src/airtime.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\airtime.cpp.o] Error 1
In file included from src/buzz/buzz.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\buzz\buzz.cpp.o] Error 1
In file included from src/SerialConsole.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\SerialConsole.cpp.o] Error 1

@ndnestor
Copy link
Author

ndnestor commented Jun 6, 2024

I tried building this to test but it's not building. Missing "message.pb.h"

In file included from src/DisplayFormatters.h:2,
                 from src/DisplayFormatters.cpp:1:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
In file included from src/GPSStatus.h:2,
                 from src/gps/GPS.h:5,
                 from src/ButtonThread.cpp:4:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\DisplayFormatters.cpp.o] Error 1
*** [.pio\build\heltec-v3\src\ButtonThread.cpp.o] Error 1
In file included from src/modules/Telemetry/Sensor/TelemetrySensor.h:7,
                 from src/modules/Telemetry/Sensor/INA219Sensor.h:6,
                 from src/power.h:41,
                 from src/power.cpp:13:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\Power.cpp.o] Error 1
In file included from src/mesh/Default.h:2,
                 from src/PowerFSM.cpp:11:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\PowerFSM.cpp.o] Error 1
In file included from src/RedirectablePrint.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\RedirectablePrint.cpp.o] Error 1
In file included from src/airtime.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\airtime.cpp.o] Error 1
In file included from src/buzz/buzz.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\buzz\buzz.cpp.o] Error 1
In file included from src/SerialConsole.cpp:2:
src/mesh/NodeDB.h:13:10: fatal error: mesh/generated/meshtastic/message.pb.h: No such file or directory
 #include "mesh/generated/meshtastic/message.pb.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
*** [.pio\build\heltec-v3\src\SerialConsole.cpp.o] Error 1

@HarukiToreda I was told to remove that file from the PR since it can be generated using nanopb. You can manually add message.pb.c from here and message.pb.h from here by copying and pasting the file contents.

@ndnestor
Copy link
Author

Can somebody review this please?

@HarukiToreda
Copy link
Contributor

Can somebody review this please?

even if I manually add the message files missing, this still won't build, we shouldn't have to manually add these files.

Compiling .pio\build\heltec-v3\src\input\kbMatrixBase.cpp.o
src/graphics/Screen.cpp:960:13: error: redefinition of 'void graphics::drawBattery(OLEDDisplay*, int16_t, int16_t, uint8_t*, const meshtastic::PowerStatus*)'
 static void drawBattery(OLEDDisplay *display, int16_t x, int16_t y, uint8_t *imgBuffer, const PowerStatus *powerStatus)
             ^~~~~~~~~~~
src/graphics/Screen.cpp:430:13: note: 'void graphics::drawBattery(OLEDDisplay*, int16_t, int16_t, uint8_t*, const meshtastic::PowerStatus*)' previously defined here
 static void drawBattery(OLEDDisplay *display, int16_t x, int16_t y, uint8_t *imgBuffer, const PowerStatus *powerStatus)
             ^~~~~~~~~~~
src/graphics/Screen.cpp:1420:6: error: redefinition of 'bool graphics::deltaToTimestamp(uint32_t, uint8_t*, uint8_t*, int32_t*)'
 bool deltaToTimestamp(uint32_t secondsAgo, uint8_t *hours, uint8_t *minutes, int32_t *daysAgo)
      ^~~~~~~~~~~~~~~~
src/graphics/Screen.cpp:890:6: note: 'bool graphics::deltaToTimestamp(uint32_t, uint8_t*, uint8_t*, int32_t*)' previously defined here
 bool deltaToTimestamp(uint32_t secondsAgo, uint8_t *hours, uint8_t *minutes, int32_t *daysAgo)
      ^~~~~~~~~~~~~~~~
src/graphics/Screen.cpp:960:13: warning: 'void graphics::drawBattery(OLEDDisplay*, int16_t, int16_t, uint8_t*, const meshtastic::PowerStatus*)' defined but not used [-Wunused-function]
 static void drawBattery(OLEDDisplay *display, int16_t x, int16_t y, uint8_t *imgBuffer, const PowerStatus *powerStatus)
             ^~~~~~~~~~~
In file included from src/graphics/Screen.cpp:44:
src/graphics/images.h:197:22: warning: 'poo' defined but not used [-Wunused-variable]
 static unsigned char poo[] PROGMEM = {
                      ^~~
src/graphics/images.h:186:22: warning: 'heart' defined but not used [-Wunused-variable]
 static unsigned char heart[] PROGMEM = {
                      ^~~~~
src/graphics/images.h:175:22: warning: 'devil' defined but not used [-Wunused-variable]
 static unsigned char devil[] PROGMEM = {
                      ^~~~~
src/graphics/images.h:165:22: warning: 'fog' defined but not used [-Wunused-variable]
 static unsigned char fog[] PROGMEM = {
                      ^~~
src/graphics/images.h:154:22: warning: 'cloud' defined but not used [-Wunused-variable]
 static unsigned char cloud[] PROGMEM = {
                      ^~~~~
src/graphics/images.h:143:22: warning: 'rain' defined but not used [-Wunused-variable]
 static unsigned char rain[] PROGMEM = {
                      ^~~~
src/graphics/images.h:132:22: warning: 'sun' defined but not used [-Wunused-variable]
 static unsigned char sun[] PROGMEM = {
                      ^~~
src/graphics/images.h:115:22: warning: 'deadmau5' defined but not used [-Wunused-variable]
 static unsigned char deadmau5[] PROGMEM = {
                      ^~~~~~~~
src/graphics/images.h:104:22: warning: 'cowboy' defined but not used [-Wunused-variable]
 static unsigned char cowboy[] PROGMEM = {
                      ^~~~~~
src/graphics/images.h:93:22: warning: 'wave_icon' defined but not used [-Wunused-variable]
 static unsigned char wave_icon[] PROGMEM = {
                      ^~~~~~~~~
src/graphics/images.h:82:22: warning: 'haha' defined but not used [-Wunused-variable]
 static unsigned char haha[] PROGMEM = {
                      ^~~~
src/graphics/images.h:71:22: warning: 'bang' defined but not used [-Wunused-variable]
 static unsigned char bang[] PROGMEM = {
                      ^~~~
src/graphics/images.h:61:22: warning: 'question' defined but not used [-Wunused-variable]
 static unsigned char question[] PROGMEM = {
                      ^~~~~~~~
src/graphics/images.h:51:22: warning: 'thumbdown' defined but not used [-Wunused-variable]
 static unsigned char thumbdown[] PROGMEM = {
                      ^~~~~~~~~
src/graphics/images.h:43:22: warning: 'thumbup' defined but not used [-Wunused-variable]
 static unsigned char thumbup[] PROGMEM = {
                      ^~~~~~~
*** [.pio\build\heltec-v3\src\graphics\Screen.cpp.o] Error 1

@ndnestor
Copy link
Author

Can somebody review this please?

even if I manually add the message files missing, this still won't build, we shouldn't have to manually add these files.

I would told to not include them here: #3784 (comment)

Those are strange errors. I have never messed with any of those functions. I also do not get them on my end, even if I build for a Heltec v3. I'll look into it

src/mesh/NodeDB.cpp Outdated Show resolved Hide resolved
@robertfisk
Copy link
Contributor

This feature is very cool and also beneficial for some meshtastic hardware I'm developing. @ndnestor Would you mind if I fork this and send you PRs over the next couple of weeks to help get it merged?

@ndnestor
Copy link
Author

This feature is very cool and also beneficial for some meshtastic hardware I'm developing. @ndnestor Would you mind if I fork this and send you PRs over the next couple of weeks to help get it merged?

As far as I'm aware, the only thing that needs to be changed to get this merged is that last thing @GUVWAF commented on. I'm about to push that now. But if anything else comes up that needs to be changed, I welcome the help! Thanks!

@thatSFguy
Copy link

This is exactly what I've been wanting for my t-lora 1.6 kiss messenger. Looks like the initiative lost steam, but it would be great to have this feature.
Kiss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.