Skip to content
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

Pull recent vital Matter SDK improvements #329

Merged

Conversation

Damian-Nordic and others added 2 commits October 4, 2023 09:09
1. Run Matter shell commands in Matter thread instead of
   Zephyr's shell thread. Make the shell thread wait for
   the condition variable signalled when the command ends
   in the Matter thread.

   This is done to avoid data races when accessing Matter's
   data structures, and avoid stack overflow when executing
   Matter functions in the shell thread that uses
   a relatively small stack.
2. Print either:
   - "Done" or
   - "Error: <error_string_or_code>"
   after each shell command. This is to align with other
   platforms and be able to remove unnecessary logging
   from existing shell command implementations.

Signed-off-by: Damian Krolik <[email protected]>
* Optimize compilation time

1. Do not to include cluster-objects.h if not needed as
   parsing such a large header significantly impacts the
   build time and in most cases it is enough to include
   ClusterEnums.h.
2. Do not build the controller library for embedded targets,
   by default. If needed, this can be overriden using the
   chip_build_controller argument from lib.gni.

* Fix build
…t of Server.h. (#26519)

This makes it easier to use for clients that don't have reliable wall-clock
time.
…y default. (#26530)

If an explicit validity policy is injected that validates notBefore/notAfter, we
will do that, but if the app author just doesn't think about time-based
validation default to not validating, because there's a good chance it will just
lead to unexpected failures due to bad clocks and whatnot.
…rs. (#28278)

* Reduce the size of FabricInfo by reordering some members.

On 32-bit systems this changes the size from 152 bytes to 144 bytes.

On 64-bit systems this changes the size from 168 bytes to 152 bytes.

* Address review comment.

* Remove static asserts, because offsetof is not happy on FabricInfo.
Copy link
Contributor

@kkasperczyk-no kkasperczyk-no left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to review all the commits from Boris, as I assume they were approved in the upstream, so it should be ok, however I have two questions:

  1. If I understand correctly these are all post Matter 1.1 commits that you pull to the 2.5.0 release. Are we sure that none of them breaks the compatibility with 1.1 by using some new 1.2 related features/bug fixes?
  2. Do you think running integration suite is enough to verify it or we should at least once run the nightly against this PR? I'm not suggesting it's necessary, these changes are just hard to review, so maybe tests would prove we're safe.

@markaj-nordic markaj-nordic force-pushed the pull_recent_improvements branch from 9d968a6 to bc50d54 Compare October 9, 2023 08:51
…ssion. (#28078)

If the ReadClient is doing the CASE establishment itself, it should give the
client a way to modify the ReadPrepareParams based on the session information.
…n packet. (#28346)

* Improve encoding of lists where first item can't fit in packet.

When the first item of a list can't fit in the current packet, it's better to
just send the packet without the list at all, then encode as many items as we
can in the packet that follows before we start doing IB-per-item.

Otherwise (before this change) we encode an empty list IB, then end up doing
IB-per-item for the whole list.

* Improve unit tests.
@markaj-nordic markaj-nordic force-pushed the pull_recent_improvements branch 2 times, most recently from 8889e70 to 48751e9 Compare October 9, 2023 09:35
* Improve size calculation for our packets.

We were hardcoding kMaxAppMessageLen and kMaxSecureSduLengthBytes, but those
defines were not even consistent with each other, and both were overly
conservative.  Specific changes:

1. Fix computation of CHIP_SYSTEM_HEADER_RESERVE_SIZE: we don't have any "crypto
   headers".
2. Fix computation of kMaxAppMessageLen to use our actual packet buffer sizes
   and defined header sizes.
3. Fix kMaxSecureSduLengthBytes to be consistent with kMaxAppMessageLen.

* Simplify the size computation logic.
…#28569)

* Fix cancellation of subscriptions to work correctly.

When a subscription came in with KeepSubscriptions set to false, we would just
directly delete the ReadHandlers for the subscriptions not being kept, instead
of calling Close().

That meant we skipped deleting subscription persistence data, and also failed to
correcly update the reporting engine's round-robin reporting state, which could
lead to subscriptions not being serviced fairly.

The fix is to just call Close() just like we do for out-of-resource eviction,
instead of manually deleting the object.

* Fix build issue.
In BuildSingleReportDataAttributeReportIBs if we failed to successfully call
ReserveBuffer(kReservedSizeEndOfReportIBs) (because there was not enough space
in the packet), we would end up trying to EndOfAttributeReportIBs() after
unreserving (which would not match our never-happened reserve) and then
VerifyOrDie that we succeeded.

The fix is to keep track of whether we actually successfully reserved things,
and not override out-of-space-errors if we have not.
…6761)

* Do proper ACL checks on event reads/subscriptions.

This adds the following functionality:

1. We now correctly detect subsciptions that don't have any access to anything,
   even if they have an event path in the subscribe request.  For paths with a
   wildcard event id, this check assumes read privileges are needed when event
   lists are disabled, and uses the actual event-specific privileges when event
   lists are enabled.
2. When doing reads of an unsupported event, correctly return an
   errors instead of an empty event list.
3. Fix various unit test mocks to provide the information needed for the new
   checks.
4. Update expectation in existing YAML test that was checking an "unimplemented
   event" case.

* Address review comments.

* Fix darwin build.

* Fix Darwin tests, now that we get errors for unsupported events.

* Move function declarations to a non-codegen-dependent header.

* Handle ACL checks for event wildcards even if we have no EventList.

* Update to spec change for unsupported event errors.

* Address review comments.
* Update Groups cluster XML to match spec.

Spec changes happened in CHIP-Specifications/connectedhomeip-spec#6429

Fixes project-chip/connectedhomeip#26209

* Auto-update ZAP files.

* Regenerate generated code.
There are three fixes here:

1. Move the epoch key validity checks up front, since per spec those should
   happen before any internal state verification checks.

2. Add a check that the GroupKeySecurityPolicy in the keyset has a valid value
   for GroupKeySecurityPolicyEnum.

3. If we don't support MCSP, we should be failing out if the
   GroupKeySecurityPolicy is set to CacheAndSync.

Fixes project-chip/connectedhomeip#26692
…it's defined. (#27679)

The definition of CHIP_CONFIG_SECURE_SESSION_POOL_SIZE uses
CHIP_CONFIG_MAX_FABRICS but came before we ensures CHIP_CONFIG_MAX_FABRICS is
defined.  This change just moves it to right after the definition of
CHIP_CONFIG_MAX_FABRICS.
…toryConfig. (#27949)

We were not checking the length (which must be 2), so would allow 1-char or
0-char values.

Also aligns the exact logic with the Location attribute write code and adds some
error logging.
@markaj-nordic markaj-nordic force-pushed the pull_recent_improvements branch from 48751e9 to bab4188 Compare October 9, 2023 09:48
@markaj-nordic
Copy link
Contributor Author

I'm not sure how to review all the commits from Boris, as I assume they were approved in the upstream, so it should be ok, however I have two questions:

1. If I understand correctly these are all post Matter 1.1 commits that you pull to the 2.5.0 release. Are we sure that none of them breaks the compatibility with 1.1 by using some new 1.2 related features/bug fixes?

2. Do you think running integration suite is enough to verify it or we should at least once run the nightly against this PR? I'm not suggesting it's necessary, these changes are just hard to review, so maybe tests would prove we're safe.

I hope we have already filtered out commit which affected the 1.1 version. The rest of the commits are more performance improvement related. Anyway, of course I agree that we should run nightly suite on this PR.

@markaj-nordic markaj-nordic removed the DNM label Oct 10, 2023
@markaj-nordic markaj-nordic merged commit 96ea933 into nrfconnect:master Oct 10, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants