-
Notifications
You must be signed in to change notification settings - Fork 525
Support DPS feature #2750
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
Open
HunsupJung
wants to merge
5
commits into
main
Choose a base branch
from
feature/doorlock-dps-feature
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Support DPS feature #2750
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e201ff7
Support DPS feature
HunsupJung 6d1d895
Fix typo
HunsupJung b7db8b3
Merge branch 'main' into feature/doorlock-dps-feature
HunsupJung 8843cef
Add nil checking and test unit for DPS
HunsupJung ac48fc2
Add doorState setting to info_changed
HunsupJung File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that at least some locks will set the
DOOR_POSITION_SENSORfeature flag by default even if a door sensor is not configured. They will report NULL for theDoorStateattribute until one has been configured. So I think we should checkDoorStateand theDPSflag and if they are non-NULL and the flag is set then we can saydoorStateis supported on this lock.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpmanley
I am sorry for the late reply and thank you for the review.
Do you mean to read
DoorLockcluster'sAttributeListand set the flag if there isDoorStateattribute?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem @HunsupJung . I mean reading the DoorState attribute to confirm it's a non-null value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpmanley
How about setting to
unspecifiedErrorwith logging whenDoorStateattribute is nil?I think It's not easy to change profile when device start sending the normal value for
DoorStateafter continuously sending nil value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do something similar to #2828 by setting a field like
profiling_data.DOOR_STATE_NON_NULLwhen theDoorStateattribute is reported as a non-null value. When that field changes from not being set to being set, then that will trigger the re-profile.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we wouldn't include the capability by default, Hunsup is right that we'd need to do a read, which would probably be fine. However, to avoid the problem of reads failing I think we could just add
DoorStateas a subscribed attribute indevice_initwithadd_subscribed_attribute, and then if we get a non-null value reported back, then do what Tom suggested withindoor_state_handlerwith some logic like this to re-profile the device:Then within match_profile, check for the presence of the
profiling_data.DOOR_STATE_NON_NULLas the basis for includingdoorStatein the profile.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, since it's not a mandatory attribute, maybe create a new separate field rather than putting this field within
profiling_data, otherwise locks that don't support DPS would be blocked here.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my recomendation:
Using my PR here as a blueprint,
we should update this table:
Then, in
device_init:Next, in
door_state_handler:Finally, in
do_configure, we can remove this section:and we can replace it with the following logic:
probably over by the
BATTERY_SUPPORTlogic.This ensures that whether the DOOR_POSITION_SENSOR flag is set and whether the DoorState is populated, the profiling will occur as expected. It also ensures that if the doorState capability is ever set, it will never be unset. Finally, it ensures that if the DOOR_POSITION_SENSOR is ever enabled during the device's lifetime in a FW update or similar, we will re-attempt a device profile matching (at least on driver restart).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, to ensure that any re-profiling works as expected, we should generally add gating like this for all modular capability insertions:
to ensure that if an optional capability is already supported, we continue to enable it.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hcarter-775
If it is excluded from the feature, the capability should also be subtracted.
@tpmanley , @hcarter-775 , @nickolas-deboom
In conclusion, I understand that it seems to be your opinion to continue reading
DoorStatevalue indevice_init.In the case of
DPS, the feature may be turned on and off. if it adjusts the profile at the same time infeature_maphandler (not included in this commit) anddevice_init, duplicated change may occur.