Fix issues in 27575 #32772
Closed
Fix issues in 27575 #32772
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,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda and
mhazley
pullapprove
bot
requested review from
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
…_r1240951613 Do not see a way to avoid reloading project-chip#26082 (comment)
…2621779 Co-authored-by: Boris Zbarsky <[email protected]>
…/files#r1242627777 - added new delegate functions to nofify important changes - free readclient memory once operation is complete - don't immediately emit time failure event when trusted time source is set
…242655854 - https://github.com/project-chip/connectedhomeip/pull/26082/files#r1242660231 - https://github.com/project-chip/connectedhomeip/pull/26082/files#r1242658973 - https://github.com/project-chip/connectedhomeip/pull/26082/files#r1242657708 - https://github.com/project-chip/connectedhomeip/pull/26082/files#r1242667611 - https://github.com/project-chip/connectedhomeip/pull/26082/files#r1242673209 - https://github.com/project-chip/connectedhomeip/pull/26082/files#r1242683543
…rrent ToT and is fixed here
|
PR #32772: Size comparison from 036e7bf to 4b25a04 Full report (3 builds for cc32xx, stm32)
|
fessehaeve
force-pushed
the
pull-request/timesync-fix-27575
branch
from
4b25a04 to
cb49534
Compare
- fix format on logging
|
PR #32772: Size comparison from 036e7bf to 987b7a7 Increases (5 builds for cyw30739, esp32, telink)
Decreases (4 builds for efr32, esp32, psoc6)
Full report (55 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, mbed, psoc6, qpg, stm32, telink)
|
There was a problem hiding this comment.
This seems like it should have been multiple sane-sized reviewable PRs, not a single giant blob....
There was a problem hiding this comment.
I could break this in to 2 or 3 PRs but more would be too much for me to maintain.
Would you prefer I do that @bzbarsky-apple ?
There was a problem hiding this comment.
Is that because the dozens of issues listed in #27575 involve touching the same bits of code? Or something else?
Ideally, we would have one PR per issue or close to it. Right now trying to review this and make sure it actually fixes the list of issues it's claiming is something like O(N*M) where N is size of PR and M is number of issues it claims to fix....
There was a problem hiding this comment.
To be exact, these are not issues. They are all comments (may be for 1 or 2 exceptions) that are tracked in 1 issue.
There was a problem hiding this comment.
I know they are tracked in one github issue. But they are independent bits that need to be fixed, and comparing the PR to that list is very painful.
There was a problem hiding this comment.
I tried to break down the PR but at this point it needs too much work to align the changes without causing build errors.
Therefore, I will list down all the changes in this PR in the description. I hope that works for you.
There was a problem hiding this comment.
The big problem is comparing the list in the description to the list of things this is supposed to be fixing and comparing one list or the other to the code....
That is, how do I check within sane time that this PR in fact fixes the things it claims to be fixing?
- reading DSToffset keeps the list updated, therefore only remaining valid entry is returned on reading it
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.
Description of issues fixed from #27575 :
Addition of delegate functions for better usability: