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

fix tests scenarios failing with undefined reference to z_impl_sys_rand_get #82346

Closed
wants to merge 2 commits into from

Conversation

manuargue
Copy link
Member

@manuargue manuargue commented Nov 29, 2024

Some tests are failing with undefined reference to z_impl_sys_rand_get for boards hexiwear/mk64f12 and frdm_k64f/mk64f12. It looks like these boards lack of RNG driver and and the test scenarios are not enabling any test RNG, so enable it.

Not sure about side effects, so I'm open to better suggestions -I'm just trying to unblock unrelated CI failures in another pr.

Fixes #82344

Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

Not sure about this change.

The documentation for the option states:

	  This option is intended to be selected only by application-level
	  configurations (e.g. in tests and samples) to indicate that the
	  application is allowed to run with a random number generator that is not
	  truly random. Board-level configurations must not select this option
	  unless the sole purpose of the board is testing (e.g. QEMU emulation
	  boards).

Not sure if we can consider these cases here tests.

Furthermore, the option is hardened, which will give a build warning. That doesn't like something we really want to have in samples.

An alternative to this PR is to remove the boards that do not support a random generator for applications that require a random generator.

Enable entropy generator as needed by these samples.

Fixes zephyrproject-rtos#82344

Signed-off-by: Manuel Argüelles <[email protected]>
Enable entropy generator as needed by these tests.

Fixes zephyrproject-rtos#82344

Signed-off-by: Manuel Argüelles <[email protected]>
@manuargue
Copy link
Member Author

An alternative to this PR is to remove the boards that do not support a random generator for applications that require a random generator.

my bad, it seems that is possible to enable CONFIG_ENTROPY_GENERATOR=y for these SoCs -which I'm not familiar with :)

@Thalley
Copy link
Collaborator

Thalley commented Nov 29, 2024

An alternative to this PR is to remove the boards that do not support a random generator for applications that require a random generator.

my bad, it seems that is possible to enable CONFIG_ENTROPY_GENERATOR=y for these SoCs -which I'm not familiar with :)

Doing a search for CONFIG_ENTROPY_GENERATOR=y show that it's more common to enable this in the prj.conf files, rather than in board files.

I'm not sure which way is "correct"

@manuargue
Copy link
Member Author

manuargue commented Nov 29, 2024

Doing a search for CONFIG_ENTROPY_GENERATOR=y show that it's more common to enable this in the prj.conf files, rather than in board files.

I based on https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/bluetooth/peripheral_ht/boards/frdm_rw612.conf#L2. I'm not familiar enough with the tests to enable it at prj.conf, so I'll let others with better knowledge than me on this to comment.

@alwa-nordic alwa-nordic removed their assignment Nov 29, 2024
@jhedberg
Copy link
Member

I'd assume it's the enabling of Bluetooth that brings this dependency. It might make more sense to do this as a select statement in the HCI driver that this board uses, similar to what was done for ESP32 here: #82198

@manuargue
Copy link
Member Author

I'd assume it's the enabling of Bluetooth that brings this dependency. It might make more sense to do this as a select statement in the HCI driver that this board uses, similar to what was done for ESP32 here: #82198

It's using the BT_H4/zephyr,bt-hci-uart generic driver, not an specific NXP implementation.

I see from https://github.com/zephyrproject-rtos/zephyr/pull/79931/files three other different approaches to this same problem:

  1. Select CONFIG_ENTROPY_GENERATOR=y from test/sample prj.conf
  2. Select CONFIG_ENTROPY_GENERATOR=y from the board's sample/test conf overlay
  3. Set CONFIG_ENTROPY_GENERATOR default to y in the board defconfig when BT=y

If the dependency comes from the BT_H4 driver, then I agree with you to select it from there. Otherwise, (3) may be a better fit.

@jhedberg
Copy link
Member

I need to take a look of it's some subset of Bluetooth functionality (or subset boards with Bluetooth) that need this, or if it's always needed. If it's the latter, maybe we should just add the select to config BT.

@jhedberg
Copy link
Member

Right now this seems to be fixing this on a per-app basis, which would then leave all other Bluetooth apps broken with this board. So out of the various alternatives being discussed that's probably the worst option.

@jhedberg
Copy link
Member

Could you try if the same issue gets fixed by #82363? If so, then we should probably move to that instead of selecting ENTROPY_GENERATOR separately for each board or HCI driver.

@manuargue
Copy link
Member Author

Could you try if the same issue gets fixed by #82363? If so, then we should probably move to that instead of selecting ENTROPY_GENERATOR separately for each board or HCI driver.

@jhedberg agree, it fixes the issue indeed, thanks! Btw you may want to also cleanup all the tests/samples overlays that are selecting ENTROPY_GENERATOR for specific boards after your change.

@manuargue
Copy link
Member Author

moved to #82363

@manuargue manuargue closed this Dec 1, 2024
@manuargue manuargue deleted the nxp-fix-82344 branch December 1, 2024 03:44
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.

mk64f12: test failing with undefined reference to 'z_impl_sys_rand_get'
6 participants