AttributeCache should cache data versions and use them in subsequent … #16602
Conversation
This file contains 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
pullapprove
bot
requested review from
anush-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
lazarkov,
LuDuda and
lzgrablic02
bzbarsky-apple
approved these changes
Apr 12, 2022
|
PR #16602: Size comparison from 2e33dec to e590ac5 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #16602: Size comparison from 2e33dec to dad6641 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
yunhanw-google
force-pushed
the
feature/issue_16243
branch
2 times, most recently
from
a7d9c7a to
109efea
Compare
yunhanw-google
force-pushed
the
feature/issue_16243
branch
from
109efea to
062ea42
Compare
|
PR #16602: Size comparison from 3af5410 to 062ea42 Increases (20 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
Full report (20 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
|
yunhanw-google
force-pushed
the
feature/issue_16243
branch
from
062ea42 to
bc88894
Compare
|
PR #16602: Size comparison from 3af5410 to bc88894 Increases (20 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
Full report (20 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
|
…Read/Subscribe requests Move DataVersionFilter encoding to the end of read/subscribe request followwing the spec either using external version filters or using cached data versions if cache is available
yunhanw-google
force-pushed
the
feature/issue_16243
branch
from
bc88894 to
6159605
Compare
|
PR #16602: Size comparison from 184bda1 to 6159605 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
yunhanw-google
force-pushed
the
feature/issue_16243
branch
from
6159605 to
ff29870
Compare
|
PR #16602: Size comparison from e70cc69 to ff29870 Increases above 0.2%:
Increases (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (31 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
mrjerryjohns
approved these changes
Apr 14, 2022
|
(I hope this is the right place to ask, please apologize/redirect if not) I found this commit breaks ReadRequestMessage protocol compatibility across April 14th, as it changes the context-specific tag numbers, see diff below. Of course I understand this is all in development, so it might be an non-issue - if so, please apologize me asking. My question - as the protocol break is nowhere mentioned in the PR - is: is this really an intentional change? It means that builds before and after April 14 are fundamentally incompatible. If this was an adaption to bring code in alignment with specs, I'd guess that would have been mentioned in the commit (I do not have access to the latest specs, but the tag order before this PR did conform to an earlier version). I debugged down to this while trying to get one of the linux examples pair with the Home.app on iPad. Apparently the current iOS versions, including latest betas, still use a version before this change, so any read request to and from builds after this PR fail with diff --git a/src/app/MessageDef/ReadRequestMessage.h b/src/app/MessageDef/ReadRequestMessage.h
index 07e46d910..8e1465618 100644
--- a/src/app/MessageDef/ReadRequestMessage.h
+++ b/src/app/MessageDef/ReadRequestMessage.h
@@ -36,10 +36,10 @@ namespace ReadRequestMessage {
enum class Tag : uint8_t
{
kAttributeRequests = 0,
- kDataVersionFilters = 1,
- kEventRequests = 2,
- kEventFilters = 3,
- kIsFabricFiltered = 4,
+ kEventRequests = 1,
+ kEventFilters = 2,
+ kIsFabricFiltered = 3,
+ kDataVersionFilters = 4,
};
class Parser : public MessageParser |
|
@plan44 The spec used to not define which tag DataVersionFilters was at all. That was fixed in https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/4284 and then the SDK was aligned with the spec. |
|
thank you very much for the explanation! It's not so easy to work with an Open SDK and a closed spec - a small single person company like mine can't afford a csaiot membership just for doing a PoC. Scaling up the 7k yearly fee from mine to Apple's revenue, Apple would have to pay around 10 billion $ ;-) |
|
@plan44 Yes, I totally understand your pain.... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
AttributeCacheshould cache data versions and use them in subsequent Read/Subscribe requests #16243Change overview
Note: we don't allow multiple read/subscribe interaction to use the same attribute cache at the same time.
Testing
Expect no versions would be cached.
Expect cache C1 version
Expect no C1 attributes in report, only C2A1 attribute in report
Expect no C1 attributes in report, only C2A1 attribute in report
Expect C1A1, C1A2, C2A1 attribute in report, and invalidate the cached pending and committed data version since no wildcard attributes exists in mRequestPathSet.
Expect C1A1, C1A2, C2A1 attribute in report
Expect C1 attributes in report, and C2A1 attribute in report and cache latest data version
data version and construct data version list but no enough memory, finally fully rollback data version filter
Expect C1 attributes in report, and C2A1 attribute in report
Expect C1, C2, C3 attributes in report, and cache their versions
finally partially rollback data version filter with only C2
Expect C1, C3 attributes(7 attributes) in report