-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix computation of min/max interval for subscriptions to be spec comp… #19262
Merged
bzbarsky-apple
merged 3 commits into
project-chip:master
from
yunhanw-google:feature/fix_max_computation
Jun 11, 2022
Merged
Fix computation of min/max interval for subscriptions to be spec comp… #19262
bzbarsky-apple
merged 3 commits into
project-chip:master
from
yunhanw-google:feature/fix_max_computation
Jun 11, 2022
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
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
lazarkov,
LuDuda,
mlepage-google and
msandstedt
June 7, 2022 08:11
pullapprove
bot
requested review from
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
June 7, 2022 08:11
PR #19262: Size comparison from 55ab764 to aff91d0 Increases (5 builds for cc13x2_26x2)
Decreases (29 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Full report (30 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
mrjerryjohns
reviewed
Jun 7, 2022
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.
Logic isn't quite right.
mrjerryjohns
reviewed
Jun 7, 2022
bzbarsky-apple
approved these changes
Jun 8, 2022
woody-apple
approved these changes
Jun 9, 2022
yunhanw-google
force-pushed
the
feature/fix_max_computation
branch
from
June 10, 2022 22:08
03c9f05
to
f1dfa08
Compare
PR #19262: Size comparison from d1d5ca8 to f1dfa08 Increases (2 builds for linux)
Decreases (7 builds for cyw30739, linux, telink)
Full report (8 builds for cyw30739, linux, mbed, telink)
|
yunhanw-google
force-pushed
the
feature/fix_max_computation
branch
from
June 10, 2022 22:31
f1dfa08
to
5ecf97a
Compare
PR #19262: Size comparison from d1d5ca8 to 5ecf97a Increases (7 builds for cc13x2_26x2, linux)
Decreases (40 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, nrfconnect, p6, telink)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
msandstedt
approved these changes
Jun 11, 2022
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
Recent changes to the spec now alter the way the final min/max intervals are computed:
MinIntervalFloor ≤ MaxInterval ≤ MAX(SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT, MaxIntervalCeiling)
Where SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT is set to 60m in the spec.
Fixes #19129
spec change is https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5297
Change overview
Remove MinInterval from the Subscribe Response and update the APIs to automatically compute this based on the requested value from the client.
Alter the ReadHandler::SetReportingIntervals to correctly handle the above constraints
Testing
Subscriber requesting a number of different values for max-interval:
a. With no server adjustment.
b. With server adjustment to a value greater than client-requested, but less than 60m (allowed).
c. With server adjustment to a value greater than client-requested, but greater than 60 (not allowed).
a. With no server adjustment.
b. With server adjustment to a value lower than 60m.
c. With server adjustment to a value larger than 60m, but less than max interval .
d. With server adjustment to a value larger than max interval .