openhab-addons
openhab-addons copied to clipboard
openhab
[moonraker] Initial contribution
New Binding for Klipper/Moonraker controlled 3D Printers
This is the initial contribution for a new binding for 3D printers controlled by Klipper via the Moonraker Web API.
A compiled version can be found at: https://github.com/arjanmels/openhab2-addons/releases/tag/v1.0.0
Forum discussion: https://community.openhab.org/t/klipper-moonraker-binding-for-3d-printer/118958
For more information on Klipper: klipper | Klipper is a 3d-printer firmware (klipper3d.org) For more information on the Moonraker Web API: Arksine/moonraker: Web API Server for Klipper (github.com)
Hi. Just FYI - it looks like your PR includes several duplicate files with names that differ only in case (e.g. MoonrakerConfiguration.java and moonrakerConfiguration.java).
@bobadair Fixed the duplicate file names with different casing by starting applying patch file to current master. (Looses history, but that was not that significant yet anyway.)
Also removed teh dependencies on org.apache.commons.
FYI the channels need to be lowerCamelCase, see here: https://www.openhab.org/docs/developer/guidelines.html#java-coding-style
@Skinah The channels directly match names as received from the xml. I could built in a translation function (e.g. replace _ with next letter capital). Also I use a double __ in some names to create a virtual level of hierarchy. (To automatically cluster some of the recieved info into one channel group.)
I wonder if adhering to the lowerCamelCase guideline is worth the extra code/complications. Would like your view.
I am not doing a review I just wanted to give you something to work on whilst you wait as it can be a while due to a back log. I suspect the answer will be yes to implement a java Map. You can always ask in the forum in the developer section if there are reasons to ignore the rule. I do not know.
@Skinah The channels directly match names as received from the xml. I could built in a translation function (e.g. replace _ with next letter capital). Also I use a double __ in some names to create a virtual level of hierarchy. (To automatically cluster some of the recieved info into one channel group.)
I wonder if adhering to the lowerCamelCase guideline is worth the extra code/complications. Would like your view.
I haven't looked into your code, but this is a common scenario. Depending on your implementation this might be really simple to fix:
@XStreamAlias("Name_Of_XML_element") private String nameOfProperty;
@arjanmels It's been a long time you submitted your binding. Are you still interested in a review?
The channel names should be preferably camelCase, but there are many bindings, which use underscores. Double underscores should be avoided, though.
@fwolter I have limited time available at the moment and I see interest for this binding is not very large. So I am hesitant to put the time in or just publish it on the market place (with the disadvantage that a lot of the auto maintenance is not happening).
The fact that nobody responded to your community post doesn't mean there is no interest. The link click counters in your post show a quite contrary picture. You can keep this open and raise your hand as soon as you have more time available to implement any review feedback. I will do a complete review, then. WDYT?
I am going to change my Marlin printer over to Klipper as I see it as better, so I am interested in this binding and hope you continue to go through with it since you have already done the bulk of the work.
What advantages or extra features does this way have over using the Octoprint API?
Any lack of questions on the forum probably stem from:
Most people don't have a 3d printer. Most people that do, use Marlin based firmware on the printers and not Klipper. Most people that do use Klipper probably already have it working in openHAB via the Octoprint API or by using a MQTT plugin.
@fwolter Good proposal. Once I have more time I'll look into the double underscores: will require some rewrite as I use it for automatic parsing of properties. Do you have another suggestion for an acceptable symbol to separate parts of a name (which can contain a single underscore themselves)?
Otherwise I might need to revert to tables (which I don't like as any change to the api will require a change to this bniding. On the other hand there are so many exception being handled via switch statements anyway, that forward compatibility is questionable anyway.
@Skinah The more direct access allows more details and also to features like the on/off switch commands.
I won't insist on changing the channel names to camelCase, but please reconsider if you really need an additional separation in channel names for a hierachy. Maybe there's a third solution beside this and using tables?
@arjanmels Is this ready to be merged? If you can reply to the last post by fwolter that would let us know if it is ready or if it still needs to wait for changes. Thanks for the contribution.
@skinah, @fwolter Let me try to find some time to revisit this and see if I can find a better solution. Also some users report and issue with dependency (https://community.openhab.org/t/klipper-moonraker-binding-for-3d-printer/118958/6). Will aim to address this weekend.
@jlaur Correct, I am busy with overhaul to address the review feedback. Will let you know once a new review is needed.
, I am busy with overhaul to address the review feedback. Will let you know once a new review is needed.
Hi @arjanmels are you planning to proceed with this PR? it has been around here for quite some time now and i'm happy to review or help you out, but if you can't proceed, it might be better to just close this.
Unfortunately it looks like we cannot bring this PR to the finish line. After two years of inactivity i close this PR. If there is a need to restart, feel free to re-open or start a new PR.