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

Fixed status leds for meshing aand nozzle cleaning #404

Closed
wants to merge 8 commits into from

Conversation

smwoodward
Copy link
Contributor

I noticed that the status LEDs wasn't working for bed meshing and z-tilt/qgl.

@Surion79
Copy link
Collaborator

Thank you, please use for each functional change an own PR. This has two mixed changes (LED setting and Z movement)

@smwoodward
Copy link
Contributor Author

I don't understand what you mean. Are you saying I should do a PR for only the meshing fix, a PR for the qgl fix, and a separate PR for z-tilt?

@smwoodward
Copy link
Contributor Author

Oh wait, I see what you're saying. I must have used the wrong branch when I submitted this.

@smwoodward
Copy link
Contributor Author

Thank you, please use for each functional change an own PR. This has two mixed changes (LED setting and Z movement)

Ok, got that fixed.

@Surion79 Surion79 changed the title Fixed status leds for meshing and z-tilt/qgl Fixed status leds for meshing aand nozzle cleaning Dec 17, 2023
@Surion79
Copy link
Collaborator

now i made a mess by accident. Can you please restore the nozzle_cleaning.cfg without the empty line? the original one

@smwoodward
Copy link
Contributor Author

nozzle_cleaning.cfg

See if that fixed it.

@Surion79
Copy link
Collaborator

Thanks! now i got it as i wanted it :D

@Surion79
Copy link
Collaborator

Surion79 commented Dec 17, 2023

i need to check the timings because of the strobe flashing we had a couple weeks ago. But looks good for me

@Surion79 Surion79 self-requested a review December 17, 2023 21:42
@Surion79 Surion79 self-assigned this Dec 17, 2023
@smwoodward
Copy link
Contributor Author

I would think this would essentially work as all of the other status functions.

@Surion79
Copy link
Collaborator

If you look at the files you added it directly above of _CG28 which includes already status leds right at the start. which means it would flash. Could you please move your code below _CG28? Probably needs some changes in _CG28 as well, but need to check that (_CG28 has an end status led set which would flash when your code addition would be live)

@smwoodward
Copy link
Contributor Author

If you look at the files you added it directly above of _CG28 which includes already status leds right at the start. which means it would flash. Could you please move your code below _CG28? Probably needs some changes in _CG28 as well, but need to check that (_CG28 has an end status led set which would flash when your code addition would be live)

I see. If you need to move them around, please do. I put setting the status after the initial set commands like the other helpers. I didn't think about if there was any type of loop on the homing rename and such.

@Frix-x
Copy link
Owner

Frix-x commented Dec 18, 2023

Hello, not sure to understand this PR since everything is already set correctly in the main calling macros and the files you modified are conditionnaly included.

Do you have a specific need or something for this? I'm not against doing this change but then we will need to clean the other parts of the code where the STATUS_LED commands are not needed anymore. For example here:

{% if status_leds_enabled %}
STATUS_LEDS COLOR="LEVELING"
{% endif %}

This block is the one that manage the leds for QGL and Z_TILT at the same time. There is other examples like this one.

@smwoodward
Copy link
Contributor Author

Hello, not sure to understand this PR since everything is already set correctly in the main calling macros and the files you modified are conditionnaly included.

Do you have a specific need or something for this? I'm not against doing this change but then we will need to clean the other parts of the code where the STATUS_LED commands are not needed anymore. For example here:

{% if status_leds_enabled %}
STATUS_LEDS COLOR="LEVELING"
{% endif %}

This block is the one that manage the leds for QGL and Z_TILT at the same time. There is other examples like this one.

The status animation for me isn't working for QGL/Ztilt and meshing. I'll double check and look at things, but all other status' work.

@Frix-x
Copy link
Owner

Frix-x commented Dec 18, 2023

Hello, not sure to understand this PR since everything is already set correctly in the main calling macros and the files you modified are conditionnaly included.
Do you have a specific need or something for this? I'm not against doing this change but then we will need to clean the other parts of the code where the STATUS_LED commands are not needed anymore. For example here:

{% if status_leds_enabled %}
STATUS_LEDS COLOR="LEVELING"
{% endif %}

This block is the one that manage the leds for QGL and Z_TILT at the same time. There is other examples like this one.

The status animation for me isn't working for QGL/Ztilt and meshing. I'll double check and look at things, but all other status' work.

Oh, it may be because you call directly the QGL function and not the generic Klippain tilting process. But yes in this case it would be better to move the functions as you've made it indeed...

@Frix-x
Copy link
Owner

Frix-x commented Dec 18, 2023

Ok I've just checked and these files are ok to be modified. So we can add your changes. But can you update the calling macros to remove the original call that are not needed anymore?
Also, in this case, the START_PRINT modules will also need to be updated to remove these LED settings since they will be handled in the sub macros directly.

@smwoodward
Copy link
Contributor Author

Hello, not sure to understand this PR since everything is already set correctly in the main calling macros and the files you modified are conditionnaly included.

Do you have a specific need or something for this? I'm not against doing this change but then we will need to clean the other parts of the code where the STATUS_LED commands are not needed anymore. For example here:

{% if status_leds_enabled %}
STATUS_LEDS COLOR="LEVELING"
{% endif %}

This block is the one that manage the leds for QGL and Z_TILT at the same time. There is other examples like this one.

Hello, not sure to understand this PR since everything is already set correctly in the main calling macros and the files you modified are conditionnaly included.
Do you have a specific need or something for this? I'm not against doing this change but then we will need to clean the other parts of the code where the STATUS_LED commands are not needed anymore. For example here:

{% if status_leds_enabled %}
STATUS_LEDS COLOR="LEVELING"
{% endif %}

This block is the one that manage the leds for QGL and Z_TILT at the same time. There is other examples like this one.

The status animation for me isn't working for QGL/Ztilt and meshing. I'll double check and look at things, but all other status' work.

Oh, it may be because you call directly the QGL function and not the generic Klippain tilting process. But yes in this case it would be better to move the functions as you've made it indeed...

That may be what I'm experiencing also. Now I am using klipper-led-effect with the barf examples that is mentioned here #164. I am going to attempt to add this in, and I have a separate config that translates all of the klippain status calls to what klipper-led-effect uses to start and stop those animations.

@smwoodward
Copy link
Contributor Author

Ok I've just checked and these files are ok to be modified. So we can add your changes. But can you update the calling macros to remove the original call that are not needed anymore? Also, in this case, the START_PRINT modules will also need to be updated to remove these LED settings since they will be handled in the sub macros directly.

The only thing that is in the Print_start is a BUSY status and a PRINTING status call.

Something that I did just notice, all of the other sub macros do usually call a status_led of READY, but I did not add that to these. Would you want that called at the end of the sub macro? Arguably If a print is started, it should call the BUSY status and not ready, but I would assume that if it's left as READY that when the next sub macro calls the next call you wouldn't see the READY status.

@EricZimmerman
Copy link
Collaborator

it would be nice to have all the sub calls get their own status colors/phase indicators

@smwoodward
Copy link
Contributor Author

it would be nice to have all the sub calls get their own status colors/phase indicators

For the most part, they already do.

@Surion79
Copy link
Collaborator

it is important to understand, that this PR is re-introducing strobe flashing on LED status calls as it is integrated in this PR here, @Frix-x .
With these changes the leds are turned to the color and will instantaneously reflash to a different color since _CG28 will always call a Status LED every time, either when it runs G28 or even when not.
It would need to do some code movement and probably a change in _CG28 so the flashing is not reintroduced

@Frix-x
Copy link
Owner

Frix-x commented Dec 20, 2023

I'll close this one in favor of #412 that is a big rework of the LED system and that contain the same feature. So this will get merged soon :)

@Frix-x Frix-x closed this Dec 20, 2023
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.

4 participants