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

[config] Add Intermatic PE653 and PE953 pool/spa controller and remote. #2289

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

j9brown
Copy link

@j9brown j9brown commented Jul 3, 2020

I'm not sure about the VerifyChanged compatibility declarations. The ozw config file only seems to pick up instance 1 and doesn't actually set the verify changed flags.

This device requires polling and verify changes to work correctly. Calling "enablePoll" and "setChangeVerified" at runtime for each value of interest does the trick. What's the best way to encode that here?

@Fishwaldo
Copy link
Member

This device requires polling and verify changes to work correctly

It shouldn't. Both are basically doing the same thing - polling continuously polls the device for changes. VerifyChanges will poll the device for changes after we get a update - if two reports are the same it publishes the value.

I don't accept config files that enable polling as thats more of a Policy Decision than a config decision.

VerifiedChanges can be enabled as per https://github.com/OpenZWave/open-zwave/wiki/CommandClass-Compatibility-Flags#all-commandclasses

<!-- This device reports 6 switches but there are only 5 circuits.
The switch on the default endpoint is redundant with endpoint 1 so
we omit it from the list of instances. -->
<CommandClass id="32">
Copy link
Member

Choose a reason for hiding this comment

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

This should be auto discovered. Its not needed.

Copy link
Author

Choose a reason for hiding this comment

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

Per the comment, this is intentional. When auto-discovered, the device presents 6 switch instances however physically there are only 5 of them. By listing them explicitly I am able to suppress the redundant switch instance and label each of them in the same manner as they appear in the product documentation.

FWIW, when I first set up the device, I got fooled by the redundant switch instance and was really confused why the switches weren't controlling the correct loads.

<Instance index="4" label="Circuit 4" />
<Instance index="5" label="Circuit 5" />
</CommandClass>
<CommandClass id="37">
Copy link
Member

Choose a reason for hiding this comment

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

This should be auto discovered. Its not needed.

Copy link
Author

Choose a reason for hiding this comment

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

Per prior comment, I'm listing the switching explicitly so as to suppress a redundant instance that should not be enumerated.

<Instance index="4" label="Circuit 4" />
<Instance index="5" label="Circuit 5" />
<Compatibility>
<VerifyChanged index="1">true</VerifyChanged>
Copy link
Member

Choose a reason for hiding this comment

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

index refers to the ValueID Index - Not instance Index. I don't think you need a entry for each "instance"

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will remove the others.

</Compatibility>
</CommandClass>

<CommandClass id="49">
Copy link
Member

Choose a reason for hiding this comment

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

This should be auto discovered. Its not needed.

Copy link
Author

Choose a reason for hiding this comment

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

This sensor is auto-discovered but it ends up having the incorrect label and units so I'm listing it explicitly to ensure the correct information is presented to the user.

</Value>
</CommandClass>

<CommandClass id="67">
Copy link
Member

Choose a reason for hiding this comment

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

This should be auto discovered. Its not needed.

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately this is not the case. Auto discovery only detects instance index 1. Also, it ends up with the wrong label and units so I'm listing both thermostats explicitly to ensure the correct information is presented to the user. It took me some time to reverse engineer this.

<Entry author="Jeff Brown - [email protected]" date="29 June 2020" revision="1">Initial creation</Entry>
</ChangeLog>
</MetaData>
</Product>
Copy link
Member

Choose a reason for hiding this comment

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

No Config Params or Association Groups?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately the config params are very complicated and undocumented. This device is intended to be configured using the associated PE953 remote control. Having watched the traffic between the two, there's a fair bit of manufacturer specific communication going on during that process.

The device doesn't support associations.

Copy link
Author

@j9brown j9brown left a comment

Choose a reason for hiding this comment

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

Hope that answers your questions. This device is certainly a little odd. :)

</Compatibility>
</CommandClass>

<CommandClass id="49">
Copy link
Author

Choose a reason for hiding this comment

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

This sensor is auto-discovered but it ends up having the incorrect label and units so I'm listing it explicitly to ensure the correct information is presented to the user.

<Instance index="4" label="Circuit 4" />
<Instance index="5" label="Circuit 5" />
<Compatibility>
<VerifyChanged index="1">true</VerifyChanged>
Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will remove the others.

</Value>
</CommandClass>

<CommandClass id="67">
Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately this is not the case. Auto discovery only detects instance index 1. Also, it ends up with the wrong label and units so I'm listing both thermostats explicitly to ensure the correct information is presented to the user. It took me some time to reverse engineer this.

<Entry author="Jeff Brown - [email protected]" date="29 June 2020" revision="1">Initial creation</Entry>
</ChangeLog>
</MetaData>
</Product>
Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately the config params are very complicated and undocumented. This device is intended to be configured using the associated PE953 remote control. Having watched the traffic between the two, there's a fair bit of manufacturer specific communication going on during that process.

The device doesn't support associations.

@j9brown
Copy link
Author

j9brown commented Jul 10, 2020

Hmm, so I just noticed something strange after making these changes and refreshing the node info. I see 10 switches showing up now instead of 5.

I can resolve this by adding "endpoint" to the Instance declarations but it fails make xmltest.

<Instance index="1" endpoint="1" label="Circuit 1" />
<Instance index="2" endpoint="2" label="Circuit 2" />
<Instance index="3" endpoint="3" label="Circuit 3" />
<Instance index="4" endpoint="4" label="Circuit 4" />
<Instance index="5" endpoint="5" label="Circuit 5" />

I must have removed these attributes prior to upload and failed to retest my change (sorry!).

What's the right way of solving this problem while also eliminating the redundant 6th switch endpoint from the enumeration?

@j9brown
Copy link
Author

j9brown commented Jul 20, 2020

Any thoughts on this?

@Fishwaldo
Copy link
Member

You probably need to add https://github.com/OpenZWave/open-zwave/wiki/CommandClass-Compatibility-Flags#multiinstance-commandclass MapRootToEndpoint in the config file - Without all the other CommandClass Info you have right now. Otherwise, please post a log file of the interview after issuing a refreshNodeInfo command.

@j9brown
Copy link
Author

j9brown commented Jul 24, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants