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

Resolved issue https://github.com/letscontrolit/ESPEasy/issues/1844; #183

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

Conversation

JoostDkr
Copy link
Contributor

@JoostDkr JoostDkr commented Apr 2, 2022

Also a checkbox is added to the device-webform for switching on/off sending debug/info-log to EASpeasy-log

Update _P208_Nokia_LCD_5110.ino
Resolved issue that could cause esp-easy-weblog stops working.
code optimizations based on reviewers comments.
//}
void myLog(const String & message) {
String log;
if (sentlog){

Choose a reason for hiding this comment

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

  • The order of these 2 lines could be swapped
  • You could also check in the if wether the log-level is set (when compiling against a current ESPEasy mega):
    if (sentlog && logLevelActiveFor(LOG_LEVEL_INFO)) {

that would save a few cpu cycles and a bit of memory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can not use the current ESPmega (ESPEasy-mega-20220328) because of build errors......
I'm using ESPEasy_ESP32_mega-20211224 without build errors....

_P208_Nokia_LCD_5110.ino Outdated Show resolved Hide resolved
_P208_Nokia_LCD_5110.ino Outdated Show resolved Hide resolved
_P208_Nokia_LCD_5110.ino Show resolved Hide resolved
_P208_Nokia_LCD_5110.ino Outdated Show resolved Hide resolved
log.reserve(message.length() + 6);
log += F("P208: ");
log += message;
addLog(LOG_LEVEL_INFO, log);

Choose a reason for hiding this comment

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

when compiling on current mega branch, please use addLogMove macro when appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can not use the current ESPmega (ESPEasy-mega-20220328) because of build errors......
I'm using ESPEasy_ESP32_mega-20211224 without build errors....

Copy link
Member

Choose a reason for hiding this comment

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

What build errors do you have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unnamed

Copy link

@tonhuisman tonhuisman Apr 3, 2022

Choose a reason for hiding this comment

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

I've tried to replay the scenario you have most likely taken:

  • Downloaded the ESPEasy-20220328 sources zip-file from the Releases page
  • Unzipped that file using the 7-zip tool (I always avoid using the Windows explorer .zip extracter, as that thing has been dangerously broken since the first ever release around Windows XP!)
  • Got the _P208_Nokia_LCD_5110.ino from this PR
  • Opened the unzipped folder using VSCode with PlatformIO installed
  • Added Adafruit PCD8544 Nokia 5110 LCD library to lib_deps in both platformio_esp32_envs.ini and platformio_esp82xx_base.ini (don't forget to add a comma after the last entry already there)
  • Added #define USES_P208 to the #ifdef PLUGIN_BUILD_NORMAL section of define_plugin_sets.h to include the plugin (a bit hacky, I know 👼)
  • Clicked Build for the normal_ESP8266_4M1M PIO configuration: successful on first execution
  • Clicked Build for the normal_ESP32_4M316k PIO configuration: successful after a few retries (PIO is known to sometimes have issues when switching configuration like this)

Not sure what the difference is with what you did, but it should be mostly the same, possibly you unpacked the sources-zip using the default Windows tool?

@JoostDkr
Copy link
Contributor Author

JoostDkr commented Apr 3, 2022 via email

@@ -2,6 +2,7 @@
//################################## Plugin 208: NOKIA 5110 lcd #########################################
//#######################################################################################################
#include "_Plugin_Helper.h"
#include<string>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this include is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right. This is not needed

@tonhuisman
Copy link

@JoostDkr I have reserved Plugin ID [P141] in the ESPEasy mega repository to migrate/rewrite this plugin. (and assigned the task to me 😃)

It will be a rewrite because we prefer to use the generic ESPEasy AdafruitGFX Helper for all displays that extend the AdafruitGFX library, to have a uniform command structure for all displays, reduce code-size (it is instantiated, not #included), and thus reduce maintenance effort. And any feature implemented in the helper is immediately available for all displays (when applicable, of course).

I will add a new subcommand txl to the AdaGFX Helper to display multiple lines in a single (remote) command, very similar to what you have added to this plugin, but using the regular ESPEasy syntax as described here: letscontrolit/ESPEasy#2724

Effectively that would be something like:
pcd8544,txl,1,"Hello World",4,"Second on line 4",2,"Now on line 2",3,"Finally, line 3"
Giving output like this:

Hello World
Now on line 2
Finally, line 3
Second on line 4

But could be simplified to:
pcd8544,txl,1,"Hello World",,"Now on line 2",,"Then line 3",,"Last on line 4"
(note the double comma's to skip the auto-increment argument)

@JoostDkr
Copy link
Contributor Author

@tonhuisman ,
Great job.
I'm curious about your P141. If I notice it's available I'll use it.
By the way; what is the meaning of the "1" in "pcd8544,txl,1,...."?

@tonhuisman
Copy link

tonhuisman commented Aug 20, 2022

what is the meaning of the "1" in "pcd8544,txl,1,...."?

That's supposed to address line 1, like the current plugin behavior that you extended:
pcd8544,1,hello world;2,on line 2;3,now line 3, AFAICS...

(Or maybe that was already there, didn't investigate that far)

@JoostDkr
Copy link
Contributor Author

Oh. now I undestand your "autoincrement":-). Yes that was a extension made by me.

I had another extension in mind: "Control of more than 1 display". Is that something to build into your P141?
image

@tonhuisman
Copy link

"Control of more than 1 display". Is that something to build into your P141?

That's my current default for plugin development 😎, biggest challenge, at least on ESP8266, is having enough free GPIO pins and memory to hold all plugin instances 😉

@tonhuisman
Copy link

I've created a PR: letscontrolit/ESPEasy#4211
@JoostDkr Please test, you can download binaries from the connected Actions build, and report your results in the PR 👍

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.

3 participants