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

[enphase] Initial contribution #9883

Merged
merged 5 commits into from
Apr 11, 2021
Merged

[enphase] Initial contribution #9883

merged 5 commits into from
Apr 11, 2021

Conversation

Hilbrand
Copy link
Member

@Hilbrand Hilbrand commented Jan 20, 2021

Temporary install until merged:

Open console:

openhab-cli console

And install binding:

bundle:install https://openhab.jfrog.io/artifactory/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.enphase/3.1.0-SNAPSHOT/org.openhab.binding.enphase-3.1.0-SNAPSHOT.jar
bundle:start org.openhab.binding.enphase

@Hilbrand Hilbrand added the new binding If someone has started to work on a binding. For a new binding PR. label Jan 20, 2021
@Hilbrand Hilbrand requested a review from a team January 20, 2021 13:15
@Hilbrand Hilbrand added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 20, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/new-binding-enphase-envoy-solar-system-gateway/44449/41

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Copy link
Contributor

@Skinah Skinah left a comment

Choose a reason for hiding this comment

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

I hope some of these are useful feedback, if not just ignore as some may reflect my lack of java and coding knowledge.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
@Hilbrand Hilbrand requested a review from Skinah January 23, 2021 23:00
@Hilbrand Hilbrand added the oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 label Jan 24, 2021
Copy link
Contributor

@Skinah Skinah left a comment

Choose a reason for hiding this comment

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

LGTM

updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "Serial Number is not valid");
}
if (configuration.hostname.isEmpty()) {
updateStatus(ThingStatus.ONLINE, ThingStatusDetail.CONFIGURATION_PENDING,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it really be considered online at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes good question. I didn't know how to handle it otherwise. Looking at the status table: https://www.openhab.org/docs/concepts/things.html#status-details This comes closest to what would give some meaningful status in this case. Other status are not expected to have details and thus are not expected to have additional detail text. If you have another idea let me know.

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've changed this so set to UNKNOWN at this point, and to set to OFF_LINE if it doesn't have an ip address.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
@Hilbrand Hilbrand added the rebuild Triggers Jenkins PR build label Jan 27, 2021
@wborn wborn removed the rebuild Triggers Jenkins PR build label Jan 27, 2021
@Hilbrand Hilbrand requested a review from cpmeister January 27, 2021 18:47
@lsiepel
Copy link
Contributor

lsiepel commented Feb 7, 2021

Today i change some wiring and forgot to plug the ethernet cable in. :-| The Envoy remained in Status Online / config_pending / waiting to retrieve ip address of the envoy gateway. This was there for hours. I'm not sure how to fix, but i would have expected an offline state after a while, Device unreachable or something like that.

@Hilbrand
Copy link
Member Author

Hilbrand commented Feb 8, 2021

@lsiepel I did some checks. At first sight it looks like the underlying mdns discovery in openhab-core doesn't recover correctly from a situation where network is reconnected. This means it won't work in the enphase binding here as the setting of the ip address depends on the discovery. A restart of openHAB with network connected should fix the issue or edit the thing, click advanced and add the ip address manually.

@Hilbrand
Copy link
Member Author

Hilbrand commented Feb 8, 2021

@lsiepel I did some tests. It looks like the underlying mdns of openhab-core isn't able to correctly recover from network changes. A restart of openHAB seems to be required. In this case you can also manually configure the ip address by clicking show advanced on the thing configuration in the ui.

@lsiepel
Copy link
Contributor

lsiepel commented Feb 8, 2021

I fixed it by attaching the cable ;-) and disabling/enabling the thing, so no worries. The point i tried to make was that the error was not related to the configuration and waiting any longer would not help. Not a big issue'but i would expected a status offline.

} catch (final EnvoyNoHostnameException e) {
// ignore hostname exception here. It's already handled by others.
} catch (final EnvoyConnectionException e) {
logger.trace("refreshInverters connection problem", e);
Copy link
Member

Choose a reason for hiding this comment

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

It seems unreasonable to log the stacktrace when the network fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's therefor logged to trace. To have the ability to get debug information just in case something explainable is triggered.

private final EnvoyConnector connector;
private final EnvoyHostAddressCache envoyHostnameCache;

private @NonNullByDefault({}) EnvoyConfiguration configuration;
Copy link
Member

Choose a reason for hiding this comment

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

You could assign an empty configuration object to get rid of the null disabling.

Comment on lines +191 to +193
logger.info(
"This Ephase Envoy device ({}) doesn't seem to support json data. So not all channels are set.",
getThing().getUID());
Copy link
Member

Choose a reason for hiding this comment

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

Logging to info should be used sparsely e.g. a newly started component or a user file that has been loaded. This could be debug. Please check all.

Copy link
Member Author

Choose a reason for hiding this comment

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

The logging here is done to inform the user about available of certain features of their devices. It's only done at startup of the thing. The other logging is to log a configuration setting has been automatically adjusted. Therefor I think is within the spirit of of log to info.

envoy.cond_flags.acb_ctrl.cellminvoltagewarning=Cell Min Voltage Warning
envoy.cond_flags.acb_ctrl.cibcanerror=CIB CAN Error
envoy.cond_flags.acb_ctrl.cibimageerror=CIB Image Error
envoy.cond_flags.acb_ctrl.cibspierror=CIB SPI Error"
Copy link
Member

Choose a reason for hiding this comment

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

Is the " inteded? Same for below.

Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
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

@fwolter fwolter added the cre Coordinated Review Effort label Apr 9, 2021
@fwolter fwolter merged commit 53d1689 into openhab:main Apr 11, 2021
@fwolter fwolter removed the cre Coordinated Review Effort label Apr 11, 2021
@fwolter fwolter added this to the 3.1 milestone Apr 11, 2021
@Hilbrand Hilbrand deleted the enphase branch April 11, 2021 19:17
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
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. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants