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.

Sign up for GitHub

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

Jump to bottom

[ism8] Add UoM support #14206

Merged
merged 16 commits into from
Feb 25, 2024
Merged

[ism8] Add UoM support #14206

merged 16 commits into from
Feb 25, 2024

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Jan 11, 2023 •
edited
Loading

Signed-off-by: lsiepel [email protected]

  • Adds UoM to all applicable channels.
  • Adds several units tests for in- and outgoing messages.
  • Minor refactoring of Thing handler.

Due to the change in channels, this is not backwards compatible. Channel configuration and linked Items have to be adapted.

Test jar for 3.4.1 is available here: https://1drv.ms/u/s!AnMcxmvEeupwjp4L347Q22cnMI1QQg?e=ROo8Y1
Tests have been performed by @schmidmuc (thanks for that!)

Fixes: #9598

Also replaces: #14665

@lsiepel
Add UoM support
cc023db 
Signed-off-by: lsiepel <[email protected]>
@lsiepel lsiepel added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible labels Jan 11, 2023
@lsiepel
Adapt readme
71c0c58 
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Fix tests locale
d3b431d 
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Spotless
f440f49 
Signed-off-by: lsiepel <[email protected]>
@lsiepel
Fix copyright
416891d 
Signed-off-by: lsiepel <[email protected]>
@holgerfriedrich
Copy link
Member

As this is a breaking change, did you think of adding a note to update.lst in distro repo?
https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst

lsiepel added a commit to lsiepel/openhab-distro that referenced this pull request Jan 13, 2023
@lsiepel
add breaking change alert for ISM8
794fa93 
add breaking change alert for ISM8

related to: openhab/openhab-addons#14206
@lsiepel lsiepel mentioned this pull request Jan 13, 2023
Closed
@lsiepel
Copy link
Contributor Author

lsiepel commented Jan 13, 2023

As this is a breaking change, did you think of adding a note to update.lst in distro repo? https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst

Yeah, thought about it, never done that before, hope it is right.

Confectrician
Confectrician reviewed Feb 6, 2023
View reviewed changes
Copy link
Contributor

@Confectrician Confectrician left a comment

Choose a reason for hiding this comment

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

Looks good for the docs part.

@lsiepel lsiepel mentioned this pull request Mar 7, 2023
Merged
@lsiepel
Copy link
Contributor Author

lsiepel commented Mar 13, 2023

Wanted to add the instructions.xml update instructions, but i fail to find any documentation how to do this. @jlaur can you share some links/hints ?

@jlaur
Copy link
Contributor

jlaur commented Mar 14, 2023

Wanted to add the instructions.xml update instructions, but i fail to find any documentation how to do this. @jlaur can you share some links/hints ?

There is no documentation: openhab/openhab-core#3330 (comment)

A few sources for information:

@lsiepel lsiepel mentioned this pull request Mar 14, 2023
Closed
@lsiepel lsiepel mentioned this pull request Mar 25, 2023
Closed
@jlaur
Copy link
Contributor

jlaur commented May 27, 2023

Wanted to add the instructions.xml update instructions, but i fail to find any documentation how to do this. @jlaur can you share some links/hints ?

There is no documentation: openhab/openhab-core#3330 (comment)

Now there is: https://next.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

@lsiepel
Copy link
Contributor Author

lsiepel commented Aug 1, 2023 •
edited
Loading

a

Oke, i have read some more and used it elsewhere. So i can understand the basics, but now this binding is different, it adds the channels during setup dynamically.

I have this problem with the following scenario:
For example old channel-type number-readonly is split into multiple channel-types percentage-r, temperature-r and flowrate-r. Maybe i can make some rules to determine those rules, but i don't think the instructions.xml features some conditional logic, does it?

If not, this PR is ready to review/merge and the breaking change PR will still be needed.

Edit: @jlaur would be nice if you can advice on how to proceed with the upgrade instructions. As there is not a 1:1 relation between old and new channels-types.

@lsiepel lsiepel requested a review from jlaur September 5, 2023 20:41
@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 13, 2023

@openhab/add-ons-maintainers This PR has been tested and confirmed to be working as expected. Can we get this merged?

@lsiepel
Copy link
Contributor Author

lsiepel commented Nov 9, 2023

Seen some other PR's and for those cases when channels where automatically added the thing upgrade was not needed. So i think this is all set now.

jlaur
jlaur requested changes Nov 9, 2023
View reviewed changes
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.

Not a review, sorry, but I noticed two old comments pending, so thought I'd "flush" them. 🙂

bundles/org.openhab.binding.ism8/README.md Outdated Show resolved Hide resolved
...openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/util/Ism8DomainMap.java Outdated Show resolved Hide resolved
@lsiepel
Fix review comments
676635d 
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel
Fix doc mistake
acbe29f 
Signed-off-by: Leo Siepel <[email protected]>
@lsiepel lsiepel requested a review from jlaur December 14, 2023 22:32
@lsiepel lsiepel requested a review from a team December 29, 2023 11:51
@lsiepel
Copy link
Contributor Author

lsiepel commented Feb 21, 2024

@openhab/add-ons-maintainers It would be nice to have this available in 4.2 M1 Anyone able to review ?

kaikreuzer
kaikreuzer requested changes Feb 24, 2024
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thank you - just some minor comments.

...enhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8BindingConstants.java Outdated
Comment on lines 47 to 50
/**
* Channel id configuration parameter
*
*/
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
/**
* Channel id configuration parameter
*
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated

@Override
public void handleCommand(ChannelUID channelUID, Command command) {
this.logger.debug("Ism8: Handle command = {} {}", channelUID.getId(), command);
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
this.logger.debug("Ism8: Handle command = {} {}", channelUID.getId(), command);
logger.debug("Ism8: Handle command = {} {}", channelUID.getId(), command);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
return true;
}
} catch (NumberFormatException e) {
this.logger.warn(
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
this.logger.warn(
logger.warn(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
description, config);
}
} else {
this.logger.debug("Ism8: ID or type missing - Channel={} Cfg={}", description, config);
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
this.logger.debug("Ism8: ID or type missing - Channel={} Cfg={}", description, config);
logger.debug("Ism8: ID or type missing - Channel={} Cfg={}", description, config);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
try {
svr.sendData(Ism8DomainMap.toISM8WriteData(dataPoint, command));
} catch (IOException e) {
this.logger.debug("Writting to Ism8 DataPoint '{}' failed. '{}'", dataPoint.getId(), e.getMessage());
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
this.logger.debug("Writting to Ism8 DataPoint '{}' failed. '{}'", dataPoint.getId(), e.getMessage());
logger.debug("Writing to Ism8 DataPoint '{}' failed. '{}'", dataPoint.getId(), e.getMessage());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
e.getMessage());
}
} else {
this.logger.debug("Ism8: ID or type missing - Channel={} Cfg={}", channel.getLabel(),
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
this.logger.debug("Ism8: ID or type missing - Channel={} Cfg={}", channel.getLabel(),
logger.debug("Ism8: ID or type missing - Channel={} Cfg={}", channel.getLabel(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
int id = Integer.parseInt(channel.getConfiguration().get(CHANNEL_CONFIG_ID).toString());
if (id == dataPoint.getId()) {
if (dataPoint.getValueObject() != null) {
this.logger.debug("Ism8: updating channel {} with datapoint: {}", channel.getUID().getAsString(),
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
this.logger.debug("Ism8: updating channel {} with datapoint: {}", channel.getUID().getAsString(),
logger.debug("Ism8: updating channel {} with datapoint: {}", channel.getUID().getAsString(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
return true;
}
} else {
this.logger.debug("Ism8 channel: {} and DataPoint do not have a matching Id: {} vs {}",
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
this.logger.debug("Ism8 channel: {} and DataPoint do not have a matching Id: {} vs {}",
logger.debug("Ism8 channel: {} and DataPoint do not have a matching Id: {} vs {}",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
channel.getUID(), id, dataPoint.getId());
}
} catch (NumberFormatException e) {
this.logger.warn(
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
this.logger.warn(
logger.warn(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

...es/org.openhab.binding.ism8/src/main/java/org/openhab/binding/ism8/internal/Ism8Handler.java Outdated
}
}
}
this.logger.debug("Ism8: no channel was found for DataPoint id: {}", dataPoint.getId());
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
this.logger.debug("Ism8: no channel was found for DataPoint id: {}", dataPoint.getId());
logger.debug("Ism8: no channel was found for DataPoint id: {}", dataPoint.getId());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lsiepel
Review comments
b6a1831 
Signed-off-by: Leo Siepel <[email protected]>
kaikreuzer
kaikreuzer approved these changes Feb 25, 2024
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks!

@kaikreuzer kaikreuzer merged commit 52f4a64 into openhab:main Feb 25, 2024
3 checks passed
@kaikreuzer kaikreuzer added this to the 4.2 milestone Feb 25, 2024
@lsiepel lsiepel deleted the ism8-UoM branch February 25, 2024 12:52
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
@lsiepel @austvik
[ism8] Add UoM support (openhab#14206)
b7cc525 
* Add UoM support

Signed-off-by: lsiepel <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
@lsiepel @joni1993
[ism8] Add UoM support (openhab#14206)
178a7f0 
* Add UoM support

Signed-off-by: lsiepel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Confectrician Confectrician Confectrician left review comments

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@lolodomo lolodomo Awaiting requested review from lolodomo

@jlaur jlaur Awaiting requested review from jlaur

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

[ism8] Binding ism8 doesn't support UoM, values don't update
5 participants
@lsiepel @holgerfriedrich @jlaur @kaikreuzer @Confectrician