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

[dsmr] Fix delivery demand labels #16739

Merged
merged 5 commits into from
May 19, 2024
Merged

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented May 10, 2024

Regression of: #15038 (label names and update instructions)
Fixes: #16731

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel added bug An unexpected problem or unintended behavior of an add-on regression Regression that happened during the development of a release. Not shown on final release notes. labels May 10, 2024
@lsiepel lsiepel requested a review from Hilbrand as a code owner May 10, 2024 16:04
@lsiepel lsiepel requested a review from coop-git May 10, 2024 16:04
@lolodomo
Copy link
Contributor

Let's wait for @coop-git feedback.

@jlaur
Copy link
Contributor

jlaur commented May 11, 2024

@lsiepel - no update instructions needed?

@lsiepel
Copy link
Contributor Author

lsiepel commented May 11, 2024

@lsiepel - no update instructions needed?

I won't expect anyone to notice as it is just a label for a new channel in one milestone. But maybe better to do it anyway. I can come up with a commit later today.

@jlaur
Copy link
Contributor

jlaur commented May 11, 2024

@lsiepel - no update instructions needed?

I won't expect anyone to notice as it is just a label for a new channel in one milestone. But maybe better to do it anyway. I can come up with a commit later today.

In that case probably not worth it, since just a regression from a recently merged PR only available in snapshot builds. I guess the real question then is: No update instructions needed for #15038?

Copy link
Contributor

@coop-git coop-git left a comment

Choose a reason for hiding this comment

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

LGTM to show the correct wordings.
I will verify the whole setup later.

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

lsiepel commented May 11, 2024

@lsiepel - no update instructions needed?

I won't expect anyone to notice as it is just a label for a new channel in one milestone. But maybe better to do it anyway. I can come up with a commit later today.

In that case probably not worth it, since just a regression from a recently merged PR only available in snapshot builds. I guess the real question then is: No update instructions needed for #15038?

Added them as they are needed for sure. Only users who downloaded a snapshot and re-created the thing will get a log entry, but that is acceptable.

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.

It seems there is some mixup in the update instructions?

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Comment on lines 20 to 23
<properties>
<property name="thingTypeVersion">1</property>
</properties>

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you added this property to the wrong thing type. It should be added to thing id electricity_emucs_v1_0 in file meter_electricity_emucs_v1_0.xml.

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

<add-channel id="emeter_actual_demand">
<type>dsmr:actualDeliveryBelgiumType</type>
</add-channel>
<add-channel id="emeter_maximum_demand_current_month">
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to provide the label here as well, since the channel overrides it from its channel type.

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 see, wasn't aware, thanks.

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

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM

@lolodomo
Copy link
Contributor

@jlaur 's comments were all handled. Lets merge this PR now to have it in milestone 3.

@lolodomo lolodomo merged commit 20ed0e1 into openhab:main May 19, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.2 milestone May 19, 2024
@lsiepel lsiepel deleted the dsmr-fix-label branch May 19, 2024 10:37
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
* Fix delivery demand labels
* Add upgrade instructions

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
* Fix delivery demand labels
* Add upgrade instructions

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
bug An unexpected problem or unintended behavior of an add-on regression Regression that happened during the development of a release. Not shown on final release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dsmr] Just tested it in 4.2.0-SNAPSHOT and the "Actual Power Delivery" channel is shown twice in the UI.
4 participants