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

[iammeter] Iammeter Binding initial contribution #8252

Merged
merged 12 commits into from Sep 6, 2020

Conversation

lewei50
Copy link

@lewei50 lewei50 commented Aug 4, 2020

Initial contribution,Add support for Iammeter devices in Openhab.

Iammeter devices provides real-time readings of power meter over Wi-Fi.
The Iammeter binding is make data visualized in openhab system.

this binding had tested locally.

sorry that I closed PR 7799 accidentally and don't know how to open it again,so I opened this PR to continue my progress.
sorry to bother you with this.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

1 similar comment
@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@fwolter fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Aug 4, 2020
@fwolter
Copy link
Member

fwolter commented Aug 6, 2020

Did you finish making all changes from the old PR?

@lewei50
Copy link
Author

lewei50 commented Aug 7, 2020

Did you finish making all changes from the old PR?

yes.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

Your changes from the other PR look good! Here is my detailed feedback.

There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

Since this is a new binding, another maintainer needs to review your code before it can be merged.

In the meantime, you can fix some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

Please also update your sign-offs in the commit messages. The real name is required. See https://www.openhab.org/docs/developer/contributing.html#sign-your-work

@fwolter fwolter added the cre Coordinated Review Effort label Aug 12, 2020
@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@lewei50
Copy link
Author

lewei50 commented Aug 13, 2020

LGTM

Since this is a new binding, another maintainer needs to review your code before it can be merged.

In the meantime, you can fix some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

Please also update your sign-offs in the commit messages. The real name is required. See https://www.openhab.org/docs/developer/contributing.html#sign-your-work

Thanks for your help,I'v re-do the sign-off push.
thanks a lot!

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. The code isn't to complex, but you might need look into how to improve some modelling related to the different types of devices. See my comment. If you have any questions or need some help feel free to ask.

.project Outdated
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this file

Choose a reason for hiding this comment

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

file removed.

Copy link
Member

Choose a reason for hiding this comment

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

You've removed the wrong file 😉 This file is at the root, you removed the file in the binding. That file should not be removed.

bundles/org.openhab.binding.iammeter/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.iammeter/pom.xml Outdated Show resolved Hide resolved
bundles/org.openhab.binding.iammeter/README.md Outdated Show resolved Hide resolved
}
}

protected void thingStructureChanged(String channelPrefix) {
Copy link
Member

Choose a reason for hiding this comment

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

This is not right. You should model the different types as different things. So in the xml you would see 2 things, 1 with all channels and 1 with the limited set of channels. You can then create 2 different classes, or maybe create 2 classes that each parse the specific data. and in the handler factory you can than pass that specific class to this handler based on the specific thing created.

Choose a reason for hiding this comment

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

thanks for your advice,I have separated those into 2 thingHandler now.

updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"Communication error with the device: " + e.getMessage());
} catch (JsonSyntaxException je) {
logger.warn("Invalid JSON when refreshing source {}: {}", getThing().getUID(), je.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

In this case the status should probaly also be set to OFFLINE.

Choose a reason for hiding this comment

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

status set.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the updateStatus here. Is it done somewhere else?

@Hilbrand
Copy link
Member

Hilbrand commented Sep 4, 2020

If you are able to fix the last review comments on short notice we'll be able to add this binding to openHAB 2.5.9. Otherwise it will go to openHAB 3.0.

@yangbo1979
Copy link

If you are able to fix the last review comments on short notice we'll be able to add this binding to openHAB 2.5.9. Otherwise it will go to openHAB 3.0.

thanks for all your advices,I am working on fix them recently,and fixed them finally.
thanks again!

@TravisBuddy
Copy link

Travis tests have failed

Hey @lewei50,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

1 similar comment
@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Where almost there. Just some comments related to you additional changes.

.project Outdated
@@ -0,0 +1,17 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

You've removed the wrong file 😉 This file is at the root, you removed the file in the binding. That file should not be removed.

<channel id="importenergy" typeId="importenergy"/>
<channel id="exportgrid" typeId="exportgrid"/>
</channels>
<config-description>
Copy link
Member

Choose a reason for hiding this comment

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

Because all configuration parameters for the 2 things are the same you can also put the configuration in a config/config.xml and reference here to that config. See some other bindings, search for config-description-ref.

Copy link
Author

Choose a reason for hiding this comment

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

changed.

<label>Iammeter Power Meter 3080T</label>
<description>3 phases PowerMeter for Iammeter 3080T Binding</description>
<channel-groups>
<channel-group id="powerPhaseA" typeId="powerPhaseGroupA" />
Copy link
Member

Choose a reason for hiding this comment

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

You only need 1 group powerPhaseGroup. And for each group set label and description here:

Suggested change
<channel-group id="powerPhaseA" typeId="powerPhaseGroupA" />
<channel-group id="powerPhaseA" typeId="powerPhaseGroup" >
<label>Power Phase A</label>
<description>Power phase 1 for Iammeter device</description>
</channel-group>

Copy link
Author

Choose a reason for hiding this comment

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

changed.

String content = "";
InputStream stream = new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));

String response = HttpUtil.executeUrl(httpMethod, url, stream, null, TIMEOUT_MS);
Copy link
Member

Choose a reason for hiding this comment

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

The stream is an empty String. Do you need to send something or does it also work to use:

Suggested change
String response = HttpUtil.executeUrl(httpMethod, url, stream, null, TIMEOUT_MS);
String response = HttpUtil.executeUrl(httpMethod, url, TIMEOUT_MS);

Same applies to the other handler

Copy link
Author

Choose a reason for hiding this comment

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

both changed.

updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"Communication error with the device: " + e.getMessage());
} catch (JsonSyntaxException je) {
logger.warn("Invalid JSON when refreshing source {}: {}", getThing().getUID(), je.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the updateStatus here. Is it done somewhere else?

Comment on lines 100 to 58
JsonElement iammeterDataElement = new JsonParser().parse(response);
JsonObject iammeterData = iammeterDataElement.getAsJsonObject();
String keyWord = "Datas";
if (iammeterData.has("Datas") && iammeterData.has("SN")) {
String groups[] = { "powerPhaseA", "powerPhaseB", "powerPhaseC" };
for (int row = 0; row < groups.length; row++) {
String gpName = groups[row];
List<Channel> chnList = getThing().getChannelsOfGroup(gpName);
for (IammeterWEM3080Channel channelConfig : IammeterWEM3080Channel.values()) {
Channel chnl = chnList.get(channelConfig.ordinal());
if (chnl != null) {
State state = getDecimal(iammeterData.get(keyWord).getAsJsonArray().get(row)
.getAsJsonArray().get(channelConfig.ordinal()).toString(), channelConfig.getUnit());
updateState(chnl.getUID(), state);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Related to the class splitting. I didn't intend for you to completely copy paste the code as it duplicates a lot of code. Better make an abstract base class with all the logic and let it call methods to do the specific code. The code I selected here would be in the sub class. The method would get the response as parameter. You can make the method getDecimal protected (better rename it getQuantityState probably`.

Copy link
Author

Choose a reason for hiding this comment

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

abstract class made.

Copy link
Author

Choose a reason for hiding this comment

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

abstract class made.

ScheduledFuture<?> refreshJob = this.refreshJob;
if (refreshJob != null && !refreshJob.isCancelled()) {
refreshJob.cancel(true);
refreshJob = null;
Copy link
Member

Choose a reason for hiding this comment

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

You need to set the field to null:

Suggested change
refreshJob = null;
this.refreshJob = null;

Copy link
Author

Choose a reason for hiding this comment

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

changed.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Also-by: Yang Bo<service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

2 similar comments
@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Just some clean up comments.

Can you also run mvn spotless:apply on your binding to make sure the style complies, otherwise it might break the build (this changed recently).

*/

@NonNullByDefault
public class IammeterBaseHandler extends BaseThingHandler {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class IammeterBaseHandler extends BaseThingHandler {
public abstract class IammeterBaseHandler extends BaseThingHandler {

Copy link
Author

Choose a reason for hiding this comment

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

added.

Copy link
Author

Choose a reason for hiding this comment

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

codes reformated using mvn spotless:apply.

String url = "http://" + config.username + ":" + config.password + "@" + config.host + ":" + config.port
+ "/monitorjson";
String content = "";
InputStream stream = new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
Copy link
Member

Choose a reason for hiding this comment

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

This is not used anymore and can be removed.

Copy link
Author

Choose a reason for hiding this comment

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

removed.


resolveData(response);

// JsonElement iammeterDataElement = new JsonParser().parse(response);
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this commented out code.

Copy link
Author

Choose a reason for hiding this comment

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

removed.

}
}

protected void resolveData(String response) {
Copy link
Member

Choose a reason for hiding this comment

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

By maing this class abstract you can also make this method abstract:

Suggested change
protected void resolveData(String response) {
protected abstract void resolveData(String response);

Copy link
Author

Choose a reason for hiding this comment

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

changed.

Signed-off-by: lewei50 <service@lewei50.com>
Signed-off-by: lewei50 <service@lewei50.com>
@TravisBuddy
Copy link

Travis tests have failed

Hey @lewei50,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

1 similar comment
@TravisBuddy
Copy link

Travis tests have failed

Hey @lewei50,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests were successful

Hey @lewei50,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

LGTM

@Hilbrand Hilbrand merged commit a228349 into openhab:2.5.x Sep 6, 2020
@Hilbrand Hilbrand added this to the 2.5.9 milestone Sep 6, 2020
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020
Signed-off-by: Yang Bo <service@lewei50.com>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
Signed-off-by: Yang Bo <service@lewei50.com>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Oct 8, 2020
Signed-off-by: Yang Bo <service@lewei50.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort 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

5 participants