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

Decent implementation of Klipper_led_effects and rainbow barf lets #412

Merged
merged 84 commits into from
Dec 21, 2023

Conversation

claudioguareschi
Copy link
Contributor

I took the liberty to modify your excellent klipain to add support for led_effects and rainbow barf.
Led_effects can be set separately for caseligts, plain sb status leds/rainbow barf in any combination.
Respecting yout phylosophy configuration is done in printer.cfg
Few files were created in config/hardware/lights to support rainbow barf and led effects.
Logic in mcros/hardware_functions/status_leds.cfg was adapted to support led effects, attempting to respect the existing code as much as possible.
Leveling and meshing functions were modified to support led status changes when run from mainsail/fluidd

I played with the effects a little bit, tested using plain sb 3 led setup (I have a barf coming soon). The effects I choose respecting the existing default colors as much as possible are ok for now but need to be refined to make the final setup look really good. Creating the right default effects is a work in progress.

Hope this is useful. Let me know what you think

Claudio

tehniemer and others added 8 commits December 13, 2023 10:34
Co-authored-by: Jan-Gerrit Drexhage <102791900+Surion79@users.noreply.github.com>
Co-authored-by: Félix Boisselier <felix@fboisselier.fr>
Co-authored-by: Jan-Gerrit Drexhage <102791900+Surion79@users.noreply.github.com>
Co-authored-by: Félix Boisselier <felix@fboisselier.fr>
Co-authored-by: Félix Boisselier <felix@fboisselier.fr>
@Surion79
Copy link
Collaborator

that looks pretty nice on first flythrough :)

@Benoitone
Copy link
Collaborator

I'm very interested to add this future too for the next ERCF 🤩

_SET_LEDS_BY_NAME LEDS="nozzle" COLOR={status_color[color].nozzle} TRANSMIT=1
{% elif printer["gcode_macro _USER_VARIABLES"].status_leds_logo_led_name == "status_leds_effects"%}
{% set print_line = "Effect changed - logo: %s, nozzle: %s\n" % ("sb_logo_" + status_color[color].logo, "sb_nozzle_" + status_color[color].nozzle) %}
{action_respond_info(print_line)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this needed? looks like some sort of info printing of the print?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(i mean line 188)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I wouldn't have it on the finished implementation. It is there more for debug purposes and to make sure effects are triggered when they are supposed to. For example QGL effect wasn't working for me and printing out this info allowed me to find a reset to the ready state in conditional homing (_CG28 macro) outside its if condition that was messing things up. It was making the regular status led flicker and totally messing up the effects. This implementation is not completed yet. I just submitted to see if you guys were interested in me continuing pursuing this avenue, as I saw other people already attempting to integrate led effects into klippain.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I'm all in for this PR 🚀
Thanks a lot for your work!

Copy link
Owner

@Frix-x Frix-x left a comment

Choose a reason for hiding this comment

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

Ok thank you very much for this big PR. I'm really happy about it and it's already pretty good.
I've made some comments to make it a little bit better on the user side.

Also some more comments here:

  1. Please rename all "rb" references in the filenames, etc... to something more easy to understand like "rainbowbarf" or at least "rbarf"
  2. You've made some change about the placement of the STATUS_LED call in the submacros. This is pretty good and a work that has already started with Fixed status leds for meshing aand nozzle cleaning #404 . But I'll close Fixed status leds for meshing aand nozzle cleaning #404 in favor of your PR that is more complete. In order to clean up a little bit and prepare for a future merge, can you remove the comments you've left at the previous places?

user_templates/printer.cfg Outdated Show resolved Hide resolved
macros/miscs/startup.cfg Outdated Show resolved Hide resolved
_SET_LEDS_BY_NAME LEDS="nozzle" COLOR={status_color[color].nozzle} TRANSMIT=1
{% elif printer["gcode_macro _USER_VARIABLES"].status_leds_logo_led_name == "status_leds_effects"%}
{% set print_line = "Effect changed - logo: %s, nozzle: %s\n" % ("sb_logo_" + status_color[color].logo, "sb_nozzle_" + status_color[color].nozzle) %}
{action_respond_info(print_line)}
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I'm all in for this PR 🚀
Thanks a lot for your work!

@Surion79
Copy link
Collaborator

i really like the quality of the PR. It is indeed decent and thought through.

@claudioguareschi
Copy link
Contributor Author

Be glad to make all the proposed fixes. Easily done.
I would love for somebody with a better aesthetic sense than I have to help out with some thought out effects that make the printer look good without making it look like a flying saucer!
A couple of questions:
Should we consider additional printer states for heater bed soaking and/or ercf functions?

@Surion79
Copy link
Collaborator

A couple of questions: Should we consider additional printer states for heater bed soaking and/or ercf functions?

Regarding ERCF/HH, Benetoine is currently integrating an updated version. So I think for now, we should go forward with this
Regarding additional printer states: It is important to understand, when which lights are triggered. If someone is not careful, the status might result in strobe flashes (because there is nothing done between two led status calls, thats why _CG28 got changed recently)

@Frix-x
Copy link
Owner

Frix-x commented Dec 20, 2023

Should we consider additional printer states for heater bed soaking and/or ercf functions?

I think this is not needed for now. We can keep it like this since all the previous state are correctly managed.

On the ERCF topic, there is a big ERCF PR (#371) ongoing to get compatibility with the ERCFv2 and the latest HH software. So I think the ERCF states could be managed directly in this PR when your PR is merged. Feel free to participate to it if you want :)

@Frix-x Frix-x added the enhancement New feature or request label Dec 20, 2023
@Surion79
Copy link
Collaborator

@Frix-x the base must be changed to develop in this PR. since a lot of effort is done already, i cannot grasp possible issues on rebase to develop. currently it is pointing to main

@Benoitone
Copy link
Collaborator

Should we consider additional printer states for heater bed soaking and/or ercf functions?

I think this is not needed for now. We can keep it like this since all the previous state are correctly managed.

On the ERCF topic, there is a big ERCF PR (#371) ongoing to get compatibility with the ERCFv2 and the latest HH software. So I think the ERCF states could be managed directly in this PR when your PR is merged. Feel free to participate to it if you want :)

For the MMU/ERCF klipper_led seems to be natively supported by Happy_Hare... I haven't been able to test it yet... I am in the process of assembling my ERCFv2...
But this PR interests me a lot for the management of the other LEDs and to see how it behaves.
I would be in favor of pushing it on a new branch of klippain before its merge when @Frix-x wishes. So that I can see what it could give in parallel with the management of Happy_Hare.

@Benoitone
Copy link
Collaborator

As an off topic, I still have a problem with LED flashing at the time of the QGL in _MODULE_TILTING I think there is something to check on this side...

@Surion79
Copy link
Collaborator

Surion79 commented Dec 20, 2023

Should we consider additional printer states for heater bed soaking and/or ercf functions?

I think this is not needed for now. We can keep it like this since all the previous state are correctly managed.
On the ERCF topic, there is a big ERCF PR (#371) ongoing to get compatibility with the ERCFv2 and the latest HH software. So I think the ERCF states could be managed directly in this PR when your PR is merged. Feel free to participate to it if you want :)

For the MMU/ERCF klipper_led seems to be natively supported by Happy_Hare... I haven't been able to test it yet... I am in the process of assembling my ERCFv2... But this PR interests me a lot for the management of the other LEDs and to see how it behaves. I would be in favor of pushing it on a new branch of klippain before its merge when @Frix-x wishes. So that I can see what it could give in parallel with the management of Happy_Hare.

i suggest to complete the big HH-PR first before sidetracking it for another feature, IMHO. Ex manager had a saying: better metal on the road than gold in the storage (meaning software)

@Surion79
Copy link
Collaborator

As an off topic, I still have a problem with LED flashing at the time of the QGL in _MODULE_TILTING I think there is something to check on this side...

at some point, it might need to be required to use params to differ between mainsail call and call within a process (like start_print) to correctly set the leds in manual use and automated/integrated use. I had the thought already couple time when looking at the strobe flashing issue.

@Frix-x Frix-x changed the base branch from main to develop December 21, 2023 10:00
@Frix-x Frix-x added next version This thing is already done or is planned in the next version in preparation tracking This issue is tracked and work will be done labels Dec 21, 2023
@Frix-x
Copy link
Owner

Frix-x commented Dec 21, 2023

Ok so I indeed changed the target to develop and will do a final review soon but it look like ok for me on a first sight now :)

@Frix-x
Copy link
Owner

Frix-x commented Dec 21, 2023

Mhmm I think the automated target change directly on Github was not a good idea after this number of commit and I should have done it manually... I'll fix it.

@Frix-x Frix-x changed the base branch from develop to leds-rework December 21, 2023 10:07
@Frix-x Frix-x self-assigned this Dec 21, 2023
@Frix-x Frix-x merged commit dfc0fc1 into Frix-x:leds-rework Dec 21, 2023
Frix-x added a commit that referenced this pull request Dec 21, 2023
)


Co-authored-by: Félix Boisselier <felix@fboisselier.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next version This thing is already done or is planned in the next version in preparation tracking This issue is tracked and work will be done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants