firmware icon indicating copy to clipboard operation
firmware copied to clipboard

Add persistent message storage and message log frame

Open ndnestor opened this issue 1 year ago • 24 comments

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

ndnestor avatar May 04 '24 22:05 ndnestor

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 04 '24 22:05 CLAassistant

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.

CLAassistant avatar May 04 '24 22:05 CLAassistant

Very nice.

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

GUVWAF avatar May 05 '24 06:05 GUVWAF

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

HarukiToreda avatar May 05 '24 23:05 HarukiToreda

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

meshtastic-bot avatar May 07 '24 04:05 meshtastic-bot

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.

ndnestor avatar May 09 '24 00:05 ndnestor

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.

todd-herbert avatar May 09 '24 04:05 todd-herbert

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.

ndnestor avatar May 10 '24 00:05 ndnestor

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

todd-herbert avatar May 10 '24 01:05 todd-herbert

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!

ndnestor avatar May 10 '24 01:05 ndnestor

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 avatar May 11 '24 22:05 caveman99

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.

ndnestor avatar May 11 '24 23:05 ndnestor

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

bobricius avatar May 14 '24 10:05 bobricius

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

Tony Good


From: Peter Misenko @.> Sent: Tuesday, May 14, 2024 06:20 To: meshtastic/firmware @.> Cc: tropho23 @.>; Mention @.> Subject: Re: [meshtastic/firmware] Add persistent message storage and message log frame (PR #3784)

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

— Reply to this email directly, view it on GitHubhttps://github.com/meshtastic/firmware/pull/3784#issuecomment-2109827095, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AQ7GUPQQ6Q3RDRBFYNBDIP3ZCHQPHAVCNFSM6AAAAABHHFLYOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBZHAZDOMBZGU. You are receiving this because you were mentioned.Message ID: @.***>

tropho23 avatar May 14 '24 13:05 tropho23

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

todd-herbert avatar May 14 '24 14:05 todd-herbert

Of course more than 10 would better 🙂

My ideal is the last 30 messages if available memory allows.

Tony Good


From: todd-herbert @.> Sent: Tuesday, May 14, 2024 10:03 To: meshtastic/firmware @.> Cc: tropho23 @.>; Mention @.> Subject: Re: [meshtastic/firmware] Add persistent message storage and message log frame (PR #3784)

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!

— Reply to this email directly, view it on GitHubhttps://github.com/meshtastic/firmware/pull/3784#issuecomment-2110329198, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AQ7GUPRVZX5T46LK6GXRKHTZCIKRTAVCNFSM6AAAAABHHFLYOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQGMZDSMJZHA. You are receiving this because you were mentioned.Message ID: @.***>

tropho23 avatar May 14 '24 14:05 tropho23

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 avatar May 31 '24 04:05 ndnestor

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 avatar Jun 05 '24 07:06 HarukiToreda

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 avatar Jun 06 '24 02:06 ndnestor

Can somebody review this please?

ndnestor avatar Jun 13 '24 00:06 ndnestor

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

HarukiToreda avatar Jun 17 '24 22:06 HarukiToreda

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: https://github.com/meshtastic/firmware/pull/3784#discussion_r1597515177

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

ndnestor avatar Jun 21 '24 00:06 ndnestor

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?

robertfisk avatar Aug 18 '24 02:08 robertfisk

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!

ndnestor avatar Aug 18 '24 23:08 ndnestor