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

Door Condition per 1.6 specs, adds DoorSense support #2196

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

quinnhosler
Copy link

Implements Door Condition per 1.6 specs under COMMAND_CLASS_DOOR_LOCK

Additionally includes updated xml files for Door Condition and includes August DoorSense™ support

Copy link
Member

@Fishwaldo Fishwaldo left a comment

Choose a reason for hiding this comment

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

Several changes needed.

<Item label="Unlocked/Open" value="2" />
<Item label="Unlocked/Closed" value="3" />
</Value>
</CommandClass>
Copy link
Member

Choose a reason for hiding this comment

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

This is not a Configuration CC id - Why is it required?

Copy link
Author

Choose a reason for hiding this comment

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

You're right this isn't needed and I didn't understand why I was making that change. I removed the changes to this file.

cpp/src/ValueIDIndexesDefines.def Outdated Show resolved Hide resolved
DoorLockCondition_Latched_Unlocked_Open = 0x06,
DoorLockCondition_Latched_Unlocked_Closed = 0x07,
};

Copy link
Member

Choose a reason for hiding this comment

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

Why is this required?

{
value->OnValueRefreshed(_data[3]);
value->Release();
}
Copy link
Member

Choose a reason for hiding this comment

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

Few issues here:

  1. this ValueID is not created by the Class. You need to determine if the device in question supports this ValueID (probably based upon the CommandClass Version) and then create the ValueID, and subsequently, in this specific block, only do this update if the CommandClass Version is equal/above the minimum version and you should also check if index 3 is valid in the data recieved.

Copy link
Author

Choose a reason for hiding this comment

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

I was looking through the ZW docs and it appears DoorCondition is defined as far back as version 1 of the protocol and hasn't changed sense. I'm assuming this negates the need for any version checking? I added a simple validation of index 3.

@quinnhosler
Copy link
Author

Hey @Fishwaldo, thanks for your comments. I went ahead and made changes addressing them. Let me know if there are any other changes required!

@quinnhosler
Copy link
Author

What is the current thought process with this pull request? Does it need more work?

@perosb
Copy link

perosb commented Jul 23, 2020

I assume the conflicts that now occured need to be resolved first.

I'd be very interested in DoorCondition as well to test on ID Lock.
Good stuff!

@elgeniskogen
Copy link

Would this fix the problem with ID-Lock 150 where I cannot see if door is closed or open? That would be fantastic. Now a I have to use an additional sensor as I am not able use the one in the lock. I have a lock to test on if needed. Your contributions are highly appreciated :-)

@perosb
Copy link

perosb commented Sep 15, 2020

Would this fix the problem with ID-Lock 150 where I cannot see if door is closed or open? That would be fantastic. Now a I have to use an additional sensor as I am not able use the one in the lock. I have a lock to test on if needed. Your contributions are highly appreciated :-)

That is my understanding. I also have the ID Lock 150 with an additional sensor to solve this.

@labaland
Copy link

When will this be added? =)

@drgreenbaum
Copy link

Please merge this! Would be a great addition!

@labaland
Copy link

labaland commented Dec 2, 2020

Please merge this! Would be a great addition!

AGREE!!

@drgreenbaum
Copy link

@Fishwaldo what is needed to make this happen? Do we need testing?

@johnniemalm
Copy link

I have this to,

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.

7 participants