testsuite: Add busy simulator #36662

nordic-krch · 2021-07-01T09:36:23Z

Busy simulator is using counter and entropy devices to randomly trigger counter interrupts and busy loop inside that interrupt for random period of time. Module is using work to produce random values which are stored in the ring buffer. Counter interrupt priority is set in overlay. ZLI will be also an option once #32575 is merged.

Module is indented to be used in testing. Already #36464 was discovered when it was used in the test in a downstream project.

anangl · 2021-07-07T09:48:08Z

dts/bindings/test/vnd,busy-sim.yaml

+compatible: "vnd,busy-sim"
+
+properties:
+    gpios:


What about naming it active-gpios?

anangl · 2021-07-07T13:02:36Z

subsys/testsuite/busy_sim/busy_sim.c

+	const struct device *entropy;
+	const struct device *counter;
+	const struct device *gpio_port;


These fields (and pin as well) can be initialized at compile time, so you could extract those to a separate const structure that could be placed in flash.

This should use a gpio_dt_spec, also. Optional properties like active-gpios can be initialized using GPIO_DT_SPEC_GET_OR().

This should use a gpio_dt_spec, also. Optional properties like active-gpios can be initialized using GPIO_DT_SPEC_GET_OR().

@nordic-krch did you see this one? Just checking, since you've been pushing other updates.

anangl · 2021-07-07T13:03:37Z

subsys/testsuite/busy_sim/busy_sim.c

+	sim.alarm_cfg.callback = counter_alarm_callback;
+	sim.alarm_cfg.flags = COUNTER_ALARM_CFG_EXPIRE_WHEN_LATE;
+
+	sim.entropy = device_get_binding(DT_CHOSEN_ZEPHYR_ENTROPY_LABEL);


You can initialize this pointer with DEVICE_DT_GET(DT_CHOSEN(zephyr_entropy)).

anangl · 2021-07-07T13:04:30Z

subsys/testsuite/include/busy_sim.h

+void busy_sim_start(uint32_t active_avg, uint32_t active_delta,
+			  uint32_t idle_avg, uint32_t idle_delta);


Some descriptions of these parameters would be helpful.

anangl · 2021-07-07T13:08:24Z

subsys/testsuite/busy_sim/busy_sim.c

+	uint32_t freq;
+
+	if (sim.gpio_port) {
+		gpio_pin_configure(sim.gpio_port, sim.pin,


sim.gpio_port should be checked with device_is_ready() first.

anangl · 2021-07-07T13:23:11Z

subsys/testsuite/busy_sim/busy_sim.c

+
+	if (sim.gpio_port) {
+		gpio_pin_configure(sim.gpio_port, sim.pin,
+				   GPIO_OUTPUT | GPIO_OUTPUT_INIT_LOW);


This way the flags specified in DT for this pin are ignored (what may be confusing). It would be better to use GPIO_DT_SPEC_GET() and gpio_pin_configure_dt() for handling the pin. See for instance the button sample:

zephyr/samples/basic/button/src/main.c

Lines 32 to 33 in bf3c6e7

static struct gpio_dt_spec led = GPIO_DT_SPEC_GET_OR(DT_ALIAS(led0), gpios,

{0});

zephyr/samples/basic/button/src/main.c

Lines 75 to 76 in bf3c6e7

if (led.port) {

ret = gpio_pin_configure_dt(&led, GPIO_OUTPUT);

.

anangl · 2021-07-07T13:24:01Z

subsys/testsuite/busy_sim/busy_sim.c

+#include <drivers/counter.h>
+#include <drivers/gpio.h>
+#include "busy_sim.h"
+#include <hal/nrf_gpio.h>


This is not needed.

anangl · 2021-07-07T13:32:26Z

subsys/testsuite/Kconfig

+	  It simulates cpu load by using counter device to generate random
+	  interrupts with random busy looping in the interrupt.


The term "random interrupts" is a bit ambiguous. I think something like "interrupts with random intervals" could make it more clear.

nordic-krch · 2021-07-12T07:13:02Z

@anangl thanks for review. I applied all your comments, can you re-review?

mbolivar-nordic · 2021-07-15T17:10:14Z

tests/ztest/busy_sim/boards/nrf52840dk_nrf52840_pin.overlay

+&timer0 {
+	status = "okay";
+	interrupts = <8 0>;
+	busy_sim: vnd-busy-sim {
+		compatible = "vnd,busy-sim";
+		active-gpios = <&gpio0 27 GPIO_ACTIVE_HIGH>;
+	};
+};


Can you please provide a justification for:

why you're using a parent/child relationship here instead of pointing at the timer instance from the vnd,busy-sim node using a phandle?

why only one device is supported via the busy_sim node label?

If you can support multiple busy simulator devices, I think this should use DT_INST_FOREACH_STATUS_OKAY to initialize them instead, with DT_DRV_COMPAT set to vnd_busy_sim.

If it only makes sense to support one vnd,busy-sim in the system, then why not just use DEVICE_DT_GET_ANY() and add a BUILD_ASSERT(DT_NUM_INST_STATUS_OKAY(vnd_busy_sim) == 1)?

why you're using a parent/child relationship here instead of pointing at the timer instance from the vnd,busy-sim node using a phandle?

I am not that fluent with device tree options. This one seemed ok to me as busy simulator is built on top of counter instance but i'm not attached to that so if you think it's better to use phandle i can change it. Could you provide justification why it's the way to go?

why only one device is supported via the busy_sim node label?

Since it's for testing purposes only i don't see a need for multiple simulators? Sorry but don't know where should i use DEVICE_DT_GET_ANY(). Can you be more specific?

Could you provide justification why it's the way to go?

In my experience, parent/child is for controller / peripheral type relationships (compare with SPI, I2C, and flash).

Phandles seem to be used more when you want to say "use peripheral X for this": compare with properties like foo-gpios, dmas, io-channels, etc.

Sorry but don't know where should i use DEVICE_DT_GET_ANY(). Can you be more specific?

Sure; what I mean is that you don't need to force the user to make a node label, even if only one device is supported in the system.

You can do something like this instead (after thinking about it, I think DEVICE_DET_GET_ONE is better than DEVICE_DT_GET_ANY):

BUILD_ASSERT(DT_NUM_INST_STATUS_OKAY(vnd_busy_sim) == 1, "add exactly one vnd,busy-sim node to the devicetree"); const struct device *my_busy_sim = DEVICE_DT_GET_ONE(vnd_busy_sim); if (!device_is_ready(my_busy_sim)) { /* ... handle error here ... */ } else { /* device is good to go */ }

Then your node doesn't need to be labeled busy_sim in the DTS. You can just make a status okay node with the right compatible and everything just works.

I've converted busy_sim to the device and used phandle for counter. Implemented it as separate commit, will squash if it's ok.

mbolivar-nordic

Adding a temporary -1 for now until the DT descriptions are sorted out. I think the overall idea makes sense though.

mbolivar-nordic · 2021-07-20T17:29:14Z

subsys/testsuite/busy_sim/busy_sim.c

+	.gpio_port = DEVICE_DT_GET(DT_GPIO_CTLR(DT_BUSY_SIM, active_gpios)),
+	.pin = DT_GPIO_PIN(DT_BUSY_SIM, active_gpios),
+	.pin_spec = GPIO_DT_SPEC_GET_OR(DT_BUSY_SIM, active_gpios, {0}),


This is weird. The pin_spec structure already has the port and pin, so this is redundant.

removed gpio-port and pin.

mbolivar-nordic · 2021-07-22T00:56:59Z

Could you please squash things together to make this easier to review?

nordic-krch · 2021-07-22T13:14:53Z

Could you please squash

@mbolivar-nordic done

mbolivar-nordic

Thanks for squashing. Looking very good to me now, just a couple of minor issues.

subsys/testsuite/busy_sim/busy_sim.c

tests/ztest/busy_sim/boards/nrf52840dk_nrf52840.overlay

tests/ztest/busy_sim/boards/nrf52840dk_nrf52840_pin.overlay

tests/ztest/busy_sim/src/main.c

Busy simulator is using counter device and entropy device to generate random cpu load. Counter device cofiguration can be used to set cpu load interrupt priority and optional pin that can be set during the load. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

Added test that validates busy simulator. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

mbolivar-nordic

Thanks! LGTM.

mbolivar-nordic · 2021-07-28T23:56:35Z

nordic-krch added the DNM label 9 days ago

@nordic-krch did you mean to do this, or was it an accident?

nordic-krch · 2021-07-29T04:34:27Z

@nordic-krch did you mean to do this, or was it an accident?

Removed, it was added when commits were not squashed.

nordic-krch · 2021-07-29T04:35:04Z

@anangl can you take a look?

nordic-krch requested review from galak and nashif as code owners July 1, 2021 09:36

nordic-krch requested review from anangl and ioannisg July 1, 2021 09:36

github-actions bot added area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Test Framework Issues related not to a particular test, but to the framework instead area: Tests Issues related to a particular existing or missing test labels Jul 1, 2021

nordic-krch force-pushed the busy_sim branch from bc46cf9 to aa901d4 Compare July 1, 2021 12:36

zephyrbot added the area: Testsuite Testsuite label Jul 6, 2021

zephyrbot requested a review from mbolivar-nordic July 6, 2021 13:53

zephyrbot assigned nashif Jul 6, 2021

nashif approved these changes Jul 7, 2021

View reviewed changes

anangl reviewed Jul 7, 2021

View reviewed changes

nordic-krch force-pushed the busy_sim branch from aa901d4 to ddad165 Compare July 12, 2021 07:12

mbolivar-nordic reviewed Jul 15, 2021

View reviewed changes

mbolivar-nordic suggested changes Jul 15, 2021

View reviewed changes

nordic-krch force-pushed the busy_sim branch 2 times, most recently from d94e95b to 01c50ba Compare July 20, 2021 07:11

nordic-krch added the DNM This PR should not be merged (Do Not Merge) label Jul 20, 2021

mbolivar-nordic reviewed Jul 20, 2021

View reviewed changes

nordic-krch force-pushed the busy_sim branch from 01c50ba to 03cd918 Compare July 21, 2021 05:36

nordic-krch force-pushed the busy_sim branch from 03cd918 to 79ea80d Compare July 22, 2021 13:14

mbolivar-nordic suggested changes Jul 22, 2021

View reviewed changes

nordic-krch added 2 commits July 23, 2021 06:14

tests: ztest: Add test for busy simulator

3a5a77f

Added test that validates busy simulator. Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>

nordic-krch force-pushed the busy_sim branch from 79ea80d to 3a5a77f Compare July 23, 2021 04:14

nordic-krch requested a review from mbolivar-nordic July 23, 2021 04:15

mbolivar-nordic approved these changes Jul 28, 2021

View reviewed changes

nordic-krch removed the DNM This PR should not be merged (Do Not Merge) label Jul 29, 2021

anangl approved these changes Jul 29, 2021

View reviewed changes

cfriedt merged commit 7423f5f into zephyrproject-rtos:main Jul 29, 2021

nordic-krch mentioned this pull request Oct 25, 2021

drivers: watchdog: Add software watchdog based on counter #39659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testsuite: Add busy simulator #36662

testsuite: Add busy simulator #36662

nordic-krch commented Jul 1, 2021

anangl Jul 7, 2021

anangl Jul 7, 2021

mbolivar-nordic Jul 15, 2021

mbolivar-nordic Jul 20, 2021

anangl Jul 7, 2021

anangl Jul 7, 2021

anangl Jul 7, 2021

anangl Jul 7, 2021

anangl Jul 7, 2021

anangl Jul 7, 2021

nordic-krch commented Jul 12, 2021

mbolivar-nordic Jul 15, 2021

nordic-krch Jul 19, 2021

mbolivar-nordic Jul 19, 2021

nordic-krch Jul 20, 2021

mbolivar-nordic left a comment

mbolivar-nordic Jul 20, 2021

nordic-krch Jul 21, 2021

mbolivar-nordic commented Jul 22, 2021

nordic-krch commented Jul 22, 2021

mbolivar-nordic left a comment

mbolivar-nordic left a comment

mbolivar-nordic commented Jul 28, 2021

nordic-krch commented Jul 29, 2021

nordic-krch commented Jul 29, 2021

		void busy_sim_start(uint32_t active_avg, uint32_t active_delta,
		uint32_t idle_avg, uint32_t idle_delta);

	static struct gpio_dt_spec led = GPIO_DT_SPEC_GET_OR(DT_ALIAS(led0), gpios,
	{0});

	if (led.port) {
	ret = gpio_pin_configure_dt(&led, GPIO_OUTPUT);

		It simulates cpu load by using counter device to generate random
		interrupts with random busy looping in the interrupt.

testsuite: Add busy simulator #36662

testsuite: Add busy simulator #36662

Conversation

nordic-krch commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar-nordic commented Jul 22, 2021

nordic-krch commented Jul 22, 2021

mbolivar-nordic left a comment

Choose a reason for hiding this comment

mbolivar-nordic left a comment

Choose a reason for hiding this comment

mbolivar-nordic commented Jul 28, 2021

nordic-krch commented Jul 29, 2021

nordic-krch commented Jul 29, 2021