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

event LEDs mode, pragma once, PID knob_led interaction, preheatdone sound, small fixes and cleanup #1209

Conversation

traffic-light
Copy link
Contributor

@traffic-light traffic-light commented Oct 26, 2020

Whats inside this PR

  • Added the printer event led mode for neopixels and encoder wheel.
  • The knob color will get the correct color when wakeup lcd is called
  • Knob led will be red when PID tuning is started (not yet implemented) --> Make the led not go off when screen is idle
  • Knob led will be green when PID tuning is finished (not yet implemented) --> Some work to do on this one
  • Renamed language_du to language_nl. This is the correct landcode
  • using pragma_once in all the header files.
  • Make a beep when preheating is finished (only locally)
  • Some code cleaning (Spacing etc.)
  • Added curly braches on one line if-statements at some places where I was working on

Please use the version in the link below. The current version was not intended to push and is not tested and buggy.
https://github.com/traffic-light/BIGTREETECH-TouchScreenFirmware_peter/tree/2551f6447b753540c3e9ca6f9a7df0c3770da1a6

printer event LEDs mode workaround

During printing:

  • Gradually change from blue to violet as the heated bed gets to target temp
  • Gradually change from violet to red as the hotend gets to temperature
  • Change to white to illuminate work surface
  • Change to green once print has finished
  • Turn off after the print has finished and the user exited the printing menu

This will also happen to the RGB led of the encoder wheel (when available).
The only difference is that the knob won't turn into white but will be the color that's configured in the feature menu.

For developers

  • Added a first version of a styleguide
  • Added the notification in the pullrequest template to inform users to read this styleguide

Linked issues that will be implemented / resolved

Why is this a draft?

I'm still working on it. It will cost some time. If you have any ideas or advise, please leave a comment

The way languages are set is changed. Made a new branch for it
* added sequential mode to languages
* Added seguential mode to config
* Added encoder led wakeup in wakeup LCD

Bugs to check
* feature settings doens't keep it's settings
Still the odd behavior that the settings from the ini file gets corrupted when rebootig
Need a better name for it instead of 'setSequentialModeColor'
Maybe something like: sequentialModeWorker
@digant73
Copy link
Contributor

Try to set PARA_SIZE to 768 in flashStore.h and see if you solve the problem

@traffic-light
Copy link
Contributor Author

traffic-light commented Oct 26, 2020

@digant73 Thank you. Didn't think about that option. Was checking my own code thinking I messed up somewhere.

I'll try this when I'm working on it.

Edit: Did try it out (had to wait before I could go to bed)
It's still there...

@traffic-light traffic-light changed the title sequential mode whileprint sequential mode whileprint (help needed with feature issues. See 'why is this a draft') Oct 27, 2020
@Mactastic1-5
Copy link

Mactastic1-5 commented Oct 27, 2020

I'm glad you're working towards getting #define PRINTER_EVENT_LEDS implemented. It should work as indicated by Marlin in Configuration.h (below). While performing a PID tuning, I think the LEDs should change to different colors like a rainbow, then Green. Considering there is an OFF button in the LED Color section, maybe there should be an AUTO button as well, which would default back to Printer Event LEDs if OFF or one of the other colors are selected.

/**

  • Printer Event LEDs
  • During printing, the LEDs will reflect the printer status:
    • Gradually change from blue to violet as the heated bed gets to target temp
    • Gradually change from violet to red as the hotend gets to temperature
    • Change to white to illuminate work surface
    • Change to green once print has finished
    • Turn off after the print has finished and the user has pushed a button
      */

@traffic-light
Copy link
Contributor Author

traffic-light commented Oct 27, 2020

Printer Event LEDs
During printing, the LEDs will reflect the printer status:

Gradually change from blue to violet as the heated bed gets to target temp

Gradually change from violet to red as the hotend gets to temperature

Change to white to illuminate work surface

Change to green once print has finished

Turn off after the print has finished and the user has pushed a button

Yes. This is implemented.

Maybe is should change the name. I thought they called it sequential mode...

@Mactastic1-5
Copy link

Printer Event LEDs
During printing, the LEDs will reflect the printer status:

Gradually change from blue to violet as the heated bed gets to target temp

Gradually change from violet to red as the hotend gets to temperature

Change to white to illuminate work surface

Change to green once print has finished

