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

Character Font, Scrolling Text, API, Alternative Clock with Second Pixel #69

Merged
merged 14 commits into from
Jan 15, 2024

Conversation

kohlsalem
Copy link
Contributor

Hi,

i implemented the followig features:

  • converted the font from last CT Make demo scetch into this implementation
  • added Screen.scrollText("Hello World!"); function that shows text
  • added an alternative clock implementation using the font
  • added a (static) blackout for the clock at night
  • added a moving pixel to have the clock less static

to my assesment this changes should not change/break anything existing...

Best
Michael

{

if ( (timeinfo.tm_hour*60+timeinfo.tm_min) < 6*60+30 ||
(timeinfo.tm_hour*60+timeinfo.tm_min) > 22*60+30 ) //only between 6:30 and 22:30

Choose a reason for hiding this comment

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

May I recommend these hours be definable in the constants file? They could then fall back to your defaults if not defined.

I'm also looking into this as part of #67 and personally would like to see this as a global setting instead of a plugin setting so perhaps this whole conditional could be removed and we can implement it globally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the rewiew, no doubts, constants would be better then what i did here, however:

  • Actually i would as well like to see this as a global feature, not something plugin specific. I would happily add it globaly, if @ph1p agrees, but i think this would be a different PR
  • And i believe it should not be constant but needs to be configurable, so once the PR with the wifi manager is mereged, i could add an additional user parameter there.

So, for now it is just a local quick fix in that plugin to keep my significant other happy. :-)

Best
Michael

@jekkos
Copy link
Contributor

jekkos commented Dec 2, 2023

Thanks! Very nice. This was a feature I was planning to add myself. Also the REST endpoint to send messages can now be used by home assistant, so the panel can display notifications.

@kohlsalem
Copy link
Contributor Author

I added propper message handling and printing of graphs,. Obviously i am a little bit "under knowledged", i was very surprised to find my commits automatically added in this PR, i planned to have it in a seperate one.

However, @ph1p, has this monster now have a chance to get mergend?

@ph1p
Copy link
Owner

ph1p commented Dec 3, 2023

Hi @kohlsalem :) First of all, thank you so much for this PR! And it definitely gets a chance to be merged! I think display text is one of the biggest missing features. I also flashed it on my ESP32 and it works fine, except for long texts. My ESP hangs when sending texts that are too long.

Great work!

@kohlsalem
Copy link
Contributor Author

Hmm, which version? in the vesion before my very last commit i found the same problem- aparrently it is not good to spend to much time in a http handler. However, in the most recent commit i fixed that....

Where is your upper limit? If it still crashes that would mean i can not blockout the loop for a longer time. Solvable but... ugly.

@kohlsalem
Copy link
Contributor Author

kohlsalem commented Dec 3, 2023

thank you so much for this PR!

"Danichfüa!" Thanks for that project. Nicely crafted! It was looking so obviously ahead the the hacked scetch from C'T, that i decided to throw out the 8266 and soildered a esp32...

Just to see that a font is missing :-D i added the scrolling already to the 8266 code, so that part was c&p. Converting the font was a but more tricky and ended up in a monster-excel-sheet.

I added some fixes and a chapter to the readme. from my POV it should me somewhat ok...

@kohlsalem kohlsalem changed the title Character Font, Scrolling Text, Alternative Clock with Second Pixel Character Font, Scrolling Text, API, Alternative Clock with Second Pixel Dec 4, 2023
@kohlsalem
Copy link
Contributor Author

Closes #9

Copy link
Owner

@ph1p ph1p left a comment

Choose a reason for hiding this comment

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

Thanks again! Great work! :) I think it would be nice to add a loop flag and a delay parameter to control the speed of movement and display the message infinitely. Is that possible?
Could it be that the repeat flag is not working properly? I pass an int and it only repeats once.
And could you please format your code?

README.md Outdated Show resolved Hide resolved
include/messages.h Outdated Show resolved Hide resolved
src/messages.cpp Outdated Show resolved Hide resolved
src/messages.cpp Outdated Show resolved Hide resolved
@kohlsalem
Copy link
Contributor Author

Thanks for proposals - yes, i have obviously overseen dev clutter.

@ph1p, delay is clear, that was already on my todo list. And as you said, should'nt be a big deal.

What would the loop flag be for? If you do not pass any repeat, the message shows only once.

I did think about some infinite looping (only show messages) undtil you press the button. Was that your idea?

@kohlsalem
Copy link
Contributor Author

kohlsalem commented Dec 5, 2023

  • Added Delay as parameter
  • Added Scaling for the graph as patameter (miny & maxy)
  • changed rendering of the graph from pixel to line
  • Ammended readme
  • and infinite message repeat

@kohlsalem
Copy link
Contributor Author

and some bug fixes and minor improvements

@kohlsalem kohlsalem requested a review from ph1p December 6, 2023 07:41
@kohlsalem
Copy link
Contributor Author

@ph1p Hi Phil,
normally i should have no big pressure with the merge, but since i messed with the propper use of branches, i brought myself a bit into a situation i cant resolveby myself. trying to get the recently meged PRs i messed up beyond my abilities. Is there a way to merge this PR as it is, or shall i better startover with a new fork and try to recreate the PR?

@MauiKano
Copy link

I did add the changes from kohlsalem to my fork under https://github.com/MauiKano/ikea-led-obegraensad
Works well for me an I do like the messages a lot. I used it for displaying a boot message in the main.cpp and started writing my own plugin FiveLetterWords. This randomly shows a word out of a large list of words from https://github.com/charlesreid1/five-letter-words . More could be done with that plugin. Oh yes, I modified the simple clock plugin and added a second indicator. I pixel that wanders around the edge of the frame which happens to have 60 pixels in total :-)

@ph1p
Copy link
Owner

ph1p commented Dec 17, 2023

Hi! Could you resolve the conflicts? I'll merge this afterwards. Thank you :)

@MauiKano
Copy link

you catch me on the wrong foot. which conflict ?

@kohlsalem
Copy link
Contributor Author

Hmm. It is emparrassing, but cant resolve the conflicts (unless someone is willing to help me with the version control mess i probably created).

@MauiKano
Copy link

can you please be more specific ? I was able to pull your code into my fork and all compiles and runs well.

@kohlsalem
Copy link
Contributor Author

lease be more specific ? I was able to pull your code into my fork and all compiles and runs well.

Well, despite my decades in software development if am maximum stupid in work with git. since i could not understand how to use it directly in vscode propperly, use it via the githup desktop. I tried quire some htings, but did not suceed to resolve the conflicts, Not logically (the seem minimal) but technically in github.
If you could hint me via teams (michael@kohlsalem.com) would be awsome, as i said, i am a more than a little list in the branches...

@ph1p
Copy link
Owner

ph1p commented Dec 19, 2023

I'll fix the commit history and resolve the conflicts :)

@kohlsalem
Copy link
Contributor Author

I'll fix the commit history and resolve the conflicts :)

you are my hero. I promise to do proper branches for my next feature and hope i can do better with this...

@ph1p
Copy link
Owner

ph1p commented Dec 20, 2023

Ah I see. So the repeat is after 1 minute. This is a little bit confusion. It would be great to set this with delay and add a duration flag to determine how long it should take to see the text. What do you think?

Just checkout your repository:

git clone git@github.com:kohlsalem/ikea-led-obegraensad.git
# or 
git clone https://github.com/kohlsalem/ikea-led-obegraensad.git

@kohlsalem
Copy link
Contributor Author

Hmm. I get your point. But Hmm.

The usecase i had in mind ist, that i have a default view. Clock, weather, game of live, whatever. And then some Infomessage shows.

Features i imagine:

  • Show indicator, that messages are about to be shown (Blinking pixels in the 4 corners.
  • Allow to confirm messages with the button. IF the button is pressed while showing, the message will be stopped

I like the idea to show the messages at the full minute, becaus i jus know when it will be shown again. If i set the duration it will become really random.

I could imagine a "urgent" flag, which shows the Message continuously until button pressed?

@kohlsalem
Copy link
Contributor Author

@ph1p so, any thoughts on my proposal? Actually, since the PC is anyway alreasy to big for my preference, i would be somehow happy if you could merege it before i add new features...@

@c64emulator
Copy link

Der Messagetext sollte zur Sicherheit auf gültige Zeichen überprüft werden, bevor er ausgegeben wird: momentan bootet der ESP neu, wenn Umlaute (ä,ö,ü) im Messagetext enthalten sind.
Anregung: zumindest die deutschen Sonderzeichen/Umlaute würde ich noch in die Tabelle in 'signs.cpp' einarbeiten.

@kohlsalem
Copy link
Contributor Author

Autsch. Das tut echt weh. Super peinlich. Nicht validierter User-Input als Lookup in einem Array. Immerhin bin ich überrascht, das der ESP mit einem Speicherschutz daher kommt. ich hätte mit Random Pixel für die Umlaute gerechnet. Wird gefixt!

@c64emulator
Copy link

c64emulator commented Jan 11, 2024

Ich denke, es ist sinnvoll, aus diesem PR mehrere zu machen. Das Plugin mit dem Zeichensatz sollte abgetrennt werden. Die Nutzung des Zeichensatzes könnte lizenzrechtlich problematisch sein.
Als Plan-B könnte auch ein anderer Zeichensatz verwendet werden (z.B. https://github.com/micropython/micropython/blob/master/extmod/font_petme128_8x8.h)

@c64emulator
Copy link

Noch eine Anregung für den Message-Teil: es sind ja noch ein paar Pins am ESP frei, ein Piezo-Buzzer könnte angeschlossen werden. Dann könnte bei einer Message auch ein Beep (oder sogar eine Melodie?) ausgegeben werden.

@kohlsalem
Copy link
Contributor Author

Ich denke, es ist sinnvoll, aus diesem PR mehrere zu machen. Das Plugin mit dem Zeichensatz sollte abgetrennt werden.
Ja, ich meine auch der PR sollte (in einner Bugfreien Version) erstmal reinmigriert werden. Wird sonst gefühlt einfach zu groß.

Zeichensätze wechselbar wäre z.B. definitiv ein separates Thema.

@c64emulator
Copy link

Noch eine Anmerkung zum Messageteil: Du nutzt ja momentan den 6x7-Font. Du rechnest damit auch in 'Screen.scrollText' (hardcodiert...). Zwischen den einzelnen Zeichen sollte aber mindestens ein Spalte Abstand sein, damit sie besser lesbar sind, wenn sie über das Panel scrollen.

@kohlsalem
Copy link
Contributor Author

Hast Du das mit der Spalte mal ausprobiert? ich hab und hatte es für sch*** ähh, schlecht befunden...

@kohlsalem
Copy link
Contributor Author

Ansonsten, für beliebige fonts /umschaltbare fonts müsste man das ganze irgendwie anders aufziehen. Da müssten die Maße dann aussen um das Array nochmal festgeschrieben werden. bzw, Es bräuchte ein Array von Fonts mit Metadaten(Größe, Namen, ...) den Fontdetails, ...

Fixed out of bounds crash on umlauts, introduced german umlauts, introduced handling of different fonts.
@kohlsalem
Copy link
Contributor Author

  • Crashes fixed
  • Umlauts introduced
  • Font handling introduced
  • Better readable numbers in text
  • Correct spacing handling

and still trusting that it is worth to be merged ;-)

last minute feature of memory improvements had a bug causing lowercase umlauts not to be shown
@kohlsalem
Copy link
Contributor Author

so, and as a (hopefully last) feature to this: While Messges are pending for display, an indicator is flashin in upper left corner

@ph1p
Copy link
Owner

ph1p commented Jan 15, 2024

Thank you! :) I'll take a look at it

@ph1p ph1p merged commit 69d4af2 into ph1p:main Jan 15, 2024
@kohlsalem
Copy link
Contributor Author

Merci!

@thunder-62
Copy link

aehm... apologies for the spoiler... Hopefully, I made made a mistake and someone can help me to get it corrected...
When I send "http://192.168.20.59/message?text=ABC" in a GET request, my Display shows "789" seems like the index to an ASCII Table is somehow incorrect...

@kohlsalem
Copy link
Contributor Author

Hmm, is it literally translating ABC to 789, or is that an example?
I'm pretty sure it worked for ordinary letters.

  • what did you exactly send and see?
  • With what browser?
  • could you try with wget?

i assume the call comes with bad coding.
could you uncomment line 141 in messages.cpp?
for(int i = 0;i<text.size();i++)Screen.scrollText(std::to_string(text[i]));
That should print the char code which was recieved....

@thunder-62
Copy link

thunder-62 commented Jan 17, 2024

Hi kohlsalem,
it is literally ABC-->789.
first I tried using just Chrome
then I installed the restman chrome Extension and sent it with a pure GET request. same result.

As soon as I am back, I will uncomment the line and tell you about the result... looking at the code: it writes the chars to serial right?

Cheers,
Uwe

@kohlsalem
Copy link
Contributor Author

Hi Uwe,

I'll check again today, anyway strange.

I cant see, what "Last Second" Change should have caused this, since this seems to be a obious Bug :-/

Best Michael

@kohlsalem
Copy link
Contributor Author

Ok. I found the issue, without testing it. It was a last minute change - or the missing ability to use Copy&Past Properly. By making changes on the fonts for digits and creating a new "font" with bold digits, i apparently pasted them at the origin. And tested only with digits sinceafter - which works fine.

In file src/signs.cpp please remove line 86-95.

THe index numbers in the comments at the end should continuosly count up.

@thunder-62
Copy link

yessss... removing the lines was the fix! Thank you!

@kohlsalem
Copy link
Contributor Author

@ph1p The duplication did not happen while i copied, apparently it is a glitch during your reformatting.

I do not understand, why you did reformat it? Yes, the source code is smaller, but after compilation... byte is byte....

The big benifit of thebin format (and the reason why i used it) you can see and modify characters directly in the code. That is lost on hex....

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.

7 participants