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

spoolman: connection and proxy improvements #792

Merged
merged 15 commits into from
Jan 21, 2024

Conversation

Arksine
Copy link
Owner

@Arksine Arksine commented Jan 19, 2024

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
Copy link
Owner Author

Arksine commented Jan 19, 2024

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
Copy link
Contributor

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

@Zeanon
Copy link

Zeanon commented Jan 19, 2024

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
Copy link
Contributor

meteyou commented Jan 20, 2024

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
Copy link
Owner Author

Arksine commented Jan 20, 2024

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.

Connect to the spoolman sevice via websocket to receive
spool events.  In addition, this gives Moonraker a persistent
connection to know when the service is available.

Signed-off-by:  Eric Callahan  <arksine.code@gmail.com>
The loop time is monotonic and can't be affected by
changes to the system clock.

Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
The exisiting implementation of spoolman's proxy endpoint
returns responses and errors exactly as they are received
by spoolman.  This creates a problem of ambiguity, as the
frontend cannot easily diffentiate between an error returned
by Moonraker and an error returned by Spoolman.

This implements a "v2" alternate response to proxy requests.
All requests to spoolman will return success, with responses
wrapped in a top level object.  Successful requests will be
returned in a "spoolman_response" object, errors in a
"spoolman_error" object.

Initially v2 responses will be opt-in to prevent breaking existing
spoolman implementations.  However, as of this commit the v1
response is deprecated and will be removed in a future version
of Moonraker.

Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by: Eric Callahan <arksine.code@gmail.com>
Use a python dict to act as a queue for reporting used filament
per spool.  This eliminates the need for locks and resolves
potential issues with spool changes when the Spoolman
service is not available.

In addition, add support for tracking multiple tools

Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Avoid running multiple callbacks if a timer is stopped and restarted
in quick succession.

Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
This performs a significant refactor to the ZipDeploy class, making it near identiical to WebClientDeploy.  Zipped applications have the same
"release_info" requirement as web clients.  Unlike web clients, they may also
configure the dependency and service options available to git repos.

The ZipDeploy class can also support we clients, eliminating duplicate code
and the need to keep web_deploy.py.

Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Reduce verbosity in release mode and add connection status to the log rollover.

Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
Signed-off-by:  Eric Callahan <arksine.code@gmail.com>
@Arksine Arksine force-pushed the dev-spoolman-websocket-20240116 branch from a9b6deb to d10ce87 Compare January 21, 2024 13:24
@Arksine Arksine merged commit d10ce87 into master Jan 21, 2024
2 checks passed
@Arksine Arksine deleted the dev-spoolman-websocket-20240116 branch January 21, 2024 15:24
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