Aqara FP400: Support MCD and Re-profiling - #3118
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 73 files 540 suites 0s ⏱️ Results for commit b1b778a. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 30a04ff |
| end | ||
| end | ||
|
|
||
| --- Deeply compare two values. |
There was a problem hiding this comment.
since this is now being used in 2 subdrivers, I just moved this to the main utils file
30a04ff to
0b2b178
Compare
|
Profile category check: ✅ Passed - all profiles have a category defined. |
|
matter-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against b1b778a |
52a629d to
b1b778a
Compare
|
@hdlee27 Could you share your test result? |
| "Test profile change on endpoints increment in infoChanged for FP400", | ||
| function() | ||
| local current_profile_id = mock_device.profile.id | ||
| local incremented_matter_endpoints = matter_endpoints |
There was a problem hiding this comment.
| local incremented_matter_endpoints = matter_endpoints | |
| local incremented_matter_endpoints = utils.deep_copy(matter_endpoints) |
This is modifying the table that is used in the mock_device table itself. It is also modifying it for subsequent tests.
There was a problem hiding this comment.
haha, I actually did that semi on purpose for the subsequent tests. You're right that that's pretty contrived though
|
|
||
| function UpdateMetadataRequest.init() | ||
| local self = setmetatable({}, UpdateMetadataRequest) | ||
| self.profile = nil |
There was a problem hiding this comment.
If there is some device-specific information, should we pass in the device and save it on the device table? It feels like this could be a potential source of confusion or bugs to share device-specific state in one of these shared modules. IMO, device-specific data should be stored in the device table, even if temporary.
I would not expect us to run into issues in the real world because this should all happen within the same driver on a single thread so theoretically we shouldn't seen instances where multiple devices get mixed up in this during the length of a profile configuration step, but it seems like a precedent we wouldn't want to set. What do you think?
There was a problem hiding this comment.
Hmm, what are you imagining would happen? Since this table is locally instantiated within a function call, all its values would be local to the function, I think.
There was a problem hiding this comment.
Yeah, it shouldn't be an issue, I was more just calling out that it seemed strange to me storing device-specific variables on a type that is used globally. For example, if for some reason one of the functions on this type is called by another device before init has been called to clear the previous data, then the call would return stale data not relevant to the new device. It won't be an issue as long as init is called each time this type needs to be used for a new device, which is what is being done currently. So there are set of implicit requirements when using this type:
initmust be called before using it- the lifetime of the data can only be guaranteed in a single function call
An alternative to avoid these implicit requirements is just to pass in a table as a parameter. I personally think that is cleaner, but also at that point I can see that it hardly requires this abstraction since that is essentially what is being done in the match_profile function already. I don't feel too strongly one way or the other, so I think this is good, but I'd just add a comment somewhere indicating that init must be called to clear out any stale data before use.
| end | ||
|
|
||
| device:try_update_metadata({profile = profile_name, optional_component_capabilities = optional_supported_component_capabilities}) | ||
| device:try_update_metadata(updated_metadata:add_profile("aqs-modular" .. preference_tags):format()) |
There was a problem hiding this comment.
What if we just had a function that was something like "build_update_metadata_request" that took the profile as a parameter and then just returned the formatted metadata request instead of needing to daisy chain these methods together? It would remove the need to call format() on these requests, and would simplify it to a single function.
There was a problem hiding this comment.
I don't love the idea of adding a profile parameter to this call, since it kinda takes away the concept that the internals of the type will handle everything for you, given the appropriate data is stored in it. I also don't think this:
updated_metadata:build_update_metadata_request("aqs-modular" .. preference_tags)
is significantly more clear than this:
updated_metadata:add_profile("aqs-modular" .. preference_tags):format()
Also, the current solution can do something like this:
updated_metadata:add_profile("aqs-modular" .. preference_tags)
... later, somewhere down the line ...
updated_metadata:format()
I do kinda like the wording format_request() over the generic format(), but idk about adding a parameter. Like obviously now the system only has 2 pieces, but this makes it a bit more extensible, and there's no real "reason" why the profile name should be a parameter but the optional table shouldn't be, except that it's less complex. So to me, this is the more generic way of dealing with a new type/structure like this. What do you think?
There was a problem hiding this comment.
Yeah I think format_request might be a little more self-documenting, but otherwise I think with how this helper type is set up now I agree that passing in the profile as a parameter is inconsistent since the table itself is not passed in and is state that is saved on the type. I left some comments related to this on the discussion above.
| -- Copyright 2026 SmartThings, Inc. | ||
| -- Licensed under the Apache License, Version 2.0 | ||
|
|
||
| local UpdateMetadataRequest = {} |
There was a problem hiding this comment.
Would we plan to gradually update the rest of the drivers to utilize a new module like this? If so, this seems generic enough that we could even consider adding these to the lua libs if we want to proceed with this
There was a problem hiding this comment.
As needed, I'd think so, why not! And I generally agree, I can make a ticket for that.
4 participants
Description of Change
Support MCD profile and endpoint-comparison re-profiling for the FP400.
The endpoint data model re-interview logic will be included in 0.62+. For hubs below this version, the infoChanged trigger will never occur, unless perhaps a software version update occurs.
Summary of Completed Tests
On-device testing completed.
Adding/removing endpoints via the Aqara app updates the FP400's endpoint model, which in turn updates the device profile. All endpoint attribute updates are properly mapped to the appropriate component.