Skip to content

Conversation

@krish2718
Copy link
Contributor

This enables the assert for minimum libc HEAP and fails boot, saves us time from hard to debug issues due to low HEAP.

@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. manifest labels Feb 15, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Feb 15, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
sdk-hostap nrfconnect/sdk-hostap@8d3f26d nrfconnect/sdk-hostap#67 nrfconnect/sdk-hostap#67/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Feb 15, 2023

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

@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.

@krish2718
Copy link
Contributor Author

With recent influx of changes the RAM usage is at peak, so, we have made changes to samples to decrease CONFIG_HEAP_MEM_POOL_SIZE but still to avoid obscure failures in WPA supplicant this PR helps by adding the assert early. Please verify CONFIG_HEAP_MEM_POOL_SIZE in your respective samples @markaj-nordic @simensrostad @wbober

@krish2718 krish2718 added this to the 2.3.0 milestone Feb 15, 2023
@simensrostad
Copy link
Contributor

@krish2718

I get a build warning in the MQTT sample for this manifest update:

warning: LOG_BUFFER_SIZE (defined at
/home/simensr/ncs/nrf/subsys/net/openthread/Kconfig.defconfig:53,
/home/simensr/ncs/modules/lib/hostap/zephyr/Kconfig:129, subsys/logging/Kconfig.processing:120) was
assigned the value '3072' but got the value '2048'. See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_LOG_BUFFER_SIZE and/or look up
LOG_BUFFER_SIZE in the menuconfig/guiconfig interface. The Application Development Primer, Setting
Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful
too.

I peaked a bit into sdk-hostap and saw that the log buffer is redefined in there. Shouldn't it be up to the application/sample to set this buffer depending on the modules it includes. If the sample uses immediate logging its not used either AFAIK.

@krish2718
Copy link
Contributor Author

That's just the default, sample can overwrite that, let me check why it is warning?

@krish2718 krish2718 added the bugfix Fixes a known bug label Feb 16, 2023
@krish2718
Copy link
Contributor Author

CI failures are being fixed by #10304

@markaj-nordic
Copy link
Contributor

CI failures are being fixed by #10304

@krish2718 the Matter PR is now merged, you can rebase your branch.

@krish2718 krish2718 force-pushed the add_min_libc_ram_assert branch from 18124bc to 721537a Compare February 16, 2023 10:44
@NordicBuilder NordicBuilder removed the DNM label Feb 16, 2023
@simensrostad
Copy link
Contributor

That's just the default, sample can overwrite that, let me check why it is warning?

Its a warning because its set in the board config overlay file. Since the default is higher with these changes I think it can be removed from the nrf7002dk_nrf5340_cpuapp in this PR.

@tmon-nordic
Copy link
Contributor

CI failures are being fixed by #10304

@krish2718 the Matter PR is now merged, you can rebase your branch.

And now the CI is failing due to too big code size (FLASH overflow).

@krish2718
Copy link
Contributor Author

CI failures are being fixed by #10304

@krish2718 the Matter PR is now merged, you can rebase your branch.

And now the CI is failing due to too big code size (FLASH overflow).

Thanks. This again seems to be Matter tests @markaj-nordic can you please check, Matter is OOM again?

@markaj-nordic
Copy link
Contributor

markaj-nordic commented Feb 17, 2023

CI failures are being fixed by #10304

@krish2718 the Matter PR is now merged, you can rebase your branch.

And now the CI is failing due to too big code size (FLASH overflow).

Thanks. This again seems to be Matter tests @markaj-nordic can you please check, Matter is OOM again?

I guess we cannot force the applications to select the ASSERT_VERBOSE or DEBUG. This needs to be relaxed (i.e. by using imply instead of select) to allow Matter samples to fit into the flash.

This enables the assert for minimum libc HEAP and fails boot, saves us
time from hard to debug issues due to low HEAP.

Signed-off-by: Krishna T <krishna.t@nordicsemi.no>
@krish2718
Copy link
Contributor Author

The CI is still failing even with imply because now matter has to disable them explicitly, two options,

  • Abandon this PR and find an alternative approach that doesn't enable the DEBUG and ASSERT for all
  • Matter should disable these explicitly to save spaces
    Ideas @rlubos @markaj-nordic

@markaj-nordic
Copy link
Contributor

The CI is still failing even with imply because now matter has to disable them explicitly, two options,

* Abandon this PR and find an alternative approach that doesn't enable the DEBUG and ASSERT for all

* Matter should disable these explicitly to save spaces
  Ideas @rlubos @markaj-nordic

I am wondering if it makes any sens to do it the other way around, i.e. having at least the CONFIG_DEBUG=n by default and only turn it on explicitly in NCS samples?

@LuDuda
Copy link
Contributor

LuDuda commented Feb 20, 2023

I would also recommend to set CONFIG_DEBUG only on the sample level - not on component level. Alternatively, maybe you can create a new KConfig like WPA_SUPP_DEBUG that is set in Wi-Fi samples and selects needed DEBUG functionalities.

Btw. I would pull inside WPA_SUPP_DEBUG also RESET_ON_FATAL_ERROR which in my opinion should not be configured on Wi-Fi component level.

@krish2718 krish2718 removed this from the 2.3.0 milestone Feb 20, 2023
@krish2718
Copy link
Contributor Author

ok, dropped 2.3.0 milestone and also raised a revert PR for hostapd, let me revisit this.

@krish2718 krish2718 closed this Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a known bug changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. DNM manifest manifest-sdk-hostap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants