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

[knx] Bugfix problem with DPT 251.600 #15723

Merged
merged 5 commits into from
Oct 10, 2023
Merged

[knx] Bugfix problem with DPT 251.600 #15723

merged 5 commits into from
Oct 10, 2023

Conversation

genesis81
Copy link
Contributor

@genesis81 genesis81 commented Oct 8, 2023

In the old implementation the datatype was implemented with byte 0-255.
The DPT 251.600 is defined with 0-100%, change this from byte to PercentType.

Signed-off-by: Marco Müller [email protected]

@J-N-K
Copy link
Member

J-N-K commented Oct 8, 2023

Technically this is correct. The question is what actual problem occurs with the current implementation, because this change is breaking and I can't see any real issue with using 0-255.

@genesis81
Copy link
Contributor Author

When i try to send a new color to the bus i have always an error out of range.

See my detailed description in this threat: #https://community.openhab.org/t/rgbw-color-out-of-range-dpt-251-600/150055

In the old implementation the datatype was implemented with byte 0-255.
The DPT 251.600 is defined with 0-100%, change this from byte to
PercentType.

Signed-off-by: Marco Mueller <[email protected]>
@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/rgbw-color-out-of-range-dpt-251-600/150055/2

@genesis81
Copy link
Contributor Author

Technically this is correct. The question is what actual problem occurs with the current implementation, because this change is breaking and I can't see any real issue with using 0-255.

I have checked the code again. I don't see that the change is breaking, actual we have a bug. When you send a value over 100 calimero reject the request. That means all value over 100 is not working and will not send to the knx bus.

@J-N-K
Copy link
Member

J-N-K commented Oct 9, 2023

Correct, I misread the change. I would suggest a minor change though, to get higher accuracy.

@genesis81
Copy link
Contributor Author

Correct, I misread the change. I would suggest a minor change though, to get higher accuracy.

I'm complete open for any suggest, please tell me what we can optimize.

@holgerfriedrich
Copy link
Member

@J-N-K @genesis81 Give me some time, just trying to get a proper test which includes the Calimero part. Until now we have only tests for ValueEncoder decode/encode. With a custom KNXNetworkLink implementation, we should see everything down to the KNX frame level including the coding done in Calimero.

…ing/knx/internal/dpt/ValueEncoder.java

Co-authored-by: J-N-K <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
@J-N-K
Copy link
Member

J-N-K commented Oct 9, 2023

@holgerfriedrich Really nice. I think this can be fixed anyway, I check in calimero code that a float in the range from 0 to 100 is expected here.

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM, @holgerfriedrich do you agree?

@J-N-K
Copy link
Member

J-N-K commented Oct 9, 2023

I think you need to apply spotless

Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of an add-on label Oct 9, 2023
@J-N-K
Copy link
Member

J-N-K commented Oct 9, 2023

Test failures are independent, I also see them in my local builds without any changes.

@genesis81
Copy link
Contributor Author

Test failures are independent, I also see them in my local builds without any changes.

Looks like changes in the core system that break the tests.

@J-N-K
Copy link
Member

J-N-K commented Oct 9, 2023

I already commented in the PR that includes the cause of these problems.

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for providing this bugfix!

Just for reference: openhab/openhab-core#3834 (core change currently breaking CI test) and #15727 (WIP new tests)

@J-N-K J-N-K added this to the 4.1 milestone Oct 10, 2023
@J-N-K J-N-K merged commit 06d8e75 into openhab:main Oct 10, 2023
1 of 3 checks passed
pat-git023 pushed a commit to pat-git023/openhab-addons that referenced this pull request Oct 13, 2023
* Fix problem with DPT 251.600.
In the old implementation the datatype was implemented with byte 0-255.
The DPT 251.600 is defined with 0-100%, change this from byte to
PercentType.

Signed-off-by: Marco Mueller <[email protected]>
querdenker2k pushed a commit to querdenker2k/openhab-addons that referenced this pull request Oct 21, 2023
* Fix problem with DPT 251.600.
In the old implementation the datatype was implemented with byte 0-255.
The DPT 251.600 is defined with 0-100%, change this from byte to
PercentType.

Signed-off-by: Marco Mueller <[email protected]>
querdenker2k pushed a commit to querdenker2k/openhab-addons that referenced this pull request Oct 29, 2023
* Fix problem with DPT 251.600.
In the old implementation the datatype was implemented with byte 0-255.
The DPT 251.600 is defined with 0-100%, change this from byte to
PercentType.

Signed-off-by: Marco Mueller <[email protected]>
Signed-off-by: querdenker2k <[email protected]>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* Fix problem with DPT 251.600.
In the old implementation the datatype was implemented with byte 0-255.
The DPT 251.600 is defined with 0-100%, change this from byte to
PercentType.

Signed-off-by: Marco Mueller <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants