Rework the notion of Epoch#449
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a stub implementation of the Software Diagnostics cluster, allowing the project to pass conformance tests TC_DGSW_2_1 and TC_DGSW_2_2. The changes include the new cluster handler, its integration into the root endpoint, and updates to the integration test runner. Additionally, the long-read and subscription tests were refactored to use a set-based path verification approach for better maintainability. Feedback was provided regarding the loss of data value verification in these tests, suggesting the use of a map or a subset of checks to ensure data correctness alongside path coverage.
|
PR #449: Size comparison from 2418d1d to 934bff0 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #449: Size comparison from 2418d1d to 8c26500 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #449: Size comparison from 2418d1d to 49a8d33 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
more-handlers
branch
2 times, most recently
from
d31ff74 to
7f223ee
Compare
|
PR #449: Size comparison from 7d96427 to 650f7d5 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #449: Size comparison from da2d79e to bead23c Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #449: Size comparison from da2d79e to 7c6cd69 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
changed the title
|
PR #449: Size comparison from da2d79e to ceb98ab Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
more-handlers
branch
2 times, most recently
from
d73b27c to
042e676
Compare
ivmarkov
changed the title
Contributor
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the Software Diagnostics and Time Synchronization clusters in rs-matter. It introduces a robust mechanism for tracking Last-Known-Good UTC Time, including persistence and build-time seeding, and adds a TimeSyncClient for time synchronization. The code changes also refactor the system handler registration to be more modular using builders. I have kept the review comment regarding the manual Debug and defmt::Format implementations in NocHandler, as it suggests a cleaner, more idiomatic approach.
|
PR #449: Size comparison from da2d79e to d611cd4 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #449: Size comparison from da2d79e to f7acce4 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #449: Size comparison from da2d79e to 5396a16 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
…o reduce code size
|
PR #449: Size comparison from da2d79e to 4d1b3a2 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
(This PR started with the introduction of the Software Diagnostics cluster handler, which currently I cannot remove, as it is intermingled with the rest, but that's a small code change anyway.)
===
The important change here is the retirement of the
Epochtype-alias from thers-mattercodebase.The
Epoch (= fn() -> core::time::Duration)did have the semantics of a piece of code returning the current time since the Unix epoch.However, and in 99% of the cases, we needed a much weaker semantics - i.e. to measure some duration between a previous event, and a current event, where both events did happen during the current MCU boot cycle.
This is perfectly doable by just utilizing
embassy_time::Instant::now()whose semantics is anyway "monotonic time since boot", so most of the call-sites usingEpochwere just switched toembassy_time::Instant::now().The few call-sites that did need a wall-clock (not even a monotonic btw) - these are primarily certificate validation call-sites - are now switched to a wholly-new infrastructure that revolves around the
TimeSynccluster, which is now implemented.Out of the box,
rs-matter(and itsTimeSynccluster handler) now does support:NB: To reduce the flash-code-size price of the new time-sync infrastructure, the
endpoints::with_XXX_sysroot-endpoint handler-builders had to be rewritten to work upside-down:This allowed to combine multiple non-async handlers together before wrapping them with
Async(...)which reduces the code-bloat generated by the compiler async state machinery.