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

[rfxcom] Worked on repairing support for RFXThermostat3 messages #5765

Merged
merged 5 commits into from
Jul 3, 2019
Merged

[rfxcom] Worked on repairing support for RFXThermostat3 messages #5765

merged 5 commits into from
Jul 3, 2019

Conversation

martinvw
Copy link
Member

@martinvw martinvw commented Jun 26, 2019 •
edited
Loading

Fixes #4633

Test jar:
org.openhab.binding.rfxcom-2.5.0-SNAPSHOT.jar.zip

New test jar:
org.openhab.binding.rfxcom-2.5.0-SNAPSHOT.jar.zip

To use the Themostat3 now you should be able to either use the string-item to control all the commands:

GashaardCommandString.sendCommand("RUN_DOWN")

Or use either the command or control items:

GashaardCommand.sendCommand(OFF)
GasHaardControl.sendCommand(UP)

If you proper compact ideas / examples for the README they are welcome so I can include them in the PR.

@martinvw martinvw changed the title Worked on repairing support for RFXThermostat3 messages [rfxcom] Worked on repairing support for RFXThermostat3 messages Jun 26, 2019
@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/using-rfxcom-command-with-multiple-values-but-binding-expects-switch/73573/3

@martinvw
Copy link
Member Author

@thigger it would be great if you could also do some tests with this, thanks!

@martinvw martinvw force-pushed the feature/4633-improve-thermostat3-support branch 2 times, most recently from 070ca83 to 3e2a987 Compare June 26, 2019 20:41
@martinvw martinvw added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 27, 2019
@martinvw martinvw force-pushed the feature/4633-improve-thermostat3-support branch from 3e2a987 to 86d22b5 Compare June 27, 2019 18:44
@martinvw
Copy link
Member Author

I just updated with a new version, the control item should be defined as a rollershutter it will not work otherwise.

@Bappie
Copy link

Bappie commented Jun 27, 2019

The string-item is fully functional for the Thermostat3.

@martinvw martinvw force-pushed the feature/4633-improve-thermostat3-support branch 2 times, most recently from 444022b to f65b870 Compare June 28, 2019 19:17
@martinvw
Copy link
Member Author

@Jamstah / @paulianttila / @lolodomo / @openhab/2-x-add-ons-maintainers anyone up for a review, it's not too long and has tests :-)

And it's even tested by someone in the community, thanks @Bappie, he also confirmed (via email) that UP / DOWN works on the control channel and that ON/OFF still works on the command channel.

For why its implemented like this see the linked issue

@TravisBuddy
Copy link

Travis tests were successful

Hey @martinvw,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@martinvw martinvw requested a review from a team as a code owner June 28, 2019 20:20
@martinvw martinvw force-pushed the feature/4633-improve-thermostat3-support branch 2 times, most recently from 4d7b4bc to 5ce472f Compare June 28, 2019 20:25
@martinvw martinvw mentioned this pull request Jun 28, 2019
Merged
@thigger
Copy link

thigger commented Jun 29, 2019

Is there a version with the same for Lighting5? My Thermostat3 device (it's a gas fire) should be testable shortly but right now I could only test the Lighting5 relay I mentioned in #5539

@martinvw
Copy link
Member Author

Is there a version with the same for Lighting5? My Thermostat3 device (it's a gas fire) should be testable shortly but right now I could only test the Lighting5 relay I mentioned in #5539

When this is merged I aim to work on that next 👍

@martinvw
Worked on repairing support for RFXThermostat3 messages
bda0680 
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw martinvw force-pushed the feature/4633-improve-thermostat3-support branch from 5ce472f to bda0680 Compare July 2, 2019 06:21
Hilbrand
Hilbrand requested changes Jul 2, 2019
View reviewed changes
bundles/org.openhab.binding.rfxcom/src/main/resources/ESH-INF/thing/channels.xml Outdated Show resolved Hide resolved
...src/test/java/org/openhab/binding/rfxcom/internal/messages/RFXComThermostat3MessageTest.java Outdated Show resolved Hide resolved
...src/test/java/org/openhab/binding/rfxcom/internal/messages/RFXComThermostat3MessageTest.java Outdated Show resolved Hide resolved
...com/src/main/java/org/openhab/binding/rfxcom/internal/messages/RFXComThermostat3Message.java Outdated Show resolved Hide resolved
bundles/org.openhab.binding.rfxcom/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.rfxcom/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.rfxcom/README.md

\* `control` supports:

* UP
Copy link
Member

Choose a reason for hiding this comment

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

Also supports ON and OFF.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure not as far as what I read in the source code

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 did when it was a dimmer but not now its a rollershutter

Copy link
Member

Choose a reason for hiding this comment

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

I based my comment on this implementation:
https://github.com/openhab/openhab2-addons/blob/1184fac60c8d51cc62cda7655bef622df6f2fb57/bundles/org.openhab.binding.rfxcom/src/main/java/org/openhab/binding/rfxcom/internal/messages/RFXComThermostat3Message.java#L179

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair, that piece of code is unreachable because of the framework.

martinvw and others added 3 commits July 2, 2019 14:04
@martinvw @Hilbrand
Update bundles/org.openhab.binding.rfxcom/README.md
e020013 
Co-Authored-By: Hilbrand Bouwkamp <hilbrand@h72.nl>
@martinvw @Hilbrand
Update bundles/org.openhab.binding.rfxcom/README.md
028c490 
Co-Authored-By: Hilbrand Bouwkamp <hilbrand@h72.nl>
@martinvw @Hilbrand
Update bundles/org.openhab.binding.rfxcom/src/test/java/org/openhab/b…
211c204 
…inding/rfxcom/internal/messages/RFXComThermostat3MessageTest.java

Co-Authored-By: Hilbrand Bouwkamp <hilbrand@h72.nl>
@martinvw
Formatted and sorted code of rfxcom module
1184fac 
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw
Copy link
Member Author

martinvw commented Jul 2, 2019

Solved all remarks except for:

Also supports ON and OFF.

@martinvw martinvw requested a review from Hilbrand July 2, 2019 12:25
@martinvw martinvw merged commit 4eebc7a into openhab:master Jul 3, 2019
@martinvw martinvw deleted the feature/4633-improve-thermostat3-support branch July 3, 2019 14:07
@wborn wborn added this to the 2.5 milestone Jul 30, 2019
sprehn pushed a commit to sprehn/openhab-addons that referenced this pull request Aug 21, 2019
@martinvw @sprehn
[rfxcom] Repaired support for RFXThermostat3 messages (openhab#5765)
f45c3d8 
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw martinvw mentioned this pull request Aug 28, 2019
Merged
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
@martinvw @ne0h
[rfxcom] Repaired support for RFXThermostat3 messages (openhab#5765)
5e898d3 
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
Signed-off-by: Maximilian Hess <mail@ne0h.de>
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Dec 8, 2019
@martinvw @Pshatsillo
[rfxcom] Repaired support for RFXThermostat3 messages (openhab#5765)
b4eda20 
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
tmrobert8 pushed a commit to tmrobert8/openhab-addons that referenced this pull request Jan 21, 2020
@martinvw @tmrobert8
[rfxcom] Repaired support for RFXThermostat3 messages (openhab#5765)
7cebf11 
Also-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Signed-off-by: Martin van Wingerden <martin@martinvw.nl>
Signed-off-by: Tim Roberts <timmarkroberts@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hilbrand Hilbrand Hilbrand approved these changes

@paulianttila paulianttila Awaiting requested review from paulianttila

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

[rfxcom] Issues with thermostat3 commands
7 participants
@martinvw @openhab-bot @Bappie @TravisBuddy @thigger @Hilbrand @wborn