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

[plclogo] PLCLogo #2286

Merged
merged 14 commits into from Dec 3, 2018

Conversation

@falkena
Copy link
Contributor

falkena commented May 20, 2017

This is a re-implementation of openHAB1 - PLCLogo binding: https://github.com/openhab/openhab1-addons/tree/master/bundles/binding/org.openhab.binding.plclogo

Additionally to previous implementations the possibility to read LOGO8 - RTC was added. Restriction to send the whole byte on single bit change only was removed.

Signed-off-by: Alexander Falkenstern alexander.falkenstern@gmail.com

@falkena falkena changed the title [WIP] Re-implementation PLCLogo binding for openHAB2. Re-implementation PLCLogo binding for openHAB2. May 20, 2017

@falkena falkena force-pushed the falkena:plclogo branch from 4cb2131 to 6a829a3 May 25, 2017

@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented May 25, 2017

Hmmm. Travis got "Bad Gateway" error...

[WARNING] Failed to access p2 repository https://dl.bintray.com/openhab/p2/openhab-deps-repo/1.0.11, use local cache. HTTP Server 'Bad Gateway' : https://dl.bintray.com/openhab/p2/openhab-deps-repo/1.0.11/artifacts.xml
[ERROR] An error occurred while transferring artifact canonical: osgi.bundle,org.apache.commons.httpclient,3.1.0.v201012070820 from repository https://dl.bintray.com/openhab/p2/openhab-deps-repo/1.0.11:
[ERROR]    HTTP Server 'Bad Gateway' : https://dl.bintray.com/openhab/p2/openhab-deps-repo/1.0.11/plugins/org.apache.commons.httpclient_3.1.0.v201012070820.jar
@martinvw

This comment has been minimized.

Copy link
Member

martinvw commented May 25, 2017

Retrigger build

@martinvw martinvw closed this May 25, 2017

@martinvw martinvw reopened this May 25, 2017

@falkena falkena force-pushed the falkena:plclogo branch 2 times, most recently from e0433f1 to 7233137 May 28, 2017

@falkena falkena force-pushed the falkena:plclogo branch 2 times, most recently from 0043171 to 3bd8a31 Jun 29, 2017

@falkena falkena force-pushed the falkena:plclogo branch from 828ac06 to d77c4bb Jul 14, 2017

@martinvw
Copy link
Member

martinvw left a comment

Thanks for your contribution, I added some review comments.

<?xml version="1.0" encoding="UTF-8"?>
<!--
Copyright (c) 2014-2017 by the respective copyright holders.

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

It seems that this file has a non standard copyright header, can you fix all of them by running

mvn license:format

Please commit only the ones for your binding.

Thing analog <ThingId> "Label" @ "Location" [ block="<name>", threshold=<number>, force=<true/false>, type="<number/date/time>" ]
```

| Parameter | Type | Required | Default | Description |

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

Hooray properly formatted tables 😄


**RTC value differs from the value shown in LOGO! (0BA7)**

This is no bug! Since there is no way to read the RTC from a 0BA7, the binding simply returns the local time of openHAB host.

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

Great extensive readme, thanks!

This comment has been minimized.

@falkena

falkena Jul 25, 2017

Author Contributor

You are welcome :-)

@@ -0,0 +1,40 @@
/*=============================================================================|

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

Could you include the moka library as a jar, please also document the patch properly,

<version>2.2.0-SNAPSHOT</version>
</parent>

<properties>

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

These are not needed

return count;
}

public static Calendar getRtcAt(byte[] buffer, int pos) {

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

What does this have to do with the surrounding enum?

* Constructor.
*/
public PLCLogoHandlerFactory() {
SUPPORTED_THING_TYPES_UIDS.add(THING_TYPE_DEVICE);

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

It feels fragile to change the value of static final from the constructor

This comment has been minimized.

@Hilbrand

Hilbrand Sep 7, 2017

Contributor

It can be by initializing the field with: ImmutableSet.of(THING_TYPE_DEVICE);

This comment has been minimized.

@martinvw

martinvw Jun 20, 2018

Member

I still do not like this:

It feels fragile to change the value of static final from the constructor

@@ -88,4 +89,4 @@
<module>org.openhab.binding.zway</module>
</modules>

</project>
</project>

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

Please leave this new line where it was :-)

@@ -249,6 +249,12 @@
<bundle start-level="80">mvn:org.openhab.binding/org.openhab.binding.pioneeravr/${project.version}</bundle>
</feature>

<feature name="openhab-binding-plclogo" description="PLCLogo Binding" version="${project.version}">
<feature>openhab-runtime-base</feature>
<feature>openhab-transport-upnp</feature>

This comment has been minimized.

@martinvw

martinvw Jul 23, 2017

Member

Do you really use upnp?

@falkena falkena force-pushed the falkena:plclogo branch from d77c4bb to 7f94a73 Jul 25, 2017

@martinvw

This comment has been minimized.

Copy link
Member

martinvw commented Jul 26, 2017

How is this related to the #1606 they both depend on moka7

@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented Jul 26, 2017

Hi @martinvw,
please see my comment for #1606

@falkena falkena force-pushed the falkena:plclogo branch from 7e4ce75 to cbd4da4 Jul 31, 2017

@falkena falkena force-pushed the falkena:plclogo branch 2 times, most recently from e8db8fe to b7dc658 Aug 8, 2017

@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented Aug 9, 2017

@martinvw Hi. Thanks for review: Especially refactoring of huge method caused a lot of changes:-) I'll take a look to discovery class next days and try to make a jar for Moka7 library. Additionally i have some questions to your comments, especially properly using of builders. What means "properly"? Would be great to get some hints, so that i can proccess. One question more: Is it possible to review modified code again, please?

Thanks and kind regards,

Alexander.

@falkena falkena force-pushed the falkena:plclogo branch 4 times, most recently from f20d3d6 to 6fd0586 Aug 14, 2017

@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented Aug 18, 2017

@martinvw Hi Martin,
From my point of view, i'm done with changes :-)

@martinvw

This comment has been minimized.

Copy link
Member

martinvw commented Aug 19, 2017

@falkena is this already some combination between the two bindings? How did the collaboration go?

@falkena falkena force-pushed the falkena:plclogo branch from 69620f0 to 4b03d82 Sep 3, 2017

@falkena falkena force-pushed the falkena:plclogo branch from 4b03d82 to db23fa4 Oct 1, 2017

@falkena falkena force-pushed the falkena:plclogo branch from db23fa4 to 5a8d0db Oct 14, 2017

@falkena falkena force-pushed the falkena:plclogo branch from 6136e9d to b27bbbb Nov 25, 2018

@kaikreuzer
Copy link
Member

kaikreuzer left a comment

Sorry for not responding earlier!
I'd love to get that into the 2.4 release as well and I think it is already in a pretty good shape! Please find some small comments below.

</parameter>
<parameter name="threshold" type="integer" min="0">
<label>Smallest value change to send</label>
<description>Smallest change of value will be send to openHAB</description>

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

typo: send -> sent


<config-description uri="thing-type:plclogo:analog">
<parameter name="kind" type="text" pattern="AI|AM|AQ|NAI|NAQ">
<label>LOGO! analog block kind</label>

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

We usually do labels in upper case, i.e. "LOGO Analog Block Kind" here. Could you go through all your labels with that in mind?

<!--Siemens LOGO! PLC -->
<bridge-type id="device">
<label>LOGO! PLC</label>
<description>Bridge for Siemens LOGO! PLC's</description>

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

Describe the device itself, e.g. "Siemens LOGO! PLC"

<!--Siemens LOGO! channels -->
<channel-type id="diagnostic">
<item-type>String</item-type>
<label>Siemens LOGO! diagnostic</label>

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

Keep channel labels as short as possible. "Diagnostic" should do here.


<channel-type id="rtc">
<item-type>DateTime</item-type>
<label>Siemens LOGO! RTC</label>

This comment has been minimized.

@kaikreuzer
.,\
OSGI-INF/,\
ESH-INF/,\
lib/,\

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

Are you sure that you also want to package the bit_io.patch file in the binary?

This comment has been minimized.

@falkena

falkena Dec 2, 2018

Author Contributor

Removed.

if (result == 0) {
updateChannel(channel, S7.GetShortAt(buffer, address - base));
} else {
logger.warn("Can not read data from LOGO!: {}.", S7Client.ErrorText(result));

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

You do a lot of WARN logging throughout the code here. Note that normally you should only use DEBUG level logging. A WARN means that you have encountered a software bug that the user should report as an issue back to you. All "expected" situations, such as network issues that might happen at the user side should only result in the Thing to go OFFLINE with an according message.

public void run() {
try {
if (Ping.checkVitality(host, LOGO_PORT, CONNECTION_TIMEOUT)) {
logger.info("LOGO! device found at: {}.", host);

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

reduce to debug

stopScan();

try {
Enumeration<NetworkInterface> devices = NetworkInterface.getNetworkInterfaces();

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

Instead of doing this on your own, you should use the NetUtil.getAllInterfaceAddresses() method

This comment has been minimized.

@falkena

falkena Dec 2, 2018

Author Contributor

I gave getAllInterfaceAddresses() a shot, sadly without any success: IP's of LOGO! devices are not returned back. Therefore leaving my implementation.

This comment has been minimized.

@kaikreuzer

kaikreuzer Dec 3, 2018

Member

Please analyse why this is so. If you look at the code, you will see that it is doing more or less the exact same thing as you, but it also filters interfaces which are not up (which is imho a bug in your code).

This comment has been minimized.

@falkena

falkena Dec 3, 2018

Author Contributor

Ok. The main difference is, i fetch all adresses contained by a given IP network and not by interface: addresses.addAll(Arrays.asList(utilities.getInfo().getAllAddresses())); Lack of "isUp" can produce more IP's to scan. Added it to implementation. Thanks for the hint.

private static final int CONNECTION_TIMEOUT = 500;
private Set<String> addresses = new TreeSet<>();

private @Nullable ExecutorService executor;

This comment has been minimized.

@kaikreuzer

kaikreuzer Nov 30, 2018

Member

Does it make sense to keep the thread pool? It might be better, to shut it down after use and create a new one upon the next scan (as they are manually triggered, they do not happen too often).

This comment has been minimized.

@falkena

falkena Dec 2, 2018

Author Contributor

Fixed.

@falkena falkena force-pushed the falkena:plclogo branch from b27bbbb to ec6a583 Dec 2, 2018

@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented Dec 2, 2018

Hi @kaikreuzer. I've performed requested changes. Only the NetUtil.getAllInterfaceAddresses() won't work. See comment above. Would you check again, if i forgot something? Thank you

Kind regards,

Alexander

falkena and others added some commits May 20, 2017

Re-implementation PLCLogo binding for openHAB2.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Documentation update merged from pull-request by BlackAlpha700.
Signed-off-by: Carsten <carsten@hoewische.net>
Fix example section. Update troubleshooting section.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Fix initialization issues. Avoid update of thing, if it's not initial…
…ized.

Extend LOGO! client logging. Remove not need toString conversion.

Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Minor changes of german localization.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Remove 'empty' {@inheritdoc}, initialization with default null, not n…
…eed <properties> and default constructors.

Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Refactor to long method. Replace design pattern.
Change log level from error to warn. Minor.

Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Replace Moka7 source with jar.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Fixed filter regexps for blocks (#7)
Signed-off-by: Jacek Tomasiak <jacek.tomasiak@gmail.com>
Remove one-shot calculation of block address/bit values.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Use builders properly...
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
openHAB starts too slow, if many things are available.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>

@falkena falkena force-pushed the falkena:plclogo branch from ec6a583 to 179afb0 Dec 3, 2018

falkena added some commits Jan 5, 2018

Add virtual pulse thing.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Add `diagnostic` and `weekday` channels. Changes due to code review.
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>

@falkena falkena force-pushed the falkena:plclogo branch from 179afb0 to fc99b54 Dec 3, 2018

@kaikreuzer
Copy link
Member

kaikreuzer left a comment

Excellent! Many thanks for your quick updates and sorry again that the review took so long!

@kaikreuzer kaikreuzer merged commit 778fc69 into openhab:master Dec 3, 2018

2 of 3 checks passed

code-review/pullapprove Approval required by 1 of: cweitkamp, kaikreuzer, martinvw, wborn
Details
PR-openHAB2-Addons Build #11424 ended
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented Dec 3, 2018

Thank you and no problem: All good things needs their time :-)

@martinvw martinvw added this to the 2.4 milestone Dec 9, 2018

@martinvw martinvw changed the title Re-implementation PLCLogo binding for openHAB2. [plclogo] PLCLogo Dec 11, 2018

@vgrebenschikov

This comment has been minimized.

Copy link

vgrebenschikov commented Dec 24, 2018

Do you support different read-write addresses for one Item:

@vgrebenschikov

This comment has been minimized.

Copy link

vgrebenschikov commented Dec 24, 2018

Do you support oh2 discovery for available I/Q/AI/AQ ports?

@falkena

This comment has been minimized.

Copy link
Contributor Author

falkena commented Dec 30, 2018

Hi @vgrebenschikov,
glad to hear from you again :-) Sorry about late reply, i was quite busy last week. About your questions:
The answers are no and no. About different locations: I think, new thing with 2 channels is need. One channel must be read-only and one must deactivate automatic updates. "Trigger Channels" might be an option. About discovery: Well, there is nothing to discover anymore, since layout is more or less well defined. Nevertheless i think it's a good idea to populate incomig box with things digita/analog things after the Logo! type (0BA7/0BA8) is known.

Kind regards,

Alexander.

PS: Can we discuss here further, please?

chaton78 added a commit to chaton78/openhab2-addons that referenced this pull request Jan 1, 2019

Re-implementation PLCLogo binding for openHAB2. (openhab#2286)
Signed-off-by: Alexander Falkenstern <alexander.falkenstern@gmail.com>
Signed-off-by: Pascal Larin <plarin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.