Orbs screen saver - Matrix rain style #239
No reviewers
Labels
No labels
bug
conflicts
documentation
DO NOT MERGE
duplicate
enhancement
good first issue
hardware
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
greg/info-orbs!239
Loading…
Reference in a new issue
No description provided.
Delete branch "Orbs-ScreenSaver"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A small widget to provide a 'Matrix' style rain effect..
I think I've included everything..
Enjoy.
I've corrected the small error - lower case "w" :-(
Thank you for your contribution!
I'll to review the code soon, but here are some general thoughts that came to mind:
I think the Widget should be called MatrixWidget or similar, not ScreenSaverWidget, because it's just a normal Widget and not really a screensaver.
Also, if we were to implement a screensaver mechanic, I think it would be best, if the user could configure a Widget of his choice that will be displayed after a timeout.
Please also make sure to use the same naming/formatting schema like the rest of the code base:
widgets/screensaverwidgetorwidgets/matrixwidget)ScreenSaverWidgetorMatrixWidget).clang-formatin the repo (this will be done automatically if you open theinfo-orbs.code-workspacein VSCode)@ -15,6 +15,7 @@#define INCLUDE_PARQET // include ParqetWidget//#define INCLUDE_WEBDATA // include WebDataWidget//#define INCLUDE_MQTT // include MQTTWidget#define INCLUDE_SCREENSAVERIf you rename the Widget, please also rename the config name and add a comment with the actual widget name
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVERI don't think that it's a good idea to use widget specific code in ScreenManager.
The manager classes (ScreenManager, ConfigManager...) should not have knowledge of the widgets.
@ -8,6 +8,7 @@#include <SPI.h>#include <TFT_eSPI.h>#include <TJpg_Decoder.h>#include "ScreenSaverwidget/DigitalRainAnimation.hpp"ScreenManager should not include widget headers
@ -10,3 +11,4 @@#include <ArduinoLog.h>TFT_eSPI tft = TFT_eSPI();DigitalRainAnimation<TFT_eSPI> matrix_effect = DigitalRainAnimation<TFT_eSPI>();Why do you need to define the
matrix_effecthere?main.cpp should only include widget code that's absolutely necessary, i.e.
widgetSet -> add(new XXX)@ -0,0 +1,52 @@#include "ScreenSaverwidget.h"ScreenSaverwidget::ScreenSaverwidget(ScreenManager &manager, ConfigManager &config) : Widget(manager, config){m_enabled = true;Widgets should have an enable option in the WebUI, and they should be disabled by default.
@ -0,0 +12,4 @@m_config.addConfigInt("ScreenSaverWidget", "updateInterval", &m_updateInterval, "Screen Update Interval", true);}void ScreenSaverwidget::addConfigToManager(){What's this function used for?
Did you want to move the
m_config.addConfig*()from the constructor here?@ -0,0 +20,4 @@void addConfigToManager();bool m_smallFont = false;int m_textColor; // = 0x001F;int m_headtextColor; // = 0x07FF;I would set some default values here
@ -0,0 +2,4 @@ScreenSaverwidget::ScreenSaverwidget(ScreenManager &manager, ConfigManager &config) : Widget(manager, config){m_enabled = true;m_config.addConfigBool("ScreenSaverWidget", "SmallFont", &m_smallFont, "Small font (unchecked)", false);Please add a short widget specific prefix to all config keys, e.g. "SmallFont" -> "mtxSmallFont" or similar.
FYI: The key length is limited to 15 characters if I remember correctly.
@ -15,6 +15,7 @@#define INCLUDE_PARQET // include ParqetWidget//#define INCLUDE_WEBDATA // include WebDataWidget//#define INCLUDE_MQTT // include MQTTWidget#define INCLUDE_SCREENSAVERAnd please do not include it in default build (i.e. comment out the line)
@ -0,0 +2,4 @@ScreenSaverwidget::ScreenSaverwidget(ScreenManager &manager, ConfigManager &config) : Widget(manager, config){m_enabled = true;m_config.addConfigBool("ScreenSaverWidget", "SmallFont", &m_smallFont, "Small font (unchecked)", false);And this should probably be renamed to "Big font".
@ -15,6 +15,7 @@#define INCLUDE_PARQET // include ParqetWidget//#define INCLUDE_WEBDATA // include WebDataWidget//#define INCLUDE_MQTT // include MQTTWidget#define INCLUDE_SCREENSAVERHi Christian
Many thanks for all your comments and recommendations.
I'll work on updating my little widget and upload revised code as soon as I can.
I'm not an expert on OO coding so might have a few questions for you, if that's ok?
Regards
Neil
@ -15,6 +15,7 @@#define INCLUDE_PARQET // include ParqetWidget//#define INCLUDE_WEBDATA // include WebDataWidget//#define INCLUDE_MQTT // include MQTTWidget#define INCLUDE_SCREENSAVERHi Neil,
Sure, I'll try my best to answer if I can!
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVERHi Christian
I modified the ScreenManager class because I needed access to functions in the base TFT_eSPI class which are not passed by the ScreenManager class, what is the process here?
Can I propose a change to the ScreenManager class in this PR? to make them available using the standard m_manager object.
May be someone else might need them as some point in the future...
Thanks for the clarification
Neil
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVERI see, you need access to
m_tftto pass it toDigitalRainAnimation.hpp.Well,
.getDisplay()was public before, but we changed it because a widget should not use it directly.The best way would probably to use a wrapper call (i.e. a class that implements the
TFT_eSPIfunctions used byDigitalRainAnimation.hppand forward them toScreenManager.I'll do some test as soon as I can.
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVEROk, thanks
A class within a class and I want to access the internal class...
So, far I have identified I need to access these attributes of TFT_eSPI:
int16_t width();
int16_t height();
void setTextColor(uint16_t c);
void setTextColor(uint16_t c, uint16_t b);
void setTextColor(uint16_t c, uint16_t b, bool bgfill);
uint16_t color565(uint8_t r, uint8_t g, uint8_t b);
void setCursor(int16_t x, int16_t y);
void setTextSize(uint8_t s);
void print(String s);
void print(char c);
And .getDisplay...
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVERI'm happy to modify the "DigitalRainAnimation.hpp" file if needed.
As you can see the initialization takes a pointer to a TFT_eSPI class object (if I understand the code correctly)
// initialization
void DigitalRainAnimation::init(TFT_eSPI gfx,* bool biggerText, bool alphabetOnly)
{
_gfx = gfx;
setBigText(biggerText);
line_len_min = 3;
line_len_max = 10;
isAlphabetOnly = alphabetOnly;
prepareAnim();
}
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVER@flattermann
FYI, I'm managed to get the function working without needing the protected function.. i.e. I'm using ScreenManager without needing to have any Widget specific code in the core function.
I still do need to add functionality to the SM, but that is just to pass some additional TFT_eSPI functions which are not already presented...
I'll update this PR during the day
@ -8,6 +8,7 @@#include <SPI.h>#include <TFT_eSPI.h>#include <TJpg_Decoder.h>#include "ScreenSaverwidget/DigitalRainAnimation.hpp"ok, removed
@ -10,3 +11,4 @@#include <ArduinoLog.h>TFT_eSPI tft = TFT_eSPI();DigitalRainAnimation<TFT_eSPI> matrix_effect = DigitalRainAnimation<TFT_eSPI>();ok, removed
@ -0,0 +1,52 @@#include "ScreenSaverwidget.h"ScreenSaverwidget::ScreenSaverwidget(ScreenManager &manager, ConfigManager &config) : Widget(manager, config){m_enabled = true;ok, changed
@ -0,0 +12,4 @@m_config.addConfigInt("ScreenSaverWidget", "updateInterval", &m_updateInterval, "Screen Update Interval", true);}void ScreenSaverwidget::addConfigToManager(){ok, removed
@ -0,0 +20,4 @@void addConfigToManager();bool m_smallFont = false;int m_textColor; // = 0x001F;int m_headtextColor; // = 0x07FF;Defaults are set in the class, but added them here for safety..
@ -0,0 +2,4 @@ScreenSaverwidget::ScreenSaverwidget(ScreenManager &manager, ConfigManager &config) : Widget(manager, config){m_enabled = true;m_config.addConfigBool("ScreenSaverWidget", "SmallFont", &m_smallFont, "Small font (unchecked)", false);good idea, changed.
@ -15,6 +15,7 @@#define INCLUDE_PARQET // include ParqetWidget//#define INCLUDE_WEBDATA // include WebDataWidget//#define INCLUDE_MQTT // include MQTTWidget#define INCLUDE_SCREENSAVERwidget renamed to MatrixWidget
formatting update to follow project standards
Will be disabled as standard and include better comments.
@ -18,6 +18,13 @@ ScreenManager::ScreenManager(TFT_eSPI &tft) : m_tft(tft) {m_tft.fillScreen(TFT_WHITE);m_tft.setTextDatum(MC_DATUM);reset();#ifdef INCLUDE_SCREENSAVERremoved Widget specific reference from the cord code
I just noticed that it would be good if I add the support for "I18n improvements #241" PR...
Pull request closed