Turn off after the print has finished and the user has pushed a button

Yes. This is implemented.

Maybe is should change the name. I thought they called it sequential mode...

Which TFT did you use to test?

@bigtreetech bigtreetech marked this pull request as ready for review October 28, 2020 05:59
@traffic-light
Copy link
Contributor Author

@stellarspace not yet, but I'm using a tft35 e3

@bigtreetech this fr is not ready yet (and not completely tested and it's having a bug (as mentioned in the text))
Please keep it as a draft 😉. Once it's finneshed and I didn't find any problems I'll change it from draft to ready for merge

@traffic-light traffic-light marked this pull request as draft October 28, 2020 07:13
@ETE-Design
Copy link

ETE-Design commented Oct 29, 2020

So the only thing you need to implement is the color when PID Tuning, and to find the error? Would like to try it out, could you pls. merge newest master?

@mak0t0san
Copy link
Contributor

I would like to advocate for the use of curly braces in 'if' statements, even when it's only one line. There has already been a bug due to missing curly braces, and I can see this happening again.
I would say the only exception would be if the statement is on the same line as the if statement, like this:
if (x == y) doThis();

Otherwise, it should have curly braces:

if (x == y)
{
  if (x != z)
  {
    doThis();
  }
}

Is there a coding guideline somewhere for this repo?

@Mactastic1-5
Copy link

Mactastic1-5 commented Oct 30, 2020

So teh only thing you need to implement is the color when PID Tuning, and to find the error? Would like to try it out, could you pls. merge newest master?

LEDs don’t even show when idle or printing. You can only change the color of the LED to RGBW. This would change that. You can use GitHub Desktop to Merge master with p1209, then resolve the conflicts in Visual Studio Code and build the TFT firmware of your choosing.

I would like to advocate for the use of curly braces in 'if' statements, even when it's only one line. There has already been a bug due to missing curly braces, and I can see this happening again.
I would say the only exception would be if the statement is on the same line as the if statement, like this:
if (x == y) doThis();

Otherwise, it should have curly braces:

if (x == y)
{
  if (x != z)
  {
    doThis();
  }
}

Is there a coding guideline somewhere for this repo?

Coding guideline? No, this repository is poorly maintained in my opinion and there aren’t that many developers.

@traffic-light
Copy link
Contributor Author

traffic-light commented Oct 30, 2020

I would like to advocate for the use of curly braces in 'if' statements, even when it's only one line. There has already been a bug due to missing curly braces, and I can see this happening again.
I would say the only exception would be if the statement is on the same line as the if statement, like this:
if (x == y) doThis();

Otherwise, it should have curly braces:

if (x == y)
{
  if (x != z)
  {
    doThis();
  }
}

Is there a coding guideline somewhere for this repo?

I just do it like everyone else does. And by the way. There is nothing wrong with no curly brackets. When you add an extra line you should add them...

I also changed some other parts to this. By the way, it's pretty common, even within projects from big companies, to don't use them when there is just one row.

@traffic-light
Copy link
Contributor Author

So the only thing you need to implement is the color when PID Tuning, and to find the error? Would like to try it out, could you pls. merge newest master?

I'll merge it when I have time..
When the error is gone i've got to test if it really works. It should, but you never know

@mak0t0san
Copy link
Contributor

I would like to advocate for the use of curly braces in 'if' statements, even when it's only one line. There has already been a bug due to missing curly braces, and I can see this happening again.
I would say the only exception would be if the statement is on the same line as the if statement, like this:
if (x == y) doThis();
Otherwise, it should have curly braces:

if (x == y)
{
  if (x != z)
  {
    doThis();
  }
}

Is there a coding guideline somewhere for this repo?

I just do it like everyone else does. And by the way. There is nothing wrong with no curly brackets. When you add an extra line you should add them...

I also changed some other parts to this. By the way, it's pretty common, even within projects from big companies, to don't use them when there is just one row.

Fair enough, there are coding styles that encourage not to use them when not needed. I've been following the Microsoft guidelines so I may be a bit biased. Apple's infamous "goto fail" SSL security flaw was a result of not having curly braces so personally I prefer having them.

In the grand scheme of things, I'd like to see consistency and would love to see a coding style guideline adopted (Microsoft, Google, LLVM, etc).

@traffic-light
Copy link
Contributor Author

traffic-light commented Oct 30, 2020 via email

@Mactastic1-5
Copy link

I agree. The nice thing about visual studio is that it can already do a lot of the styling work for you. Only problem is that there will be a lot of changes in the repo... I agree it's better to use curly braces. It also possbile to go from none curly to curly to the files i'm changing/ cleaning instead of removing them.. Met vriendelijke groet; Peter van Weeterloo

________________________________ Van: Makoto Schoppert [email protected] Verzonden: vrijdag 30 oktober 2020 16:57 Aan: bigtreetech/BIGTREETECH-TouchScreenFirmware [email protected] CC: Peter van Weeterloo [email protected]; Author [email protected] Onderwerp: Re: [bigtreetech/BIGTREETECH-TouchScreenFirmware] sequential mode whileprint (help needed with feature issues. See 'why is this a draft') (#1209) I would like to advocate for the use of curly braces in 'if' statements, even when it's only one line. There has already been a bug due to missing curly braces, and I can see this happening again. I would say the only exception would be if the statement is on the same line as the if statement, like this: if (x == y) doThis(); Otherwise, it should have curly braces: if (x == y) { if (x != z) { doThis(); } } Is there a coding guideline somewhere for this repo? I just do it like everyone else does. And by the way. There is nothing wrong with no curly brackets. When you add an extra line you should add them... I also changed some other parts to this. By the way, it's pretty common, even within projects from big companies, to don't use them when there is just one row. Fair enough, there are coding styles that encourage not to use them when not needed. I've been following the Microsoft guidelineshttps://github.com/microsoft/WinObjC/wiki/Coding-Standards#223----always-use-braces-around-bodies so I may be a bit biased. Apple's infamous "goto fail" SSL security flaw was a result of not having curly braces so personally I prefer having them. In the grand scheme of things, I'd like to see consistency and would love to see a coding style guideline adopted (Microsoft, Google, LLVM, etc). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#1209 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AI2VWALIJ7HQUZXXEZ2E243SNLO7LANCNFSM4S7Z354Q.

I fixed the conflicts, but the LEDs aren't working, the Fan does go from F0 to CtL upon reboot and the Feature menu is non-functional. Not sure if I fixed the conflicts correctly. I'm new to GitHub Desktop working in tandem with Visual Studio Code.

@traffic-light
Copy link
Contributor Author

@guruathwal do you have any idea of the problem mentioned in the main post? I can't find a solution for it :(

@guruathwal
Copy link
Contributor

@traffic-light I will look into it. I need some time.

@traffic-light
Copy link
Contributor Author

traffic-light commented Nov 3, 2020

@traffic-light I will look into it. I need some time.

Thank you!

Take your time. No hurries!

(Still need to do some fine-tuning in the neopixel code I added. But first I want that the error is gone....)

@ETE-Design
Copy link

@bigtreetech Could you mabye look at this, and make the last changes to make it work and merge it?

@digant73
Copy link
Contributor

digant73 commented Aug 1, 2021

@traffic-light it seems the PR is abandoned. Do you think it can be resumed by someone else and merged in BTT main branch? If so, what's remaining to be fixed in the LED handling?

@oldman4U
Copy link
Contributor

oldman4U commented Aug 4, 2021

Hi digant73.

I believe it would make sense to start with a new PR. Beep once preheating has finished is done, so the main new stuff would be the LED color and pragma_once. This PR has been made before BTT added support for "external" Neopixel, not sure how this affects the functionality. I love, that the external led's turn off once the screen dims, something you probably want to avoid during preheating.

oldman

@digant73
Copy link
Contributor

digant73 commented Aug 4, 2021

yes I know. from this old PR only EVENT LED and the use of #pragma can be resumed. I will start with the EVENT LED first. We will see how to handle the external Neopixel controlled by the LCD's knob LED

@oldman4U
Copy link
Contributor

oldman4U commented Aug 4, 2021

👍🏻

Let me know in case I can help with testing.

@traffic-light
Copy link
Contributor Author

traffic-light commented Aug 4, 2021 via email

@oldman4U
Copy link
Contributor

oldman4U commented Aug 4, 2021

Peter is back!!!

🤩

@MarkusThur
Copy link
Contributor

So if I get it right, this PR to be replaced by a new "updated" one? Would be happy to help in it. @oldman4U @traffic-light @digant73

@oldman4U
Copy link
Contributor

oldman4U commented Aug 5, 2021

Seems so. Yes!😁

@traffic-light
Copy link
Contributor Author

traffic-light commented Aug 5, 2021 via email

@digant73
Copy link
Contributor

digant73 commented Aug 5, 2021

@traffic-light if it's ok for you (just to integrate the first version) I can port your code in the current BTT main branch. In the meanwhile users will start to provide their feedbacks. Obviously all credits to ypur work.
I should release another PR in the next days/week, so I could also integrate the event led feature (more or less as it is now).
Please let me know

@traffic-light
Copy link
Contributor Author

traffic-light commented Aug 18, 2021 via email

@oldman4U
Copy link
Contributor

traffic-light. Whats about your exam? Success!!??
;-)

@CBDesignS
Copy link

Hope you managed to get your exams passed. Still running your code from way back and Its still going good. I moved the tft35 control over to full control by Octoprint and never looked back. the machine is semi retired as I got a 2nd hand Ender 5 and upgraded it to the max with a new larger frame and build area powered with an mks sgen-l v1 also controlled by Octoprint as neopixel support was worse than btt.. Good luck boss

@traffic-light
Copy link
Contributor Author

traffic-light commented Sep 27, 2021 via email

@ETE-Design
Copy link

@traffic-light Any progress?

@ETE-Design
Copy link

@digant73 Maby you could make the Led Knob Color Change On Progress instead?

@Mactastic1-5
Copy link

I think @digant73 will get to it eventually. He's made a lot of changes that are amazing.

@discip
Copy link
Contributor

discip commented Dec 11, 2021

Good evening everyone,
@MacarioPatrick

I think @digant73 will get to it eventually. He's made a lot of changes that are amazing.

I don't know, if you and @digant73 are buddies, 🤷‍♂️ but unless you are not, than I recommend to not speak up about his schedules. 😊
There is no doubt about his capabilities!, but as you might imagine, he has priorities outside of github.

We all should be grateful to this kind of people, who are enabling us, to have all the nice features, UIs ...

@ETE-Design

@traffic-light Any progress?

As @traffic-light stated:

I need to restart my work due to much changes. It takes some time to get into the firmware again -_-'

He obviously is planning on continuing his work, but this takes time!
So we have to be a bit more patient about this getting done, while not forgetting, that this is totally up to them, if it is getting going at all.

I think I am able to share your excitement about this getting done, since I a have been there myself, in the not to distant past. 😔

Hopefully no one is offended, since that was not my intent!

kind regards

@traffic-light
Copy link
Contributor Author

traffic-light commented Dec 12, 2021

I'm so sorry that it took soo much time to react to this..
Currently I'm almost finished with school. So that will give me time. (just 2 thing remaining :-) )

At the moment it's also going better with me as person, for now.

I don't know when I have time enough to finish this project/ restart.
It also depends if I will move to another house soon.

If someone want to take this project and finish it, feel free

@traffic-light
Copy link
Contributor Author

traffic-light commented Dec 12, 2021

@discip

Thanks for your comment ;-) It's better too see this in my mailbox instead of people crying for digant. Thats why i did a comment right now

@Mactastic1-5
Copy link

@traffic-light These features have been implemented, and you can cancel the merge request. Thanks for your help!

@traffic-light
Copy link
Contributor Author

I'm having some time due to my almost finisdhed school (no exams. Just 1 excercise) (yes i should be ashamed of myself. Still not done.) and having covid again :-P

I installed the newest version of the firmware (my previous one was more then 1 year old)
I've got to say: Lot's of good changes! Good job everyone! I REALLY like the keypad :-)

@oldman4U
Copy link
Contributor

oldman4U commented Mar 3, 2022

Hi Peter. 2x COV. HOW!!??

@traffic-light
Copy link
Contributor Author

traffic-light commented Mar 3, 2022

Hi oldman. Omicron is different. But it feels like the common cold for me.
(the previous one was in 12-2020

@oldman4U
Copy link
Contributor

oldman4U commented Mar 3, 2022

It is great that you are back!

@traffic-light
Copy link
Contributor Author

traffic-light commented Mar 3, 2022

Thanks. First i've got to find out WTH is wrong with my hemera :-P
This is just awfull:
P1020736
P1020750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment