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

Use GetConnectedDevice directly to get device proxy #723

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

agners
Copy link
Collaborator

@agners agners commented May 29, 2024

Instead of using find_or_establish_case_session use the GetConnectedDevice API directly to get a device proxy for attribute reads. This has the advantage that it runs in a single node lock. With that we no longer will try to establish a case session multiple times though.

@marcelveldt
Copy link
Contributor

marcelveldt commented May 29, 2024

In the read this is fine as then we are already (pretty much) certain there should be a handle to the device.
For commissioning and node_setup its not as there we can run into race conditions with resolving (so then we need the retries).

@marcelveldt
Copy link
Contributor

@agners what's the status of this PR, can it move forward ?

@agners
Copy link
Collaborator Author

agners commented Jun 24, 2024

@agners what's the status of this PR, can it move forward ?

The Matter SDK allows to set attemptCount with FindOrEstablishSession. Ideally I would like to add that first. then we even have the retry mechanism in all cases where a session is required.

@agners
Copy link
Collaborator Author

agners commented Jun 27, 2024

In the read this is fine as then we are already (pretty much) certain there should be a handle to the device.
For commissioning and node_setup its not as there we can run into race conditions with resolving (so then we need the retries).

Yeah I think that is a good point, in read there is really less incentives to do a retry at all. The read is currently mostly (only?) used by the attribute polling, which will poll again anyways. We also have the subscription alive for all devices. So the device will eventually get offline and then the node_setup would kick in, which still has the retry logic.

So I think this is good to go as is 👍

@agners agners marked this pull request as ready for review June 27, 2024 09:43
@agners agners added the maintenance Code (quality) improvement or small enhancement which not a new feature label Jun 27, 2024
Instead of using find_or_establish_case_session use the GetConnectedDevice
API directly to get a device proxy for attribute reads. This has the
advantage that it runs in a single node lock. With that we no longer
will try to establish a case session multiple times though.
@agners agners force-pushed the use-get-connected-device-directly branch from e28b1ae to 0430bda Compare June 27, 2024 11:28
@agners agners merged commit a9c6708 into main Jun 29, 2024
4 checks passed
@agners agners deleted the use-get-connected-device-directly branch June 29, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Code (quality) improvement or small enhancement which not a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants