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

initial contribution of IPP binding #164

Closed
wants to merge 1 commit into from

Conversation

peuter
Copy link
Member

@peuter peuter commented Mar 17, 2015

including mDNS-based disovery service

@kaikreuzer
Copy link
Member

Thanks - I'll try to review it soon!

@kaikreuzer kaikreuzer self-assigned this Mar 18, 2015
@kaikreuzer kaikreuzer added the new binding If someone has started to work on a binding. For a new binding PR. label Mar 19, 2015
@kaikreuzer kaikreuzer modified the milestone: 2.0.0 alpha2 Apr 4, 2015
xsi:schemaLocation="http://eclipse.org/smarthome/schemas/binding/v1.0.0 http://eclipse.org/smarthome/schemas/binding-1.0.0.xsd">

<name>Cups Binding</name>
<description>This is the binding for Cups.</description>
Copy link
Member

Choose a reason for hiding this comment

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

Better briefly explain what Cups is about, i.e. "for the Common Unix Printing Service (CUPS)"

@kaikreuzer
Copy link
Member

However you like - we can also simply rename this PR, then we have the history in one place.

@peuter peuter changed the title initial contribution of Cups binding initial contribution of IPP binding Apr 16, 2015
@peuter
Copy link
Member Author

peuter commented Apr 16, 2015

Ok, I rebased this branch on my ipp-developemt branch, should be ready to merge now.

@kaikreuzer kaikreuzer assigned kaikreuzer and unassigned peuter Apr 17, 2015
@kaikreuzer
Copy link
Member

Thanks! I have just released a new stable of ESH, so that the mDNS fix is now available in openHAB 2 and I was able to test this binding.

One observation: From the 3 channels, only "waiting Jobs" seems to be of most interest. For the other two, I don't see too many use cases. So would it maybe make sense to mark the other two channels as "advanced", so that they do not appear in the UI by default? And I would possibly rename "Jobs" in "total jobs".

Another observation: I have a Canon printer, which is discovered but cannot be accessed. This is because hostname is null as the ServiceInfo does not return any value on getServer(). Nonetheless, the ServiceInfo contains the IP address in the field ipv4Addresses. Should maybe the IP addresses by taken instead of the hostname?

@peuter
Copy link
Member Author

peuter commented Apr 20, 2015

I agree with setting the channels to advanced as they aren´t really of much interest.
I tried to replace the whole usage of hostname in the creation of the discovery result.
I also changed the ThingUID to use service.getName(), as this should be unique.
What happens if the printer´s ip address changes? The ThingUID does not change so the URL-property gets updated, right?

@buildhive
Copy link

openhab » openhab2 #4 SUCCESS
This pull request looks good
(what's this?)

@kaikreuzer
Copy link
Member

What happens if the printer´s ip address changes? The ThingUID does not change so the URL-property gets updated, right?

Yes, correct. If you create a new discovery result for the same UID, the properties of the existing thing will be automatically updated by the framework.

@kaikreuzer kaikreuzer assigned peuter and unassigned kaikreuzer Apr 21, 2015
@kaikreuzer
Copy link
Member

@peuter Please let me know, if you see a chance to address the problem or if there is any need to discuss on the solution!

@peuter
Copy link
Member Author

peuter commented Apr 26, 2015

I thought that I solved the problem with the last commit. Have you tested the last version with your printer?

@kaikreuzer
Copy link
Member

No, as you had a question back, I thought you were not yet done. Will test it right away!

@kaikreuzer
Copy link
Member

Unfortunately, this Canon printer does not turn up during mDNS discovery at all anymore... Will have to investigate, why this is the case...
But I have an issue with another printer (which is shared through a Mac):

21:34:57.100 [DEBUG] [.ipp.handler.IppPrinterHandler:139  ] - error updating jobs channel, reason: Provider com.sun.xml.internal.bind.v2.ContextFactory could not be instantiated: javax.xml.bind.JAXBException: "ch.ethz.vppserver.schema.ippclient" doesnt contain ObjectFactory.class or jaxb.index

Any idea what this is about?


private String getPrinterName(ServiceInfo service) {
String rp = service.getPropertyString("rp");
if (rp==null) {
Copy link
Member

Choose a reason for hiding this comment

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

Is "rp" really a good default? In my case, "rp" is filled ("printers/HP_LaserJet_M2727nf_MFP__745B02_"), but the service name ("HP LaserJet M2727nf MFP (745B02) @ Kona") would actually be a much better label text. Did you come across situations, where there is a name given, but which is not usable as a label?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess you are right. This is just a "leftover" from the early stages of developing this binding.

@buildhive
Copy link

openhab » openhab2 #49 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@peuter
Copy link
Member Author

peuter commented Apr 27, 2015

 Any idea what this is about?

No, sorry. The package "ch.ethz.vppserver.schema.ippclient" does
contain the ObjectFactory.class, but I am not familiar with the cups4j
internals.

(renamed CUPS binding)

Signed-off-by: Tobias Bräutigam <tbraeutigam@gmail.com>
@buildhive
Copy link

openhab » openhab2 #50 SUCCESS
This pull request looks good
(what's this?)

@kaikreuzer
Copy link
Member

No, sorry. The package "ch.ethz.vppserver.schema.ippclient" does
contain the ObjectFactory.class, but I am not familiar with the cups4j
internals.

I found the problem: The cups lib required javax.xml classes, but they were not importer by the bundle and thus not available on the classpath.

@kaikreuzer
Copy link
Member

Thanks for the update regarding the name, it looks good now!

@kaikreuzer
Copy link
Member

Hm, I just merged it including my changes here: 8d6318c
But somehow the reference to this PR got lost, so I close it manually.

@kaikreuzer kaikreuzer closed this Apr 27, 2015
@kaikreuzer
Copy link
Member

@peuter: May I ask you to create a short documentation for this binding until Sunday (May 24th)?
I have created a short guide on how to do it: https://github.com/eclipse/smarthome/blob/master/docs/sources/development/extensions/bindings/docs.md
As an example how the result could look like, please see https://github.com/eclipse/smarthome/blob/master/addons/binding/org.eclipse.smarthome.binding.hue/README.md.
Thanks!

@peuter
Copy link
Member Author

peuter commented May 21, 2015

@kaikreuzer: As I will be "offline" later this day until next wednesday I can only provide a basic description without configuration examples, as you can see here:
https://github.com/peuter/openhab2/tree/ipp-readme/addons/binding/org.openhab.binding.ipp
If that´s ok with you I would create a pull request.

@kaikreuzer
Copy link
Member

Sure, I will accept anything you can come up with in this remaining minutes ;-)

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.

None yet

3 participants