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

add SET_PRINT_STATS_INFO G-Code for pass slicer variables to Klipper #5726

Merged
merged 11 commits into from
Oct 5, 2022

Conversation

meteyou
Copy link
Contributor

@meteyou meteyou commented Aug 23, 2022

This adds a gcode called SET_PRINT_STATS_INFO that can be used insight the slicer to pass variables like the total layer count and current layer information to Klipper.
This would, of course, also be possible with a macro, but I would prefer an integration in Klipper, as this solution is then automatically available for every client.

To use the layer count, you have to include it in the "Custom G-code" in the slicer. Perhaps SuperSlicer integrate this directly.

Example for PrusaSlicer:

# "Start G-code" (above of your start_print gcode)
SET_PRINT_STATS_INFO TOTAL_LAYER=[total_layer_count]

# "After layer change G-code"
SET_PRINT_STATS_INFO CURRENT_LAYER={layer_num + 1}

The TOTAL_LAYER and CURRENT_LAYER will be automatic reset in SDCARD_RESET_FILE.

Thx to @zellneralex for the main idea/implementation!

This adds a gcode called LAYER that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>
@meteyou
Copy link
Contributor Author

meteyou commented Aug 23, 2022

a small addition and good idea from @pedrolamas:

it would be also possible to add this gcode via moonraker preprocessor (like the exclude object gcode).

Copy link
Contributor

@zellneralex zellneralex left a comment

Choose a reason for hiding this comment

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

Looks ok for me

@KevinOConnor
Copy link
Collaborator

Thanks. I don't really have an opinion on this (other than the command should probably be more descriptive). I'd like to see others comment on this. It is a bit unusual for us to create a command just to standardize the command name.

-Kevin

@zellneralex
Copy link
Contributor

zellneralex commented Sep 2, 2022

Hello Kevin,

first sorry for the following wall of text but I feel it is needed to clarify the motivation of that PR.

The reason is not to standardize the command name, the real reason is to standardize the variables that are needed by the UI to show that information.

printer.print_stats.layer_current
printer.print_stats.layer_total

These are needed by the UIs to accurate show the total and current layer the print is at. I know we can argue now if that information is useful or needed but the simple fact is that many users want to have these information shown in the UI and requested it at fluidd and Mainsail.
Both UIs implemented it than in the way that they took the object height and layer height from the meta data to calc the total layer count and the actual Z during print and the layer height to calculate the current layer. That works somehow but has some inaccuracy e.g. when using z-hops and is totally wrong if you use features like "variable layer height".
These inaccuracies has generated questions in the discords and also user wanted to generated bugs against the UIs because of that.

I personally startet than to analyze if klipper itself could generate these information by analyzing the gcode file while loading it at print start. This method has for me the following downsides:

  1. It delays the print start as klipper needs to read the complete gcode file at least once. We talking here about seconds. Also the experience with the exclude object post processor in moonraker shows that that can last long, we taking here about times in the region of 10-20 sec for large files.
  2. klipper will run in the same uncertainties to differentiated a z-hop from a real layer change

We than came to the conclusion the best place to do it is the slicer as it knows when it adds a real layer change and should also know what the current and total layer is. And regardless if the user adds a call of a GCODE (or macro) in the layer change gcode section or by a postprocessor done in moonraker that will give us the most accurate information.

I know that this could be done with a custom macro (In this case 2 as we would need an override of SDCARD_RESET_FILE). But to be honest the UI would need to do a lot of extra stuff to secure against the case that the 2 variables above are missing. Also the experience with fluidd.cfg/mainsail.cfg and timelapse.cfg (all containing necessary setups and macros) has shown that many user struggling with simple tasks like adding them or run in problems as we need to do overrides to existing GCODEs that generates conflicts in there own config that they are not able to solve.

In the end the only reason for the GCODE Layer is to fill the 2 status variable that then can be safely used by the UI, but for the UIs it is important to have these standardize place to read these information.

We at Mainsail where the opinion that LAYER is a good name for it but if you know a better one changing it would be a piece of cake

Best regards Alex

@pedrolamas
Copy link
Contributor

As @zellneralex has very well pointed out, in Fluidd we experienced the same type of questions (and bugs!) regarding Current Layer Number & Total Layers Count.

I like the idea behind this PR: the new LAYER command is a basic status/metadata reporting tool, no more than that!

I'm not entirely sure of the name of the command though... as this is under the virtual_sdcard module, I wonder if SDCARD_LAYER or even SDCARD_REPORT_LAYER would be more appropriate!


Having said that, makes me wonder if a more generic approach should be explored: today we need the Layer Number & Count, tomorrow... who knows?

Why not make this a way for the slicer to pass metadata (key-value pairs) to Klipper that can then be retrieved by the UI?

@zellneralex
Copy link
Contributor

@pedrolamas as much I like the idea of a general key value parser as an engineer as much I fear it from the user to UI perspective .
These general commands work fine internally where you can count on a kind of "contract" that the right key is used. The UI in that case must count on that the user uses in our example layer_current and layer_total and not current_layer and total_layer as keys what make it unpredictable again.

But in general it would be a good idea to parse slicer metadata inside klipper so that the user can use them inside the macros and I have seen PRs about that already. e.g. #4935 and you know how it ended ;-)

@pedrolamas
Copy link
Contributor

But in general it would be a good idea to parse slicer metadata inside klipper so that the user can use them inside the macros and I have seen PRs about that already. e.g. #4935 and you know how it ended ;-)

A bit different as I don't want Klipper to parse anything, but to provide a command that would allow the slicer to directly pass metadata to Klipper... but in the end, I guess it might fall under the same category! 😁

@KevinOConnor
Copy link
Collaborator

Thanks for the explanation. So, to summarize:

  • The Klipper UIs need information about the print that the slicer has.
  • The slicers have the information, but don't have a standard way of exporting it. (It's sometimes exported in adhoc comments; slicer variable naming is not consistent between slicers.)
  • It is technically possible to do this with macos, but it is not really feasible. The initial configuration and ongoing maintenance burden would be too high with macros.

Did I understand the above correctly?

If a new command is added to Klipper, what is expected to generate the command? Are users expected to modify the slicer config to emit them, are slicer developers expected to change the code to emit it, or is a post-processing step expected to alter the g-code to emit it?

-Kevin

@pedrolamas
Copy link
Contributor

pedrolamas commented Sep 2, 2022

If a new command is added to Klipper, what is expected to generate the command? Are users expected to modify the slicer config to emit them, are slicer developers expected to change the code to emit it, or is a post-processing step expected to alter the g-code to emit it?

I believe there are two options there:

  • users can change their Slicer settings to add said command on layer change (Prusa Slicer and derivatives have a section for custom G-code on the Printer Settings, Cura requires a post-processing script that is already included in the default installation)
  • add a pre-processing script to Moonraker (like the existing object cancellation one) that will read/parse the current comments added by the slicers and enrich the G-code on the file with the custom command.

@zellneralex
Copy link
Contributor

@KevinOConnor yes you summarize it correct and @pedrolamas already mentioned the 2 method that normally can be done. In case of SuperSlicer we might get it also included in the the slicer but I do not believe any other slicer will do it.

The user already needs to add stuff to there slicer to be able to use timelapse or to get metadata that moonraker can extract while uploading the gcode files.

@meteyou
Copy link
Contributor Author

meteyou commented Sep 2, 2022

@pedrolamas and @zellneralex have already explained/completed most of this very well.

the name LAYER was only the shortest and simplest name for this specific use case. we could make it more generic if someone can think of other possible values for the future. maybe SDCARD_PRINT_INFO / SDCARD_PRINT_STATS for a generic command/gcode name and then we can add the attributes to it (in this case i would use LAYER_TOTAL and LAYER_CURRENT as attribute names).

@github-actions
Copy link

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
    If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@jschuh
Copy link
Contributor

jschuh commented Sep 25, 2022

In case it helps the discussion along, I've had this layer tracking functionality in my own macros for a while.

I use it to schedule events at layer changes, such as pauses that don't mar exterior perimeters, or future temperature and fan (changes beyond the typical first layer stuff). All of that functionality can of course be done through the macro system, but I agree that the big gap is integrating with the UI. If I'm scheduling these events on a live print I inevitably have to use the console, since the UI's layers are an estimate that's often wrong (as @zellneralex described). However, this is the sort of thing that would integrate very nicely with the UI if it had accurate layer state.

I also have a few thoughts to add on the implementation:

  • I don't know that it makes sense for this to be in the virtual_sdcard module, since it doesn't seem like there's a reason it can't function when not printing from SD. That does mean the command will need an explicit argument for zeroing/resetting the state (although implicitly resetting in SDCARD_RESET_FILE and CANCEL_PRINT seems like a good idea).
  • If we're bikeshedding over names, I'll put in a vote for LAYER_CHANGE or maybe ON_LAYER_CHANGE, since it more accurately captures the intent in my opinion.
  • I'd appreciate support for optionally tracking the printed Z height, for slicers that make this available.

@KevinOConnor
Copy link
Collaborator

Thanks for the feedback. I don't really have an opinion on this. I'm okay with adding the support if it is desired by the UIs.

I'd suggest some changes though:

  1. Don't modify the virtual_sdcard.py module - this support doesn't seem related to it.
  2. Call the command name SET_PRINT_STATS_INFO and place the command (and status information) in the print_stats.py module. For example: SET_PRINT_STATS_INFO CURRENT_LAYER=123
  3. Export it as a separate "info" dictionary in the "printer status" information - for example: printer.print_stats.info.current_layer

@Arksine may have comments as the original author of the print_stats.py module.

Cheers,
-Kevin

Signed-off-by: Stefan Dej <meteyou@gmail.com>
…ENT_LAYER attribute

Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
@meteyou
Copy link
Contributor Author

meteyou commented Sep 27, 2022

Thanks for your feedback!

I have implemented all the modifications and this is the new G-Code for PrusaSlicer:

# "Start G-code" (above of your start_print gcode)
SET_PRINT_STATS_INFO TOTAL_LAYER=[total_layer_count]

# "After layer change G-code"
SET_PRINT_STATS_INFO CURRENT_LAYER={layer_num + 1}

@KevinOConnor
Copy link
Collaborator

Thanks. It looks fine to me. As a minor thing, it seems odd that calling SET_PRINT_STATS_INFO without CURRENT_LAYER will actively clear the current layer.

I'll give a few more days to see if others have comments. If there are no further comments I'll look to commit.

-Kevin

…ER are exists

Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
Signed-off-by: Stefan Dej <meteyou@gmail.com>
@meteyou
Copy link
Contributor Author

meteyou commented Oct 1, 2022

@KevinOConnor updated this PR to make it fit for the future for more attributes and now current_layer will only be updated when it is set or when you set a new total_layer.

@jschuh thx for your snipped. i only add a check, if one value is None.

@pedrolamas
Copy link
Contributor

LGTM, I just tested the latest code and worked as expected!

@meteyou might be good to update the PR title (and maybe the initial post text?)

@D4SK
Copy link
Contributor

D4SK commented Oct 2, 2022

Why make a klipper specific gcode command when most slicers aleary put this info as gcode comments.

You can check out my fork for an alternative approach where gcode.py broadcasts all gcode comments as an event, and a gcode_metadata module parses them based on the slicer used. This also allows for using the print time predictions put by the slicer. Unfortunately my implementation is more intertwined with the virtual_sdcard implementation etc. and cannot easily be merged. Maybe a stripped down version can be implemented into mainline klipper. I can certainly help with that.

@meteyou meteyou changed the title add LAYER G-Code for consistent layer count add SET_PRINT_STATS_INFO G-Code for pass slicer variables to Klipper Oct 3, 2022
@meteyou
Copy link
Contributor Author

meteyou commented Oct 3, 2022

@pedrolamas thx! i updated the title and the description

@D4SK parsing comments feels wrong to me. i would rather build some kind of preprocessor into moonraker that converts the comments to gcode and then have klipper process it.

@KevinOConnor KevinOConnor merged commit b0ffb26 into Klipper3d:master Oct 5, 2022
@KevinOConnor
Copy link
Collaborator

Thanks.

-Kevin

@meteyou meteyou deleted the feat/add-layer-command branch October 5, 2022 19:06
junland added a commit to project-gemstone/klipper-aquila-n32 that referenced this pull request Nov 8, 2022
* rp2040: Suppress spurious gcc v12 array bounds warning

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* z_thermal_adjust: get_temp hotfix

M105 expects get_temp method. Module crashes when gcode_id parameter
is set and M105 called. Add methods as hotfix.

Signed-off-by: Robert Pazdzior <robertp@norbital.com>

* print_stats: add `SET_PRINT_STATS_INFO` G-Code for pass slicer variables to Klipper (Klipper3d#5726)

This adds a gcode command that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>

* spi_flash: Add SKR2, SKR3 and Creality 4.2.2/7 to spi_flash (Klipper3d#5807)

As discussed with Arksine, he has created new working settings for the Creality 4.2.x boards as the original 4.2.7 entry did not work, plus addition of SKR2 and SKR3. Tested on 4.2.2 and SKR2 and SKR3EZ.

These are using the new skip_verify functionality that was recently merged due to them using SDIO

Also removed a double definition for `monster8` it was in main definitions and aliased.

Signed-off-by: James Hartley <james@hartleyns.com>

* config: Update generic-th3d-ezboard-lite-v2.0.cfg (Klipper3d#5785)

Corrected the build instructions for the TH3D EZBoard V2 to include the command to convert Klipper.elf to a SREC bin format named firmware.bin. The SREC format is required for the bootloader installed on the board.

Signed-off-by: Anthony Dellett <anthony.dellett@gmail.com>

* config: Add BigTreeTech Manta M4P & M8P board (Klipper3d#5812)

Signed-off-by: Alan.Ma from BigTreeTech <tech@biqu3d.com>

* test: Add btt manta config files to printers.test

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* stm32: Minor code movement in fdcan.c

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* stm32: No need for fdcan_ram global pointer in fdcan.c

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* stm32: Protect message ram with barrier() calls instead of voltaile in fdcan.c

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* lib: Remove unused gcc/ directories from samd21 and samd51 directories

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* lib: Add atmel same51 and same54 build definitions

This also replaces the samd51 component files with the definitions
from the same54 repository.

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* atsamd: Add Kconfig definitions for same51j19 and same54p20 chips

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* atsamd: Move bootloader_request() from usbserial.c to main.c

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* atsamd: Add support CANbus on ATSAME5x chips

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* config: Update duet3 mini config to recommend compiling for SAME54P20

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* lib: Update to latest can2040 code

Fix gpio function selection for PIO1 usage.
Minor variable name changes.
Allow scheduling tx "matched" event after a crc match.
Allow for up to 5 passive bits after unstuf_clear_state().
Pause PIO "rx" bit reception after 10 passive bits.
Signal the CPU to process data every 10 read bits.

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* workflows: Lock comments on old PRs (no activity in a year)

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* spi_flash: Update to fix SKR-3 config (Klipper3d#5827)

it seems I made an error with my code for the SKR 3, and I copied the code from the wrong host , this PR fixes that. I have just retested with the right code and works as expected, this is confirmed by @adelyser who brought the issue to my attention.

Signed-off-by: James Hartley <james@hartleyns.com>

* stm32: Remove incorrect "spi3" definition from stm32h7

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* toolhead: Capture current junction_deviation in a Move class

If a maximum acceleration is changed between two consecutive moves,
this allows to correctly compute the junction velocity between them.

Signed-off-by: Dmitry Butyugin <dmbutyugin@google.com>

* stm32: Enable instruction and data cache on stm32h7

Signed-off-by: Konstantin Vogel <konstantin.vogel@gmx.net>

* docs: Add step rate benchmark for stm32h7

Signed-off-by: Konstantin Vogel <konstantin.vogel@gmx.net>
Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* config: Add Artillery Sidewinder X2 2022 stock printer.cfg (Klipper3d#5810)

Works for unmodified stock Artillery Sidewinder X2, 
year 2022 with Artillery Ruby v1.2 mainboard. 
All by default connected things are working fine.

Build firmware with architecture STM32F401 with "No bootloader". 
Other settings keep ther defaults

Signed-off-by: Frank Roth <developer@freakydu.de>

* docs: Add a note about fixing underlying bugs in CONTRIBUTING.md

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* adxl345: Apply correct scaling for X,Y and Z axes

According to ADXL345/ADXL343 datasheets, at 3.3V supply voltage,
which is most frequent in the various boards, the sensitivity of
X and Y axes changes to 265 LSB/g from 256 LSB/g at 2.5V.

Signed-off-by: Dmitry Butyugin <dmbutyugin@google.com>

* stm32: fix USART ORE status flag handling

If an USART RX overrun happened on a stm32g0/f0/h7, the ORE flag
would get set by hardware. This flag would also trigger an interrupt.
The problem was that this flag was never cleared on these 3 mcu families
since the ORE flag clear sequence is different to all of the older
chips.
Since the ORE flag is not used in any meaningful way anyway, it was
disabled during the init sequence.

Signed-off-by: Alex Voinea <voinea.dragos.alexandru@gmail.com>

* stm32: Use stm32f0_serial.c on stm32h7 chips

The stm32h7 uses similar usart hardware as the stm32f0 and stm32g0
chips.  Use the same code implementation for all these chips.

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* stm32: Use consistent memory position/size on stm32h743

Use the same memory start address and size on both stm32h750 and
stm32h743.

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

* menu: Conditional display of common Control utils

This will hide features in the Klipper Display menu that isn't applicable
for the machine, therefore we can add extra alignment tools in the Control
menu as well.

Also conditionally displays Setup/Calibration options.

Signed-off-by: Nickolas Grigoriadis <nagrigoriadis@gmail.com>

* fdcan: Remove spurious executable flag on fdcan.c
zellneralex added a commit to mainsail-crew/mainsail-config that referenced this pull request Jan 14, 2023
These features are based on the idea of @pedrolamas.

You can use it in combination with the Klipper3d/klipper#5726 to easily do a PAUSE or M600 either at the next layer change or a defined Layer#

Signed-off-by: Alex Zellner <alexander.zellner@googlemail.com>
eliasbakken pushed a commit to intelligent-agent/klipper that referenced this pull request Jan 31, 2023
…les to Klipper (Klipper3d#5726)

This adds a gcode command that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>
zellneralex added a commit to mainsail-crew/mainsail-config that referenced this pull request Feb 11, 2023
* Feature: pause_next_layer & pause_at_layer

These features are based on the idea of @pedrolamas.

You can use it in combination with the Klipper3d/klipper#5726 to easily do a PAUSE or M600 either at the next layer change or a defined Layer#

Signed-off-by: Alex Zellner <alexander.zellner@googlemail.com>
Gi7mo pushed a commit to Gi7mo/klipper that referenced this pull request Feb 15, 2023
…les to Klipper (Klipper3d#5726)

This adds a gcode command that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>
@stenmic
Copy link

stenmic commented Apr 15, 2023

how does it work with cura? I can't get it to work.

@pedrolamas
Copy link
Contributor

how does it work with cura? I can't get it to work.

@stenmic use Klipper Preprocessor script for Cura

tntclaus pushed a commit to tntclaus/klipper that referenced this pull request May 29, 2023
…les to Klipper (Klipper3d#5726)

This adds a gcode command that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>
revilo196 pushed a commit to revilo196/klipper that referenced this pull request Jun 24, 2023
…les to Klipper (Klipper3d#5726)

This adds a gcode command that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>
rogerlz referenced this pull request in DangerKlippers/danger-klipper Dec 19, 2023
…les to Klipper (#5726)

This adds a gcode command that can be used insight the slicer to pass the total layer count and current layer information.

Signed-off-by: Stefan Dej <meteyou@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants