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

samples: matter: Align changes in all samples to sysbuild #15686

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

ArekBalysNordic
Copy link
Contributor

@ArekBalysNordic ArekBalysNordic commented Jun 4, 2024

  • Removed redundant _release pm_static files.
  • Moved Matter diagnostic log snippets to theNRF common snippet directory.
  • Simplified and reorganized kconfig.sysbuild files.
  • Aligned Matter docs to sysbuild changes.
  • Added several entries to the migration guide.

This PR repairs multiple bugs in Matter regarding sysbuild.

Depends on #15340

@ArekBalysNordic ArekBalysNordic added this to the 2.7.0 milestone Jun 4, 2024
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jun 4, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jun 4, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@ArekBalysNordic ArekBalysNordic force-pushed the align_matter_to_sysbuild branch 2 times, most recently from 08ecb3a to 805d12c Compare June 4, 2024 09:08
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@ArekBalysNordic ArekBalysNordic force-pushed the align_matter_to_sysbuild branch 3 times, most recently from d1c86c8 to 42e0611 Compare June 4, 2024 14:35
@ArekBalysNordic ArekBalysNordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 4, 2024
@ArekBalysNordic ArekBalysNordic requested a review from a team June 4, 2024 14:48
@ArekBalysNordic ArekBalysNordic added the bugfix Fixes a known bug label Jun 5, 2024
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 5, 2024
@ArekBalysNordic ArekBalysNordic force-pushed the align_matter_to_sysbuild branch 4 times, most recently from 85d227f to 765aede Compare June 5, 2024 13:55
@ArekBalysNordic ArekBalysNordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 5, 2024
applications/matter_bridge/Kconfig.sysbuild Show resolved Hide resolved
* With the inheritance of Zephyr's :ref:`zephyr:sysbuild` in the |NCS|, some changes are provided to the Matter samples and applications:

* :kconfig:option:`CONFIG_CHIP_FACTORY_DATA_BUILD` kconfig option is deprecated and you need to use the :kconfig:option:`SB_CONFIG_MATTER_FACTORY_DATA` Kconfig option instead to enable or disable creating the factory data set during building a Matter sample.
To enable factory data support on your device, you still need to set the :kconfig:option:`CONFIG_CHIP_FACTORY_DATA` to ``y``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Really? A user should not have to set this, sysbuild should propagate that setting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But one thing is to generate factory data, other to enable support for that in Matter sample. Sysbuild forced us to use this config but it is used only for generate factory data files. In fact sysbuild complicated our factory data.

Copy link
Contributor

Choose a reason for hiding this comment

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

So should the SB option being set, force set the option set in the default image?

Copy link
Contributor Author

@ArekBalysNordic ArekBalysNordic Jun 6, 2024

Choose a reason for hiding this comment

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

OK, so the main thing is that we used to have two configs: CONFIG_CHIP_FACTORY_DATA that enables support for factory data and does not generate a factory data set (users can do it using their scripts) and it was used just to enable parsing fd in the application. The second one: CONFIG_CHIP_FACTORY_DATA_BUILD was enabling the CMAKE script for generating a factory data set.
After moving to sysbuild only the second one became "deprecated" because you suggested creating the SB_CONFIG_MATTER_FACTORY_DATA config that only generates the factory data set. So now it is complicated, and we want to make it as easy as possible for customers and not provide many changes because they're using those mechanisms in their production. I thought that the easiest way is to inform users that CONFIG_CHIP_FACTORY_DATA_BUILD has been deprecated and now they must use SB_CONFIG_MATTER_FACTORY_DATA instead. But on the other hand, the SB_CONFIG_MATTER_FACTORY_DATA kconfig works only for sysbuild, so the old one must be available for non-sysbuild purposes, right? All other things are exactly the same, and they can manipulate the CONFIG_CHIP_FACTORY_DATA kconfig to enable/disable fd support.

Are you seeing a better option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change this kconfig to SB_CONFIG_MATTER_FACTORY_DATA_BUILD, which describes the purpose better.

Copy link
Contributor

Choose a reason for hiding this comment

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

does this need updating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is correct, Users need to use SB_CONFIG_MATTER_FACTORY_DATA_GENERATE to generate factory data, but apart from that they need to use CONFIG_CHIP_FACTORY_DATA to enable factory data support on SoC.


* With the inheritance of Zephyr's :ref:`zephyr:sysbuild` in the |NCS|, some changes are provided to the Matter samples and applications:

* :kconfig:option:`CONFIG_CHIP_FACTORY_DATA_BUILD` kconfig option is deprecated and you need to use the :kconfig:option:`SB_CONFIG_MATTER_FACTORY_DATA` Kconfig option instead to enable or disable creating the factory data set during building a Matter sample.
Copy link
Contributor

Choose a reason for hiding this comment

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

deprecated is wrong, deprecated seems like it still works, but it does not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it still works for non-sysbuild (child images) sample variant that as far as I know must be supported. But ok, deprecated may be misleading here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-thinked it and "deprecated" is correct here, because in fact it still must be used for the non-sysbuild variants. I will add a one-line comment there.

samples/matter/lock/README.rst Outdated Show resolved Hide resolved
@nordicjm
Copy link
Contributor

nordicjm commented Jun 6, 2024

@kkasperczyk-no

Ok and who decides about it?

I do.

The mcumgr is the command line tool recommended by Nordic for the purpose of DFU updates for mcuboot.

You're recommended a dead, faulty tool which has the following notice

This tool is provided for evaluation use only and is not recommended for use in a production environment. It has known issues and will not respect the MCUmgr protocol properly e.g. when an error is received, instead of aborting will, in some circumstances, sit in an endless loop of sending the same command over and over again. A universal replacement for this tool is currently in development and once released, support for the go tool will be dropped entirely. It is recommended that usage of tools listed above in the Tools/libraries section are used instead of the go client.

@wbober
Copy link
Contributor

wbober commented Jun 6, 2024

@kkasperczyk-no

Ok and who decides about it?

I do.
The mcumgr is the command line tool recommended by Nordic for the purpose of DFU updates for mcuboot.

You're recommended a dead, faulty tool which has the following notice

This tool is provided for evaluation use only and is not recommended for use in a production environment. It has known issues and will not respect the MCUmgr protocol properly e.g. when an error is received, instead of aborting will, in some circumstances, sit in an endless loop of sending the same command over and over again. A universal replacement for this tool is currently in development and once released, support for the go tool will be dropped entirely. It is recommended that usage of tools listed above in the Tools/libraries section are used instead of the go client.

I'm well aware of the notice. Thank you.

The tool is used by a number of teams inside Nordic for development as well as in the CI infrastructure for end-to-end testing. This has been the case for many years. While it may have its shortcomings it serves its purpose well enough.

@ArekBalysNordic ArekBalysNordic force-pushed the align_matter_to_sysbuild branch 5 times, most recently from e18ccc5 to cac1f6e Compare June 7, 2024 13:56
@ArekBalysNordic
Copy link
Contributor Author

@nordicjm @LuDuda @kkasperczyk-no I reverted the changes and as we discussed we're going to leave SB_CONFIG_MATTER... configs for factory data and OTA, and I wrote the limitation in several places - in the migration guide, factory data guide and all sample README files. Please review it once again.

@ArekBalysNordic ArekBalysNordic force-pushed the align_matter_to_sysbuild branch 2 times, most recently from c7ab29e to 9ec7283 Compare June 7, 2024 14:04
samples/matter/lock/README.rst Outdated Show resolved Hide resolved
applications/matter_weather_station/README.rst Outdated Show resolved Hide resolved
* :kconfig:option:`SB_CONFIG_MATTER_OTA` Kconfig option has been added to enable or disable generating Matter OTA package during the building process.
* :kconfig:option:`CONFIG_CHIP_OTA_IMAGE_FILE_NAME` Kconfig option is deprecated and you need to use the :kconfig:option:`SB_CONFIG_MATTER_OTA_IMAGE_FILE_NAME` Kconfig option instead to define Matter OTA output filename.

If you modify any of the factory data-related Kconfig option you need to build a Matter sample with the pristine option to clear the CMake cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you modify any of the factory data-related Kconfig option you need to build a Matter sample with the pristine option to clear the CMake cache.
If you modify any of the factory data-related Kconfig options, you need to either build the sample or application with the pristine option to clear the CMake cache or clear the cache manually.

Feel free to leave out the manual clearing option if it was left out of the migration guide on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave it because somebody can for example remove the build directory manually and in this way, remove the cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can actually go entirely, I tested and if these options are changed, they are picked up and a new image is generated. Must have been an issue that has been fixed sometime without being aware

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I will remove the note from all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed all those lines that inform about the pristine build.

@LuDuda
Copy link
Contributor

LuDuda commented Jun 10, 2024

FYI. Have to be merged after: #15836

@ArekBalysNordic ArekBalysNordic force-pushed the align_matter_to_sysbuild branch 2 times, most recently from 0fbf045 to 0cad975 Compare June 10, 2024 14:22
@ArekBalysNordic
Copy link
Contributor Author

@nordicjm can we merge nrfconnect/sdk-connectedhomeip#453 and update SHA?

@ArekBalysNordic ArekBalysNordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 11, 2024
- Removed redundant `_release` pm_static files.
- Moved Matter diagnostic log snippets to theNRF common
snippet directory.
- Simplified and reorganized kconfig.sysbuild files.
- Aligned Matter docs to sysbuild changes.
- Added several entries to the migration guide.
- Removed wrong MATTER sysbuild configs and started
using the previous ones.

Signed-off-by: Arkadiusz Balys <arkadiusz.balys@nordicsemi.no>
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 11, 2024
@ArekBalysNordic ArekBalysNordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 11, 2024
@NordicBuilder NordicBuilder removed the DNM label Jun 11, 2024
@anangl anangl merged commit 52c4f04 into nrfconnect:main Jun 11, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug doc-required PR must not be merged without tech writer approval. manifest manifest-matter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants