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

[icalendar] Wrong Command type will be used #9820

Closed
chris922 opened this issue Jan 14, 2021 · 6 comments · Fixed by #9849
Closed

[icalendar] Wrong Command type will be used #9820

chris922 opened this issue Jan 14, 2021 · 6 comments · Fixed by #9849
Labels
bug An unexpected problem or unintended behavior of an add-on

Comments

@chris922
Copy link
Contributor

Expected Behavior

I want to setup a rule via ical that changes the mode of my Z-Wave device (Spirit Thermostatic Valve).
For this I have to send the mode 1 (= comfort mode) or 11 (= eco mode) to the device.

I would expect that setting up a calendar entry with these values 1 or 11 will change the mode of the device. But it doesn't.

Current Behavior

I setup a rule in my calendar, but the device doesn't change the mode.

I changed the log-level to TRACE and was able to see that the command will be send to the device as a QuantityType even though I just defined the item as a Number (without an extra unit of measurement).

Meaning that I can see the rule is triggered and also uses the right value. In the openHAB app I also see the mode changes, but on the device itself nothing was done.

icalendar binding log:

2021-01-14 06:39:03.630 [TRACE] [.icalendar.internal.logic.CommandTag] - Command Tag Trace: "END:OG_Bad_Heizung_Radiator_Mode:11" => Fully valid Command Tag!
2021-01-14 06:39:03.642 [DEBUG] [ar.internal.handler.ICalendarHandler] - Event: [OG Bad] Heizung aktiv, Command Tag: BEGIN:OG_Bad_Heizung_Radiator_Mode:1 => OG_Bad_Heizung_Radiator_Mode.postUpdate(QuantityType: 1)

event log:

2021-01-14 06:39:03.715 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'OG_Bad_Heizung_Radiator_Mode' received command 1
2021-01-14 06:39:03.733 [INFO ] [penhab.event.ItemStatePredictedEvent] - Item 'OG_Bad_Heizung_Radiator_Mode' predicted to become 1
2021-01-14 06:39:03.745 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'OG_Bad_Heizung_Radiator_Mode' changed from 11 to 1

z-wave log:

2021-01-14 06:39:03.733 [DEBUG] [ding.zwave.handler.ZWaveThingHandler] - NODE 63: Command received zwave:device:controller:node63:thermostat_mode --> 1 [QuantityType]
2021-01-14 06:39:03.733 [DEBUG] [ding.zwave.handler.ZWaveThingHandler] - NODE 63: Command for unknown channel zwave:device:controller:node63:thermostat_mode with QuantityType

Calendar event description:

BEGIN:OG_Bad_Heizung_Radiator_Mode:1
END:OG_Bad_Heizung_Radiator_Mode:11

As you can see always the QuantityType will be used, but I would expect a DecimalType.
When I trigger a change via app I can see a DecimalType will be send to the device. So this is the only difference I was able to find that could lead to the issue that the device itself doesn't change.

I am unsure if the z-wave binding should be capable of resolving a QuantityType to a DecimalType if required or not.
The strange thing is that I tried to reproduce this issue using JSR223 (Python) integration and trigger sendCommand with a QuantityType ... when I am doing this I could see QuantityType will be used for sendCommand, but in the z-wave logs I see a DecimalType will be used. So maybe there is already some kind of auto-conversion that will not be used within the icalendar binding.

Possible Solution

Solution 1:
Enhance the CommandTag. In case the resolved QuantityType has a unit of ONE convert it to a DecimalType.

Solution 2:
Before sending the event to the item in ICalendarHandler check if the Number item has a unit-of-measurement. If not convert the QuantityType to a DecimalType.

Solution 3:
Do the conversion somewhere else, maybe outside of this binding at all?

Solution 4:
...?

Steps to Reproduce (for Bugs)

  1. Setup a Number item
  2. Setup a calendar entry sending a number to this item
  3. Check the Command type that will be send to this item

Context

Changing my z-wave thermostatic valve mode via calendar entries.

Your Environment

  • Version used: openHAB + icalendar binding v3.0.0
  • Operating System and version: raspbian on raspberry pi4 + razberry as z-wave module
@chris922 chris922 added the bug An unexpected problem or unintended behavior of an add-on label Jan 14, 2021
@chris922
Copy link
Contributor Author

Because I prefer my solution idea 1 I gave it a try and changed the binding myself.

Up to now it looks fine. You can see the changes here -> chris922@4d9dc1e
If you like I can create a PR out of it.

I can't imagine side-effects due to changing from QuantityType to DecimalType in case there is no unit of measurement.. but I am quite new to the openhab code base, so you can probably judge better if this change is safe or not :)

@cweitkamp
Copy link
Contributor

cweitkamp commented Jan 16, 2021

Hi there, this looks similar to #9771 but the other way around. As described in #9771 (comment) I am wondering why DecimalType is missing in the list of possible commands. We have to ask @daMihe for the reason.

private static final List<Class<? extends Command>> otherCommandTypes = Arrays.asList(QuantityType.class,
OnOffType.class, OpenClosedType.class, UpDownType.class, HSBType.class, PlayPauseType.class,
RewindFastforwardType.class, StringType.class);

icalendar binding: The given command line in the calendar events will be parsed according to the above ordered list. Meaning QuantityType will be checked first. This seems to succeed but without a proper unit (One will be used instead, if you do not define something). I am wondering why DecimalType is missing in this list.

@chris922
Copy link
Contributor Author

chris922 commented Jan 16, 2021

Good point! It seems that just adding DecimalType there as the first entry should be enough.

I quickly tested this in my branch via the unit-tests and when no unit is given a DecimalType will then be used.
Haven't yet done an end-to-end test in my openHAB setup, in case @daMihe thinks this would be a good way I could also try it in my openHAB setup.

See commit chris922@8ea9102

@daMihe
Copy link
Contributor

daMihe commented Jan 17, 2021

I'm not fully sure why DecimalType is missing. AFAIR the code was partly copied (as the logic was a bit different) from openhab-core from where commands were also parsed using a non-public (in terms of stability because not defined by an API) method. But i think we can extend the logic for DecimalType here if needed.

@chris922
Copy link
Contributor Author

Ok cool, I just created the PR #9849 to add DecimalType.

cweitkamp pushed a commit that referenced this issue Jan 17, 2021
@openhab-bot
Copy link
Collaborator

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh3-icalendar-migration/116731/2

themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this issue May 10, 2021
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this issue Nov 7, 2021
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this issue May 5, 2022
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 a pull request may close this issue.

4 participants