-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added Matter contact sensor sample #24233
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
Conversation
Since quarantine was modified, please make sure you are following the process described in Quarantine Process. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: e1f673c0517f7c5ac0d9b79225baf06cc1d7b29d more detailssdk-nrf:
Github labels
List of changed files detected by CI (55)
Outputs:ToolchainVersion: 2b2cd9579a Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
f1bce89
to
84e35eb
Compare
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsdoc.z6.web.core.windows.net/PR-24233/nrf/protocols/matter/getting_started/low_power_configuration.html |
898dca4
to
04463b2
Compare
04463b2
to
5a93a8c
Compare
@nrfconnect/ncs-co-build-system please review |
samples/matter/contact_sensor/boards/nrf52840dk_nrf52840.overlay
Outdated
Show resolved
Hide resolved
samples/matter/contact_sensor/boards/nrf5340dk_nrf5340_cpuapp.overlay
Outdated
Show resolved
Hide resolved
samples/matter/contact_sensor/sysbuild/mcuboot/boards/nrf54l15dk_nrf54l15_cpuapp.conf
Outdated
Show resolved
Hide resolved
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.
why would mcuboot power off ?
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.
I don't know the details, as @ArekBalysNordic added it, but as far as I remember it was a known issue for nRF54L15 and it was recommended to be set by LL team. Without it, the power consumption was too high, after leaving the bootloader stage.
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.
@kl-cruz @nika-nordic can you help?
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.
ping @kl-cruz @nika-nordic this Kconfig and reasoning makes no sense
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.
I think this CONFIG_POWEROFF=y
is no longer needed. It is an artifact related to lack of zephyrproject-rtos/zephyr#79199 (merged in zephyrproject-rtos/zephyr#78631 ). Looks like GRTC power management was addressed properly in zephyrproject-rtos/zephyr#79199 however this CONFIG_POWEROFF=y
remained in the code.
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.
Removed
samples/matter/contact_sensor/sysbuild/mcuboot/boards/nrf5340dk_nrf5340_cpuapp.overlay
Outdated
Show resolved
Hide resolved
samples/matter/contact_sensor/sysbuild/mcuboot/boards/nrf5340dk_nrf5340_cpuapp.conf
Outdated
Show resolved
Hide resolved
samples/matter/contact_sensor/sysbuild/mcuboot/boards/nrf5340dk_nrf5340_cpuapp.conf
Outdated
Show resolved
Hide resolved
...es/matter/contact_sensor/sysbuild/mcuboot/boards/nrf54lm20dk_nrf54lm20a_cpuapp_internal.conf
Outdated
Show resolved
Hide resolved
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.
# Use minimal C library instead of the Picolib | |
CONFIG_MINIMAL_LIBC=y |
piclibc build is smaller than minimal, never use minimal going forward unless it's for native_sim or a test
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.
The background for this configuration is that in the past minimal LIBC allowed us to have much smaller memory usage than with picolib. I can see that now it's comparable, but still with picolib it's ~100 B greater memory size than with minimal libc. Also, please mind that a lot of other applications and samples from nrf use minimal libc for mcuboot (if not all of them).
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.
Minimal libc is a testing library for unit and regression tests, it is not meant for usage in applications. To reduce MCUboot build size, enable LTO on the MCUboot image. Existing usage of minimal libc should be updated to use picolibc
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.
Ok, I can see that default mcuboot configuration was changed to picolib like 2 months ago. If this is what we currently recommend then I will align Matter configs as well.
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.
@nordicjm the comparable size that I've mentioned before was for nRF54L. For nRF52840, after switching to picolibc, the memory usage was increased by 16k. Do you have idea why? I can't enable it for nRF52840, as it will simply not fit in the partition.
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.
@nordicjm I believe we just inherited it from some minimal mcuboot configuration in the past.
This is for example used here (recommended reference mcuboot configuration by Pluto): https://github.com/nrfconnect/sdk-nrf/blob/main/samples/zephyr/smp_svr_mini_boot/sysbuild/mcuboot/prj.conf#L32-L33
But ofc, if it makes code smaller and is used in testing, we should definitely switch!
Added temperature sensor sample entries for low power and tx power configuration pages. Signed-off-by: Kamil Kasperczyk <[email protected]>
5a93a8c
to
b94329f
Compare
@nordicjm this PR is in fact copy+paste of other existing Matter samples with small code changes, but configuration is the same for majority of other samples. I will align style changes to other samples in follow-up PR. You asked multiple questions about the mcuboot configuration that looks like this from at least few years, so unfortunately I'm not able to answer on all of them, because I simply don't remember, or I was not involved in configuring it this way. Nevertheless I tried to put some answers above. |
b94329f
to
b6785ae
Compare
b6785ae
to
401d56f
Compare
@nordicjm could you have a second look, so i could proceed with this PR? |
20e78e2
to
4b495c2
Compare
@nordicjm the minimal libc entry was removed. |
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.
changes OK but last 2 commits need squashing
4b495c2
to
b5e05ae
Compare
b5e05ae
to
8228151
Compare
src | ||
${ZAP_PARENT_DIR} |
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.
cmake indent is 2 spaces
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.
This comment and the one related to .overlay files applies to all Matter samples, as the files are copy-pasted. It will be fixed by: KRKNWK-20778.
# Low Power mode | ||
CONFIG_POWEROFF=y |
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.
so this is no longer needed either then as per LL comment? Whole file can go
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.
Possibly, but it would need to be verified on desk using PPK to not introduce regression in power consumption. I don't have it currently on desk to do that, so this will be done in scope of: KRKNWK-20777, as soon as I will visit office to measure it.
|
||
&cpuapp_sram { | ||
reg = <0x20000000 DT_SIZE_K(256)>; | ||
ranges = <0x0 0x20000000 0x40000>; |
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.
ranges = <0x0 0x20000000 0x40000>; | |
ranges = <0x0 0x20000000 0x40000>; |
|
||
&cpuapp_sram { | ||
reg = <0x20000000 DT_SIZE_K(512)>; | ||
ranges = <0x0 0x20000000 0x80000>; |
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.
there are multiple issues as above, fix in whole PR
Hi @nordicjm,
|
I will fix them in all samples in the following PR, because I'm working on fixing consistency between all our samples. |
Why can't they be fixed in this PR (for this sample, other samples can be fixed in a future PR)? This PR is non compliant with our coding style |
Because I'm creating a script to check consistency between our samples, it is better for me to do it at once for all of them. Moreover, we want to finalize this PR. |
Not really a valid reason for not fixing things in this PR, a script can be made later, why is that needed to fix 7 dts spacing issues, fix 1 cmake spacing issue and remove 1 file, which should take < 1 minute |
Yeah, changing in the code takes < 1 minute, whereas waiting for CI is more than 1.5 hours... |
The comments were left yesterday which means if they were fixed then, this PR would have been merged by now |
Added new Matter sample - contact sensor. Updated also documentation and .yaml test configurations. Introduced several fixes for the configuration recommended during code review. Signed-off-by: Kamil Kasperczyk <[email protected]>
8228151
to
e1f673c
Compare
Done, @nordicjm, please take a look. |
The valid reason for not fixing things in this PR is that I've communicated that we need to have this in to unblock our progress and you ignored it, requesting style fixes as a blocker for this PR. @ArekBalysNordic thank you for handling it. |
And I quote https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html
Hopefully in future those checks are automated as part of CI and then if there is a disagreeance you can argue with a bot |
And I quote: https://github.com/zephyrproject-rtos/zephyr/blob/main/doc/contribute/reviewer_expectations.rst
and
and
Probably this is a matter of perspective, but to me, we currently have SDK in some shape. We have samples that have not-aligned style or configuration from like 4-5 years. And yes it should be planned and fixed, but requesting to do that in random moment of time is not a perfect solution for us considering project planning. Especially when we are not aware that these issues exist, so to me this is actually increasing scope of PR and to me, if the style issues were there from few years it's not a blocking comment to address them now. I have already started discussion with @carlescufi about priorities, what is blocking and what is not in terms of our SDK. I have a hope we can produce some rules how to handle it and conclude it with some agreement. |
No description provided.