Skip to content

[threedprinter] Initial contribution - #20753

Open
computergeek1507 wants to merge 21 commits into
openhab:mainfrom
computergeek1507:threedprinter
Open

[threedprinter] Initial contribution#20753
computergeek1507 wants to merge 21 commits into
openhab:mainfrom
computergeek1507:threedprinter

Conversation

@computergeek1507

@computergeek1507 computergeek1507 commented May 18, 2026

Copy link
Copy Markdown
Contributor

[3D Printer Binding] Initial contribution

Description

I created a binding to monitor 3D printers status, temperatures, and job progress.
PrusaLink, Klipper, and OctoPrint firmware/server platforms are supported.
It uses HTTP to poll the printer to update the status.

I have build it locally and tested it with my Prusa and Snapmaker printers with the 5.2M4 release.

@computergeek1507
computergeek1507 requested a review from a team as a code owner May 18, 2026 13:55
@wborn wborn changed the title Threedprinter [3D Printer Binding] Initial contribution May 18, 2026
@wborn wborn added the new binding If someone has started to work on a binding. For a new binding PR. label May 18, 2026
@wborn
wborn requested a review from Copilot May 28, 2026 07:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR contributes a new threedprinter binding for monitoring and controlling local 3D printers over HTTP, supporting PrusaLink, Klipper/Moonraker, and OctoPrint.

Changes:

  • Adds binding metadata, Karaf feature, BOM/module registration, README, and thing/channel/config XML.
  • Implements shared HTTP polling and platform-specific handlers for status, temperatures, progress, previews, and commands.
  • Adds DTOs and configuration classes for PrusaLink, Klipper, and OctoPrint APIs.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
bundles/pom.xml Registers the new binding module.
bom/openhab-addons/pom.xml Adds the binding artifact to the add-ons BOM.
bundles/org.openhab.binding.threedprinter/pom.xml Defines the new binding Maven module.
bundles/org.openhab.binding.threedprinter/NOTICE Adds standard project licensing notice.
bundles/org.openhab.binding.threedprinter/README.md Documents supported platforms, configuration, channels, and examples.
bundles/org.openhab.binding.threedprinter/src/main/feature/feature.xml Adds Karaf feature definition.
bundles/org.openhab.binding.threedprinter/src/main/resources/OH-INF/addon/addon.xml Adds binding add-on metadata.
bundles/org.openhab.binding.threedprinter/src/main/resources/OH-INF/config/config.xml Defines thing configuration parameters.
bundles/org.openhab.binding.threedprinter/src/main/resources/OH-INF/i18n/threedprinter.properties Adds localized labels, descriptions, and status messages.
bundles/org.openhab.binding.threedprinter/src/main/resources/OH-INF/thing/thing-types.xml Defines PrusaLink, Klipper, and OctoPrint thing types.
bundles/org.openhab.binding.threedprinter/src/main/resources/OH-INF/thing/channel-types.xml Defines shared printer status/control channels.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/ThreedprinterBindingConstants.java Adds binding, thing, channel, and state constants.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/ThreedprinterHandlerFactory.java Creates handlers for supported thing types.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/AbstractPrinterHandler.java Adds shared scheduling and HTTP helpers.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/PrusaLinkHandler.java Implements PrusaLink polling and controls.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/KlipperHandler.java Implements Moonraker/Klipper polling and controls.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/OctoPrintHandler.java Implements OctoPrint polling and controls.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/config/PrusaLinkConfiguration.java Adds PrusaLink configuration model.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/config/KlipperConfiguration.java Adds Klipper configuration model.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/config/OctoPrintConfiguration.java Adds OctoPrint configuration model.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/dto/prusa/PrusaStatusResponse.java Adds PrusaLink status DTOs.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/dto/klipper/KlipperObjectsResponse.java Adds Moonraker object-query DTOs.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/dto/klipper/KlipperMetadataResponse.java Adds Moonraker metadata DTOs.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/dto/octoprint/OctoPrintPrinterResponse.java Adds OctoPrint printer DTOs.
bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/dto/octoprint/OctoPrintJobResponse.java Adds OctoPrint job DTOs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bundles/org.openhab.binding.threedprinter/pom.xml Outdated
Comment thread bom/openhab-addons/pom.xml Outdated
Comment thread bundles/org.openhab.binding.threedprinter/pom.xml Outdated
@jlaur jlaur changed the title [3D Printer Binding] Initial contribution [threedprinter] Initial contribution Jul 28, 2026
computergeek1507 and others added 9 commits August 1, 2026 09:45
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
The job-preview channel was only pushed a state the moment the print
filename changed, so an item linked to that channel afterward (while
the same file was still active) never received a value. Cache the
last fetched thumbnail and re-push it every refresh cycle, and add
debug logging to the previously-silent failure paths.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Same root cause as the Klipper fix: the job-preview channel was only
pushed a state the moment the file/thumbnail reference changed, so an
item linked afterward never received a value until the next print job.
Cache the last fetched thumbnail and re-push it every refresh cycle.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Co-authored-by: Wouter Born <github@maindrain.net>
Signed-off-by: Scott Hanson <computergeek1507@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

Suppressed comments (11)

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/KlipperHandler.java:186

  • time-remaining is defined as Number:Time, but it is updated with a DecimalType. Use QuantityType with seconds so the channel publishes the correct quantity type.
        if (display != null && stats != null && display.progress > 0) {
            long elapsed = (long) stats.printDuration;
            long remaining = display.progress < 1.0 ? (long) (elapsed / display.progress - elapsed) : 0L;
            updateState(CHANNEL_TIME_REMAINING, new DecimalType(remaining));
        }

bundles/org.openhab.binding.threedprinter/README.md:114

  • The print-speed and fan-speed channels are Number:Dimensionless (unitHint %), but the Items example uses plain Number. Update the example types so item-channel links work without conversion issues.
Number   MK4_PrintSpeed     "Print speed [%d %%]"            { channel="threedprinter:prusaprinter:mk4:print-speed" }
Number   MK4_FanSpeed       "Fan speed [%d %%]"              { channel="threedprinter:prusaprinter:mk4:fan-speed" }

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/PrusaLinkHandler.java:126

  • The channel-types define time-elapsed/time-remaining as Number:Time, but the handler updates them with DecimalType. This can lead to type mismatches on the event bus and items not accepting the updates. Use QuantityType with seconds instead.
            updateState(CHANNEL_JOB_PROGRESS, new DecimalType(job.progress));
            updateState(CHANNEL_TIME_ELAPSED, new DecimalType(job.timePrinting));
            updateState(CHANNEL_TIME_REMAINING, new DecimalType(job.timeRemaining));

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/PrusaLinkHandler.java:145

  • When there is no active job file, the preview cache is cleared but the job-preview channel state is not. This leaves a stale thumbnail on linked items/UI. Explicitly clear the channel (e.g., UnDefType.UNDEF) when file == null.
        if (file == null) {
            updateState(CHANNEL_JOB_NAME, new StringType(""));
            lastPreviewFilename = "";
            lastPreviewState = null;
            return;

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/OctoPrintHandler.java:150

  • The channel-types define time-elapsed/time-remaining as Number:Time, but this handler updates them with DecimalType. Use QuantityType with seconds to match the channel item type and avoid item/state type mismatches.
            updateState(CHANNEL_JOB_PROGRESS, new DecimalType(progress.completion > 0 ? progress.completion : 0));
            updateState(CHANNEL_TIME_ELAPSED, new DecimalType(progress.printTime));
            updateState(CHANNEL_TIME_REMAINING, new DecimalType(progress.printTimeLeft));

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/OctoPrintHandler.java:184

  • When filename is blank, the preview cache is cleared but the job-preview channel is not. This can leave the last thumbnail displayed even though no job is active. Clear the channel state when resetting the cache.
                } else {
                    lastPreviewFilename = "";
                    lastPreviewState = null;
                }

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/KlipperHandler.java:147

  • time-elapsed is defined as Number:Time, but the handler updates it with a DecimalType. Use a QuantityType with seconds to match the channel item type.

This issue also appears on line 182 of the same file.

            updateState(CHANNEL_JOB_NAME, new StringType(stats.filename));
            updateState(CHANNEL_TIME_ELAPSED, new DecimalType((long) stats.printDuration));

bundles/org.openhab.binding.threedprinter/src/main/java/org/openhab/binding/threedprinter/internal/handler/KlipperHandler.java:163

  • When no file is being printed (stats.filename blank), the preview cache is cleared but the job-preview channel is not. Clear the channel state as well to avoid showing a stale thumbnail.
            } else {
                lastPreviewFilename = "";
                lastPreviewState = null;
            }

bundles/org.openhab.binding.threedprinter/src/main/resources/OH-INF/i18n/threedprinter.properties:64

  • job-preview is defined in channel-types.xml, but there are no corresponding i18n entries in threedprinter.properties (other channels have them). Add label/description keys so the channel can be localized consistently.
channel-type.threedprinter.pause-resume.label = Pause / Resume
channel-type.threedprinter.pause-resume.description = ON pauses the print, OFF resumes it.
channel-type.threedprinter.cancel.label = Cancel Print
channel-type.threedprinter.cancel.description = Send ON to cancel the current print job.

bundles/org.openhab.binding.threedprinter/README.md:108

  • The channel item types for progress/time/speeds are Number:Dimensionless and Number:Time (per channel-types.xml), but the Items example uses plain Number. Users copying this will get type mismatches when linking items to channels; update the example item types accordingly.

This issue also appears on line 113 of the same file.

Number   MK4_Progress       "Progress [%.1f %%]"             { channel="threedprinter:prusaprinter:mk4:job-progress" }
Number   MK4_TimeElapsed    "Time elapsed [%d s]"            { channel="threedprinter:prusaprinter:mk4:time-elapsed" }
Number   MK4_TimeRemaining  "Time remaining [%d s]"          { channel="threedprinter:prusaprinter:mk4:time-remaining" }

bundles/org.openhab.binding.threedprinter/README.md:136

  • The print-speed channel type sets a minimum of 1, but the sitemap example allows 0 (minValue=0). Align the example with the channel constraints to avoid UI validation/errors when users copy it.
        Slider item=MK4_PrintSpeed minValue=0 maxValue=200 step=10

@lsiepel lsiepel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments I gathered earlier. Might not be valid anymore, flush them now to not waste them

Comment thread bundles/org.openhab.binding.threedprinter/README.md Outdated

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution. The overall structure looks promising, but I found a few functional issues that should be addressed before merging, particularly the PrusaLink job-control endpoints and the handling of unit-aware channels.

The review comments are AI-assisted.

Comment thread bundles/org.openhab.binding.threedprinter/README.md Outdated
Comment thread bundles/org.openhab.binding.threedprinter/README.md Outdated
computergeek1507 and others added 2 commits August 1, 2026 16:39
- Rename prusaprinter thing-type to prusa-link (lower-case-hyphen
  convention) and use the Printer semantic tag instead of
  NetworkAppliance for all three thing types.
- Use QuantityType with proper units (percent/seconds) for
  job-progress, time-elapsed, time-remaining, print-speed and
  fan-speed instead of plain DecimalType, and accept QuantityType
  commands for print-speed/fan-speed/temperature setpoints.
- Move command handling off the ThingHandler callback thread by
  dispatching through the handler scheduler, since it performs
  blocking HTTP requests.
- Check HTTP status codes on write commands (pause/resume/cancel/
  gcode) and log failures instead of silently assuming success.
- Reset job-name/progress/elapsed/remaining/preview channels to
  UNDEF when the printer reports no active job, via a shared
  clearJobState() helper.
- Reject unsupported command types for temperature/speed/fan setters
  instead of silently defaulting to 0 (which sent M104/M140 S0 and
  turned off the heater).
- Fix PrusaLink pause/resume/cancel to use the documented
  /api/v1/job/{id}/... endpoints with the job's actual ID instead of
  the bare /api/v1/job endpoint.
- Use Jetty's HttpStatus constant instead of a magic 200, and catch
  TimeoutException/ExecutionException/InterruptedException
  specifically instead of a blanket catch(Exception).
- Add missing job-preview i18n translations, and fix the print-speed
  minimum (1, not 0) to match the channel definition in the README
  and sitemap example.
- Note in the README that PrusaLink was verified against a real MK4
  on Buddy firmware 6.5.7+12836, which exposes the /api/v1 endpoints
  used by this handler.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous review findings. The PrusaLink job-control endpoints, unit-aware channel handling, asynchronous command execution, semantic tags, and job-state cleanup are substantially improved.

A thorough pass against the openHAB development guidelines and review checklist, together with the upstream APIs, found a few remaining functional issues. Most importantly, the writable PrusaLink temperature/speed/fan channels use an endpoint that Buddy firmware does not expose, and the OctoPrint thumbnail URL does not match the path generated by the PrusaSlicer Thumbnails plugin.

There are also a few smaller correctness and static-analysis issues that should be addressed before merging.

The review comments are AI-assisted.

Comment thread bundles/org.openhab.binding.threedprinter/README.md
- Make PrusaLink temperature/print-speed/fan-speed channels read-only,
  since Buddy firmware has no G-code command endpoint to actually set
  them; report fan speed as raw RPM instead of a fabricated percentage
  (the max RPM varies by printer model).
- Distinguish transport failures from HTTP error responses in the HTTP
  helpers so an auth failure isn't reported the same as an unreachable
  printer, and so commands update the Thing status instead of silently
  staying ONLINE.
- Fix OctoPrint thumbnail URL construction to match how the PrusaSlicer
  Thumbnails plugin actually names/serves files (relative path + .png).
- Clear the cached job preview when a new job has no usable thumbnail
  instead of leaving the previous job's image showing.
- Handle PrusaLink's optional time_remaining field as UNDEF rather than
  a false "0s remaining".
- Fix a nullness warning, use Locale.ROOT for state normalization, and
  clean up remaining Checkstyle formatting notices.
- Shorten channel labels to fit the 25-character guideline.
- Document which printer models were actually tested vs. merely
  expected to be compatible.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The previous review findings have been addressed well, and CI is clean on the current head. Three functional API-state issues remain:

  • Klipper print progress currently depends on optional display_status state.
  • Klipper is marked ONLINE when Moonraker is reachable even if Klippy itself is not ready.
  • OctoPrint temperature targets can be absent, but the current DTO represents an absent target as 0 °C.

Detailed findings are included inline.

- Klipper: query virtual_sdcard instead of the optional display_status
  object and use its progress value for job-progress/time-remaining,
  so file-print progress no longer depends on a display being
  configured.
- Klipper: query webhooks and only mark the Thing ONLINE when
  webhooks.state is "ready", since Moonraker can keep responding while
  Klippy itself is starting, shut down, or in an error state.
- OctoPrint: make the tool/bed temperature target nullable and publish
  UNDEF when absent instead of a fabricated 0 °C setpoint, since
  OctoPrint reports null when no target is currently set.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous Klipper and OctoPrint findings. The current CI and static-analysis run are clean, and the previously reported Klipper state/progress and OctoPrint temperature-target issues have been addressed.

Three OctoPrint correctness issues remain around the /api/job response: nullable file fields can cause runtime failures, nullable progress values are currently converted to real zero values, and failures of the job request are silently ignored while the Thing remains ONLINE.

Detailed findings are included inline.

This review was AI-assisted.

- Model file.name/display/path and progress.completion/printTime/
  printTimeLeft as nullable, since OctoPrint reports them as JSON null
  when unknown rather than omitting them; the previous primitive/
  default-"" fields either threw an NPE on isBlank() or silently
  reported a fabricated 0%/0s.
- Publish UNDEF for progress fields OctoPrint hasn't reported yet
  instead of a misleading zero.
- Report /api/job transport and HTTP failures the same way as the
  /api/printer request instead of silently returning, which left the
  Thing ONLINE with stale job state.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous OctoPrint findings. The nullable job fields and progress values are now handled correctly, failures of the OctoPrint job request are reported, and the current CI/static-analysis run is clean.

One functional error-handling issue remains that should be addressed before merge: normal API-level command rejections can currently mark an otherwise reachable printer OFFLINE. There are also two smaller correctness issues around the auxiliary PrusaLink job request and OctoPrint transition-state handling.

Details are included inline.

This review was AI-assisted.

computergeek1507 and others added 2 commits August 19, 2026 09:27
… transitions

- Add markCommandFailure() to distinguish application-level command
  rejections (404/409, e.g. a stale job ID or a cancel racing a job
  that already finished) from actual transport/auth failures. The API
  was reached and responded normally in the 404/409 case, so the Thing
  now stays ONLINE and a refresh is triggered to reconcile channels,
  instead of incorrectly going OFFLINE. Applied consistently across
  PrusaLink, Klipper, and OctoPrint command handlers.
- PrusaLink: distinguish the documented 204 "no active job" response
  from /api/v1/job from actual request failures (transport error, auth
  failure, unexpected HTTP status, malformed JSON). A failure no longer
  masquerades as "no job"; if the job ID is unchanged since the last
  successful fetch, the existing name/preview are left in place rather
  than being replaced by a transient failure.
- OctoPrint: check the cancelling/pausing transition flags before the
  general printing/paused flags, since OctoPrint keeps printing=true
  while cancelling or pausing, which made the cancelling->BUSY branch
  unreachable. Add the previously-missing pausing flag.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
- Klipper: discover additional extruders (extruder1, extruder2, ...,
  no fixed upper bound - Klipper itself supports up to 99) via a
  one-time /printer/objects/list query at startup, extend the polling
  query with whichever ones are actually present, and dynamically add
  nozzle-temperature-N/nozzle-temperature-setpoint-N channel pairs.
  Setpoint commands use SET_HEATER_TEMPERATURE HEATER=<name> TARGET=.
- OctoPrint: read tool1, tool2, ... directly out of the temperature
  map already returned by the existing /api/printer poll (the printer
  profile determines the count, so no extra discovery request is
  needed) and add the same channel pattern. Setpoint commands use
  M104 T<index> S<temp>.
- PrusaLink is intentionally unchanged: its status API only ever
  reports one active nozzle, even on tool-changer machines like the
  Prusa XL.
- Document the new nozzle-temperature-N channel pattern in the README.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
@lsiepel
lsiepel requested a review from wborn August 19, 2026 14:34

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous findings. The PrusaLink auxiliary job request and OctoPrint transition-state handling are fixed.

One command-status issue remains for Klipper/Moonraker: Moonraker reports Klippy command errors as HTTP 400, which still falls through to the connectivity-failure path and can mark an otherwise reachable printer OFFLINE. Details are included inline.

There is also a small semantic-tag correction for the PrusaLink fan RPM channel.

This review was AI-assisted.

computergeek1507 and others added 2 commits August 19, 2026 17:29
- markCommandFailure() now also treats HTTP 400 as an application-
  level command rejection. Moonraker's Klippy bridge converts a
  rejected G-code command (e.g. PAUSE when nothing is printing) into
  ServerError(..., 400) and forwards it as-is, so it was previously
  falling through to markHttpFailure() and could mark an otherwise
  reachable Klipper printer OFFLINE.
- prusa-fan-speed is a read-only measured RPM value, not a setpoint;
  tag it Measurement instead of Setpoint.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous findings. The command-status handling and PrusaLink fan semantic tag are now fixed, and the current CI/static-analysis run is clean.

The new multi-toolhead support introduces two closely related correctness issues: the primary nozzle setpoint channels report a fixed first tool (extruder / tool0), while their commands do not consistently target that same fixed tool. On multi-tool printers this can change the temperature of a different nozzle than the one represented by the channel.

Details are included inline.

This review was AI-assisted.

Klipper's unqualified M104 applies to the currently active extruder,
not necessarily the fixed extruder backing the primary nozzle
channels, so sending it could heat the wrong tool once a second
extruder is active. Use SET_HEATER_TEMPERATURE HEATER=extruder to
always target the same extruder the channel reports.

Similarly, OctoPrint's primary and extra nozzle setpoint channels now
use POST /api/printer/tool with explicit tool targets instead of raw
M104 G-code, so every channel always controls the tool it reports.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tV8uS8uwfLymjBw2mx2fn
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous multi-tool setpoint findings. Klipper now explicitly targets the primary extruder, and OctoPrint explicitly targets the corresponding tool.

One lifecycle issue remains in the Klipper multi-tool support: dynamically added nozzle channels can already exist when a new handler is created, but discovery currently adds them again unconditionally. This can make the first scheduled refresh fail after a binding restart, and for managed Things also after an openHAB restart.

There is also a smaller stale time-remaining case when a new Klipper print starts at 0% progress.

Details are included inline.

This review was AI-assisted.

Dynamically added nozzle channels can already exist on the Thing when
a new handler is created (retained across a binding restart, and
persisted across an openHAB restart for managed Things). Discovery
was unconditionally re-adding them, which throws IllegalArgumentException
from ThingBuilder.withChannel() on the first refresh and halts polling.
Only add channels that aren't already present, mirroring
OctoPrintHandler, while still rebuilding extraSetpointHeaterByChannel
for every discovered extruder.

Also set time-remaining to UNDEF when virtual_sdcard.progress is 0,
since Klipper resets progress to 0 as soon as a new print is loaded
while print_stats.filename already reflects the new job, which was
leaving the previous job's estimate stale.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tV8uS8uwfLymjBw2mx2fn
Signed-off-by: Scott Hanson <scooter_seh@yahoo.com>

@wborn wborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the previous Klipper multi-tool lifecycle findings. The duplicate-channel and stale time-remaining fixes are correct, and the current CI/static-analysis run is clean.

One configuration-lifecycle issue remains. openHAB reinitializes the same handler instance after configuration updates, but the binding retains endpoint-derived configuration, discovery, and preview state across dispose() / initialize(). This can leave a reconfigured Thing using state from the previous printer, including skipped Klipper extruder discovery and stale preview data.

Details are included inline.

This review was AI-assisted.

}

@Override
public void dispose() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BaseThingHandler.handleConfigurationUpdate() reuses the same handler instance by calling dispose() and then initialize(), but this only cancels the refresh job; the subclasses retain endpoint-specific configuration, discovery, and preview state.

For example, after changing a Klipper Thing to another host, extrudersDiscovered remains true, so discovery is skipped and queryUrlSuffix / extraSetpointHeaterByChannel can still describe the previous printer. The preview cache can likewise reuse the previous printer's image when the new printer has the same filename. If the new configuration is invalid, the early return from initialize() also leaves config pointing at the previous endpoint.

Please invalidate endpoint-derived state when the handler is reinitialized/configuration changes, including the corresponding config and preview caches in the PrusaLink and OctoPrint handlers.

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

Labels

new binding If someone has started to work on a binding. For a new binding PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants