-
-
Notifications
You must be signed in to change notification settings - Fork 795
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
Machine integration #4824
Machine integration #4824
Conversation
@matmair Do you think the Base Drivers should be exported from the machine or plugin app? (The notification adapters defined in common are exported through the plugin app) |
@wolflu05 if you are not against it I would suggest it. The idea is that there always is a simple and steady path for the things we expose for plugins while we are free to refactor the inner workings and structure without breaking plugins. |
I'm planning on add the ability that machines can expose their current states. E.g. for a label printer: Ready, Printing, Disconnected, ... do you think this makes sense to define per Machine Type or per CustomMachineDriver? (Want to have a proper Interface with Enums). Per MachineType has the ability that they are equal across all label printers. However something custom like "out of labels", "machine opened" ... is not possible. I guess there are more different states on other machine types like PnP. What about having both abilities? MachineType specific states and a machine status that's just a simple string? |
@wolflu05 status/state, in general, needs a bit of a refactor (see #4289, #4589). To keep this PR somewhat my suggestion would be to add enums to the machine type and enable drivers to define current state (the enum from machine) and state text (free text, by default from machine type state but can be set to "anything") |
- get_machines function on driver - get_machine function on driver - initialized attribute on machine
9 months passed now, this is my biggest PR and I learned a lot about InvenTree and Django during that time. Now this PR is finally ready for review 🚀 . @SchrodingersGat @matmair I can recommend to start reviewing this by reading the docs to understand how the machine registry, custom drivers and types work before taking a deeper look into the code. As an example, I adapted my cups label printer plugin to use the new driver interface. You can find it here if you want to see a real driver actually being implemented. During this PR I collected a list of related things that can be further improvements (see initial comment) which I will post in a new issue once this is merged. If you have any questions, feel free to comment, I'm really curious about your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; huge work effort, thanks @wolflu05
@wolflu05 this is a massive effort to get this up and running, apologies for any road blocks along the way :) This all looks good, but please appreciate that with such a large contribution, there are going to be some teething issue and bugs we have not spotted. It all appears to be in order and I am happy to merge now, so that we have some time before the next release to spot any obvious issues. Without any machines to fill in myself, it is hard to create any test data to work with. As we progress, it would be great to work on some more examples for the documentation of different types of machines, as we build those up. No doubt you will be tagged in a number of issues / questions around all this :) BTW regarding the style issue - once #6481 is merged, please merge master back in to your branch and we should be good to go. |
Congrats and thanks @wolflu05 - let's hope it is a smooth integration from here! |
This is actually the largest PR I've ever done. All good, feel free to ping me if there is anything I can help with. Hopefully we don't find to much issues 😆.
If you want to test it, you can create a simple driver and place it anywhere in the code like this: Testing driverfrom plugin.machine.machine_types import LabelPrinterBaseDriver
class TestLabelPrinterDriver(LabelPrinterBaseDriver):
"""Label printer driver for Dymo printers."""
SLUG = "test-driver"
NAME = "Test Driver"
DESCRIPTION = "Test label printing driver for InvenTree"
MACHINE_SETTINGS = {
"CONNECTION_URL": {
"name": "Connection url",
"description": "Connection url for the machine in format user@server:port/route",
"required": True,
}
}
def print_labels(self, machine, label, items, request, **kwargs):
print("==== Print label")
print(machine, label, items, request, kwargs)
print(machine.get_setting('CONNECTION_URL', 'D'))
print("="*10)
Definitely, I think the documentation around this has big potential, also the generated part from the docstrings.
Thank you very much for merging, this is a great step forward, I actually really hadn't expected to get this merged that quick. And yes, let's hope it is a smooth integration 😆 . |
This PR adds the machines feature described in #4789 .
fixes #4789
📝 Todos
⚠️ Before merge
♻️ External PRs/Refactors
💡Further ideas (Not this PR)
Moved to #6486