Arksine / moonraker

Web API Server for Klipper
https://moonraker.readthedocs.io
GNU General Public License v3.0
1.02k stars 392 forks source link

spoolman: connection and proxy improvements #792

Closed Arksine closed 5 months ago

Arksine commented 5 months ago

This Pull Request aims to resolve issues around Spoolman proxy errors. It appears that deleted spools or spool IDs that don't exist can lead to 404 responses that confuse frontends. To help resolve this two changes have been added:

1) Moonraker opens a websocket connection to Spoolman to listen for deleted spools. If a deleted spool ID matches Moonraker's current spool ID then it will be set to None and the spool ID changed event will be broadcast to clients. In addition the perisistent connection to Spoolman avoids attempts to make HTTP API requests when it is known that Spoolman is not available.

2) Moonraker's /server/spoolman/proxy API now provides an alternate response. It is currently opt-in via the use_v2_response argument. When set to true, successful responses to the proxy will be returned in the following format:

{
  "response":  <spoolman response object>,
  "error": null
}

If Spoolman returns an error then response will be null and error will contain error details:

{
  "response":  null,
  "error": {
    "status_code": 404,
    "message": "No spool with ID 3 found."
  }
}

This eliminates the ambiguity between Spoolman errors and Moonraker errors that can confuse frontends. If Moonraker returns an error response the frontend will know that Moonraker is the source of the error and can handle it appropriately.

@meteyou @pedrolamas @Clon1998 I believe this pull request is likely of interest to all of you.

Arksine commented 5 months ago

I should also note that this PR does not do validation when a spool_id is set, either through a frontend or from Klipper, so its possible to set a spool ID that doesn't exist in the Spoolman database. The reason for this is that it is still valid to set the spool_id when Spoolman isn't reachable. In addition, there isn't an absolute guarantee that an HTTP request to Spoolman won't timeout even when the websocket connection exists. If Donkie decides to expose the API over the websocket I'll revisit doing this.

pedrolamas commented 5 months ago

(cc'ing @matmen as he's been maintaining Spoolman integration in Fluidd)

Zeanon commented 5 months ago

Out of curiosity, I saw you already made some changes to moonraker which actually resolved the error. Is this just supposed to be a temporary fix?

meteyou commented 5 months ago

Thanks for the ping Arksine. I have prepared a PR for this improvement. Supporting the old and new versions in parallel in the GUI is also easily possible. This should allow the user to update without any problems.

Arksine commented 5 months ago

When testing my changes I noticed some behavior (not introduced by the changes themselves) that could be improved on. For example, the Spoolman component will report when new extrusion is detected, presuming that the configured "sync time" has elapsed. This can lead to a condition where some extrusion goes unreported until the spool ID is changed. If spoolman isn't connected during that change the extrusion is lost. I have refactored how reporting is done so its filament data is queued and reported on a timer. This eliminates the Locks, making the code cleaner. It also keeps spool data persistent if Spoolman is not available.

In addition I noticed that the spoolman module wasn't accounting for tool changes, so I added that (although I can't test it as I don't have any printers with multiple tools). It should work fine though.

Finally, I added a new endpoint and a new notification so frontends can be notified of Spoolman's connection status. The /server/spoolman/status endpoint reports spoolman_connected, pending_reports, and spool_id. The spool_id is redundant but I figured it made sense to add it in there. The pending_reports contain the queued extrusion yet to be reported to Spoolman, and spoolman_connected is self explanatory.

The notify_spoolman_status_changed simply reports the new connection state when its changed.