-
-
Notifications
You must be signed in to change notification settings - Fork 434
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
Do not crash for dynamically created macros with no config. #1123
Conversation
Hi @viesturz, thank you for opening this Pull Request. Code-wise this looks fine to me, and I see no problem with it, but I am failing to see how this would ever happen... I mean, how can you create a macro without it being on the config file? |
Using a Klipper extension.
In my case - this:
viesturz/klipper@3e9aa4d
It's a bit unusual and there is probably a better way long term, but for
now it makes things work.
…On Thu, Jul 20, 2023, 23:50 Pedro Lamas ***@***.***> wrote:
Hi @viesturz <https://github.com/viesturz>, thank you for opening this
Pull Request.
Code-wise this looks fine to me, and I see no problem with it, but I am
failing to see how this would ever happen... I mean, how can you create a
macro without it being on the config file?
—
Reply to this email directly, view it on GitHub
<#1123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMQT4ZQUM27LCIRWLBRGILXRGR2VANCNFSM6AAAAAA2SAADKQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This is not mainland Klipper, so that explains the behavior. Personally, I'm always skeptical of adding ad-hoc support for unknown extensions. At first glance, I don't see the changes on this particular PR becoming an issue, but these kind of solutions can become a problem when we do code changes/refactoring as we do not test with non-mainland Klipper code and so this could easily get lost... |
Fair enough.
An alternative that would work for my case would be to extend the toll
change command code to query all gcode commands instead of just macros.
That is a significantly bigger change however.
Do you think that would be acceptable, or still too custom?
…On Fri, Jul 21, 2023, 10:14 Pedro Lamas ***@***.***> wrote:
Using a Klipper extension. In my case - this: ***@***.***
<viesturz/klipper@3e9aa4d>
This is not mainland Klipper, so that explains the behavior. Personally,
I'm always skeptical of adding ad-hoc support for unknown extensions.
At first glance, I don't see the changes on this particular PR becoming an
issue, but these kind of solutions can become a problem when we do code
changes/refactoring as we do not test with non-mainland Klipper code and so
this could easily get lost...
—
Reply to this email directly, view it on GitHub
<#1123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMQT444F4P6ECPQRDPL25TXRI26PANCNFSM6AAAAAA2SAADKQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Some more thoughts on the options::
- short term: Implement a dynamic gcode macro registration in my klipper
branch. That seems to be the smallest change to make this work properly.
- when/if my branch is merged into klipper, fluidd could be updated to
support alternative way of tool discovery via the [toolchnager] object. The
gcode macro approach seems more compatible with other systems. And is
potentially a long way off.
Anyhow this PR seems like a dead end, closing.
…On Fri, Jul 21, 2023 at 12:43 PM Viesturs Zarins ***@***.***> wrote:
Fair enough.
An alternative that would work for my case would be to extend the toll
change command code to query all gcode commands instead of just macros.
That is a significantly bigger change however.
Do you think that would be acceptable, or still too custom?
On Fri, Jul 21, 2023, 10:14 Pedro Lamas ***@***.***> wrote:
> Using a Klipper extension. In my case - this: ***@***.***
> <viesturz/klipper@3e9aa4d>
>
> This is not mainland Klipper, so that explains the behavior. Personally,
> I'm always skeptical of adding ad-hoc support for unknown extensions.
>
> At first glance, I don't see the changes on this particular PR becoming
> an issue, but these kind of solutions can become a problem when we do code
> changes/refactoring as we do not test with non-mainland Klipper code and so
> this could easily get lost...
>
> —
> Reply to this email directly, view it on GitHub
> <#1123 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAMQT444F4P6ECPQRDPL25TXRI26PANCNFSM6AAAAAA2SAADKQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
I'm experimenting with auto-generated T0,T1... macros.
And fluidd just dies if macros do not have a printer config section.
This is a basic fix to avoid that.