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

[homewizard] Add current, voltage and failure channels #16995

Merged
merged 18 commits into from
Oct 7, 2024

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Jul 4, 2024

  • Add active current channels (L1,L2,L3 + sum of all)
  • Add active voltage channels (L1,L2,L3 + generic)
  • Add power failure count channels (any + long)
  • Add tests and minor refactoring
  • Changed gas timestamp now respects openHAB configured timezone

Resolves: #16988

lsiepel added 2 commits July 4, 2024 22:36
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel added enhancement An enhancement or new feature for an existing add-on additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Jul 4, 2024
@lsiepel lsiepel requested a review from wborn July 4, 2024 20:56
lsiepel added 3 commits July 7, 2024 21:14
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel changed the title [homewizard] Add current and voltage channels [homewizard] Add current, voltage and failure channels Jul 8, 2024
lsiepel added 2 commits July 12, 2024 21:43
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Jul 12, 2024
@lsiepel lsiepel requested review from a team and removed request for wborn July 12, 2024 19:55
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel
Copy link
Contributor Author

lsiepel commented Aug 24, 2024

Would be nice if we can get this merged, then it would be easier to move #13495 forward.

@lsiepel lsiepel marked this pull request as draft August 28, 2024 15:45
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel
Copy link
Contributor Author

lsiepel commented Aug 28, 2024

Got a little bit messy because of the merge conflicts. But all should be ok now. Ready for a review. Was tested by community and unit tests.

@lsiepel lsiepel marked this pull request as ready for review August 28, 2024 19:09
@lsiepel lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Aug 28, 2024
Copy link
Contributor

@jlaur jlaur 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 the new channels! I have added a few comments.

try {
return ZonedDateTime.of(year, month, day, hours, minutes, seconds, 0, ZoneId.systemDefault());
} catch (DateTimeException e) {
LoggerFactory.getLogger(DataPayload.class).warn("Unable to parse Gas timestamp: {}", gasTimestamp);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should log from a DTO get method. Perhaps this could be logged in the handler instead (even if this would require a getter for the raw value to be logged)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved logging to the handler.

Comment on lines 8 to 9
thing-type.homewizard.energy_socket.label = HomeWizard Energysocket
thing-type.homewizard.energy_socket.description = This thing provides the measurement data that is available through the http interface of a HomeWizard Energysocket.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems duplicated from the next two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result of duplice thing declaration, fixed.

Comment on lines 32 to 33
thing-type.config.homewizard.energy_socket.ipAddress.label = Network Address
thing-type.config.homewizard.energy_socket.ipAddress.description = The IP or host name of the Energysocket.
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result of duplice thing declaration, fixed.

Comment on lines 38 to 39
thing-type.config.homewizard.energy_socket.refreshDelay.label = Refresh Interval
thing-type.config.homewizard.energy_socket.refreshDelay.description = The refresh interval in seconds for polling the Energysocket.
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result of duplice thing declaration, fixed.

Comment on lines 46 to 49
thing-type.config.homewizard.watermeter.ipAddress.label = Network Address
thing-type.config.homewizard.watermeter.ipAddress.description = The IP or host name of the Watermeter.
thing-type.config.homewizard.watermeter.refreshDelay.label = Refresh Interval
thing-type.config.homewizard.watermeter.refreshDelay.description = The refresh interval in seconds for polling the Watermeter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result of duplice thing declaration, fixed.

<label>Active Voltage L3</label>
</channel>

<channel id="any_power_failures" typeId="power_failures"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The word "any" gives me the impression that it's a boolean value, but actually it's the count of power failures (long or short), if I understand it correctly. Perhaps the name could be rephrased to better express this? Perhaps power_failure_count and long_power_failure_count, respectively? Or simply power_failures and long_power_failures (which is already consistent with channel below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks, that is much better. When building up a model from the API, one tend to re-use those conventions.

@@ -98,6 +131,68 @@

</thing-type>

<thing-type id="energy_socket">
Copy link
Contributor

Choose a reason for hiding this comment

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

This thing type is already defined in line 78?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merge conflicts leftover. Fixed.


</thing-type>

<thing-type id="watermeter">
Copy link
Contributor

Choose a reason for hiding this comment

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

This thing type is already defined in line 109?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merge conflicts leftover. Fixed.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel requested a review from jlaur September 28, 2024 13:25
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
*/
public HomeWizardP1MeterHandler(Thing thing) {
public HomeWizardP1MeterHandler(Thing thing, ZoneId zoneId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to store TimeZoneProvider here in order to call timeZoneProvider.getTimeZone() at the moment when creating ZonedDateTimes. This way changes in time-zone will automatically have effect immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, totally true.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Last two comments, otherwise LGTM.

bundles/org.openhab.binding.homewizard/README.md Outdated Show resolved Hide resolved
logger.warn("Unable to parse Gas timestamp: {}", e.getMessage());
gasTimestamp = null;
}
if (gasTimestamp != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, it shouldn't be cleared (UNDEF) otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to leave it as close to the original behaviour as possible.

Co-authored-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: lsiepel <leosiepel@gmail.com>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit 6b41189 into openhab:main Oct 7, 2024
5 checks passed
@jlaur jlaur added this to the 4.3 milestone Oct 7, 2024
@lsiepel lsiepel deleted the homewizard-endpoint branch October 7, 2024 15:33
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HomeWizard] request to add current and voltage channels to Wi-Fi P1 Meter thing
2 participants