tests: drivers: can: api: Extend can api test #80063

nordic-segl · 2024-10-18T13:22:10Z

(Removed as it doesn't restore CAN configuration fully.)
// tests: drivers: can: api: Set default CAN state before each test
// Tests assume that CAN is started.
// When previous test stops CAN and fails on assertion then remaining tests may fail.
// Start CAN before every tests. Ignore error code returned when starting CAN that is already started.

tests: drivers: can: api: Add negative test for can_send()
Check error codes when sending invalid frames:

too big data payload;
wrong set of flags.

(Removed as there are dependencies to CAN core clock frequency and timing setting.)
// tests: drivers: can: api: Check value returned by can_get_bitrate_max()
// Function can_get_bitrate_max() shall return maximum supported bitrate for the CAN controller/transceiver combination.
// Check if that bitrate can be set with can_set_bitrate_data().

(Removed as there are dependencies to CAN core clock frequency and timing setting.)
// tests: drivers: can: api: Check value returned by can_get_bitrate_min()
// Function can_get_bitrate_min() shall return minimum supported bitrate for the CAN controller/transceiver combination.
// Check if that bitrate can be set with can_set_bitrate().

tests: drivers: can: api: Add negative test for can_set_bitrate_data()
There is negative test for too high data bitrate.
Add test that checks too low data bitrate.

tests: drivers: can: api: Add negative test for can_set_bitrate()
There is negative test for too high bitrate.
Add test that checks too low bitrate.

tests: drivers: can: api: Add negative test for can_add_rx_filter()
Check that error is reported when CAN filter is added without callback function.

nordic-segl · 2024-10-18T13:22:32Z

This PR relates to #76298

henrikbrixandersen · 2024-10-18T15:09:49Z

What does "Nrfx 6088" mean?

nordic-segl · 2024-10-18T21:21:56Z

What does "Nrfx 6088" mean?

"NRFX-6088" is a task number in our JIRA.

nordic-segl · 2024-10-18T21:34:22Z

What does bitrate of 0 bit/s mean?

I see, vale of 0 is returned by can_get_bitrate_min()
"Get the minimum supported bitrate for the CAN controller/transceiver combination."
https://docs.zephyrproject.org/latest/doxygen/html/group__can__interface.html#ga343456749eec6144a91bacae0d94b114

However, when I try to set such bitrate, I get -EINVAL.

henrikbrixandersen · 2024-10-18T21:47:15Z

What does bitrate of 0 bit/s mean?

It means the CAN controller has no documented lower bitrate limit.

I see, vale of 0 is returned by can_get_bitrate_min()

"Get the minimum supported bitrate for the CAN controller/transceiver combination."

https://docs.zephyrproject.org/latest/doxygen/html/group__can__interface.html#ga343456749eec6144a91bacae0d94b114

However, when I try to set such bitrate, I get -EINVAL.

Questions like this make uncomfortable accepting any CAN related code changes from you. How would you expect a bitrate of 0 bits/second to be acceptable?

nordic-segl · 2024-10-21T07:04:24Z

tests/drivers/can/api/src/canfd.c

+/**
+ * @brief Test sending CAN FD frame with too big payload.
+ */
+ZTEST(canfd, test_send_fd_dlc_out_of_range)


There is already negative test that checks error code returned by can_send() when CAN frame ID is invalid:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/classic.c#L799
Here, I add similar test. It checks that can_send() returns -EINVAL when CAN frame has invalid DLC value.
Test variant for CAN FD.

nordic-segl · 2024-10-21T07:05:44Z

tests/drivers/can/api/src/classic.c

+/**
+ * @brief Test sending standard (11-bit ID) CAN frame with too big payload.
+ */
+ZTEST(can_classic, test_send_std_id_dlc_of_range)


There is already negative test that checks error code returned by can_send() when CAN frame ID is invalid:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/classic.c#L799
Here, I add similar test. It checks that can_send() returns -EINVAL when CAN frame has invalid DLC value.
Test variant for "standard" CAN with 11-bit CAN frame ID.

nordic-segl · 2024-10-21T07:06:02Z

tests/drivers/can/api/src/classic.c

+/**
+ * @brief Test sending extended (29-bit ID) CAN frame with too big payload.
+ */
+ZTEST(can_classic, test_send_ext_id_dlc_of_range)


There is already negative test that checks error code returned by can_send() when CAN frame ID is invalid:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/classic.c#L799
Here, I add similar test. It checks that can_send() returns -EINVAL when CAN frame has invalid DLC value.
Test variant for "standard" CAN with 29-bit CAN frame ID.

nordic-segl · 2024-10-21T07:07:29Z

tests/drivers/can/api/src/canfd.c

+ * CAN FD Error State Indicator (ESI) indicates that the transmitting node is
+ * in error-passive state. Only valid in combination with CAN_FRAME_FDF.
+ */
+ZTEST(canfd, test_send_fd_incorrect_esi)


There is already negative test that checks error code returned by can_send() when CAN frame ID is invalid:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/classic.c#L799
Here, I add similar test. It checks that can_send() returns -EINVAL when CAN frame has invalid set of flags (explanation in lines 262-263, copied from API description).

nordic-segl · 2024-10-21T07:10:28Z

tests/drivers/can/api/src/canfd.c

+/**
+ * @brief Test setting a too low data phase bitrate.
+ */
+ZTEST_USER(canfd, test_set_bitrate_data_too_low)


There is already negative test that checks error code returned by can_set_bitrate_data() when bitrate value is too high:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/canfd.c#L401
Here, I add similar test. It checks that can_set_bitrate_data() returns -ENOTSUP when bitrate value is too low.

tests/drivers/can/api/src/canfd.c

nordic-segl · 2024-10-21T07:14:53Z

tests/drivers/can/api/src/classic.c

+/**
+ * @brief Test setting a too low bitrate.
+ */
+ZTEST_USER(can_classic, test_set_bitrate_too_low)


There is already negative test that checks error code returned by can_set_bitrate() when bitrate value is too high:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/classic.c#L492
Here, I add similar test. It checks that can_set_bitrate() returns -ENOTSUP when bitrate value is too low.

tests/drivers/can/api/src/classic.c

nordic-segl · 2024-10-21T07:21:46Z

tests/drivers/can/api/src/classic.c

+/**
+ * @brief Test adding filter without callback.
+ */
+ZTEST(can_classic, test_add_filter_without_callback)


There is a set of negative tests which check that can_add_rx_filter() returns -EINVAL when filter is incorrect:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/can/api/src/classic.c#L637
Here, I check that error code is returned when user tries to set filter without callback function.

nordic-segl · 2024-10-21T07:27:09Z

tests/drivers/can/api/src/common.c

@@ -244,3 +244,13 @@ void can_common_test_setup(can_mode_t initial_mode)
 	err = can_start(can_dev);
 	zassert_equal(err, 0, "failed to start CAN controller (err %d)", err);
 }
+
+void before_test(void *not_used)


This is improvement.
Test cases assume that CAN is started when test begins.
There are test that stop CAN, do testing, re-start CAN.
If such test fails on assertion then test execution is terminated. As a result, CAN is not restarted and precondition (CAN started) is not meet for remaining tests.
Here, I add code that is executed before each test. I ignore error code
-EALREADY if the device is already started.
https://docs.zephyrproject.org/latest/doxygen/html/group__can__interface.html#ga343456749eec6144a91bacae0d94b114

Please remove this part. This is not sufficient for restoring the CAN controller to a default state in all failure scenarios (e.g. installed filters, but test case failed) and as such provide limited value.

If needed, we can move this to a separate PR/issue.

"This is not sufficient for restoring the CAN controller to a default state (...)"
You are right, "test case setup function" doesn't restore other CAN parameters such as installed filters, bitrate, timing, etc.
Removing.

nordic-segl · 2024-10-21T07:34:13Z

What does bitrate of 0 bit/s mean?

It means the CAN controller has no documented lower bitrate limit.

It is undocumented feature.

I see, vale of 0 is returned by can_get_bitrate_min()
"Get the minimum supported bitrate for the CAN controller/transceiver combination."
https://docs.zephyrproject.org/latest/doxygen/html/group__can__interface.html#ga343456749eec6144a91bacae0d94b114
However, when I try to set such bitrate, I get -EINVAL.

Questions like this make uncomfortable accepting any CAN related code changes from you. How would you expect a bitrate of 0 bits/second to be acceptable?

I consider this a personal attack. I will consult my supervisor on how to proceed.

henrikbrixandersen · 2024-10-21T08:34:00Z

What does bitrate of 0 bit/s mean?

It means the CAN controller has no documented lower bitrate limit.

It is undocumented feature.

Right, let's get that added to the doxygen for can_get_bitrate_min(). I'll open a PR (#80127).

I see, vale of 0 is returned by can_get_bitrate_min()
"Get the minimum supported bitrate for the CAN controller/transceiver combination."
docs.zephyrproject.org/latest/doxygen/html/group__can__interface.html#ga343456749eec6144a91bacae0d94b114
However, when I try to set such bitrate, I get -EINVAL.

Questions like this make uncomfortable accepting any CAN related code changes from you. How would you expect a bitrate of 0 bits/second to be acceptable?

I consider this a personal attack. I will consult my supervisor on how to proceed.

I'm sorry, that was not the intention, but you are opening PRs for adding CAN subsystem API-level tests, yet comments/questions like the above makes me unsure if you have the insight into the CAN subsystem needed to do so.

henrikbrixandersen · 2024-10-22T07:46:27Z

tests/drivers/can/api/src/common.c

@@ -244,3 +244,13 @@ void can_common_test_setup(can_mode_t initial_mode)
 	err = can_start(can_dev);
 	zassert_equal(err, 0, "failed to start CAN controller (err %d)", err);
 }
+
+void before_test(void *not_used)


Please remove this part. This is not sufficient for restoring the CAN controller to a default state in all failure scenarios (e.g. installed filters, but test case failed) and as such provide limited value.

If needed, we can move this to a separate PR/issue.

Check that error is reported when CAN filter is added without callback function. Signed-off-by: Sebastian Głąb <[email protected]>

There is negative test for too high bitrate. Add test that checks too low bitrate. Signed-off-by: Sebastian Głąb <[email protected]>

There is negative test for too high data bitrate. Add test that checks too low data bitrate. Signed-off-by: Sebastian Głąb <[email protected]>

Check error codes when sending invalid frames: - too big data payload; - wrong set of flags. Signed-off-by: Sebastian Głąb <[email protected]>

Bring in additional CAN tests: nrfconnect/sdk-zephyr#2151 zephyrproject-rtos/zephyr#80063 Signed-off-by: Sebastian Głąb <[email protected]>

henrikbrixandersen · 2024-10-24T14:14:51Z

Fallout from misunderstanding the CAN_FRAME_ESI flag fixed here: #80380

nordic-segl · 2024-10-25T06:29:25Z

Fallout from misunderstanding the CAN_FRAME_ESI flag fixed here: #80380

"Fix the test for sending frames with the CAN_FRAME_ESI flag set. Sending frames with this flag set in software is never allowed."
"The test introduced in 8023a58 is wrong and holding back both local pre-v4.0.0-rc1 testing and PRs targeting v4.0.0-rc1."

That proves what You stated - I don't have sufficient knowledge to work on task "NRFX-6088: Extend testing for CAN".
I asked to assign that task to somebody else.
I apologise for the inconvenience.

nordic-segl · 2024-10-25T07:17:36Z

From the other side:
API states that can_send() shall return
"-ENOTSUP if an unsupported parameter was passed to the function. "
https://docs.zephyrproject.org/apidoc/latest/group__can__interface.html#ga446ee31913699de3c80be05d837b5fd5
You wrote "Sending frames with this flag set in software is never allowed."

IMHO, there should be more tests:
Scenario 1:
What I proposed, CONFIG_CAN_FD_MODE=y, and can_set_mode(CAN_MODE_FD) and .flags = CAN_FRAME_ESI (as this caused failure in #76134 -> driver was not detecting invalid use)
Scenario 2:
What You proposed, CONFIG_CAN_FD_MODE=y, and can_set_mode(CAN_MODE_FD) and .flags = CAN_FRAME_FDF | CAN_FRAME_ESI (as this would cause failure in fix for fail in scenario 1 in #76134 -> still, invalid use)
Correct check for scenarios 1 and 2 is implemented here: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/can/can_mcan.c#L936

Scenario 3 in "classic CAN":
CONFIG_CAN_FD_MODE=n, and .flags = CAN_FRAME_ESI
Scenario 4 in "classic can":
CONFIG_CAN_FD_MODE=n, and .flags = CAN_FRAME_FDF | CAN_FRAME_ESI
Correct check for scenarios 3 and 4 is implemented here: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/can/can_mcan.c#L948

Bring in additional CAN tests: nrfconnect/sdk-zephyr#2151 zephyrproject-rtos/zephyr#80063 Signed-off-by: Sebastian Głąb <[email protected]>

zephyrbot added the area: CAN label Oct 18, 2024

zephyrbot requested review from alexanderwachter, henrikbrixandersen, martinjaeger and str4t0m October 18, 2024 13:22

zephyrbot assigned henrikbrixandersen Oct 18, 2024

nordic-segl force-pushed the NRFX-6088_Extend_CAN_API_test branch from 42b4258 to b9786a6 Compare October 18, 2024 13:27

nordic-segl changed the title ~~Nrfx 6088 extend can api test~~ tests: drivers: can: api: Extend can api test Oct 18, 2024

nordic-segl commented Oct 21, 2024

View reviewed changes

tests/drivers/can/api/src/canfd.c Outdated Show resolved Hide resolved

nordic-segl commented Oct 21, 2024

View reviewed changes

tests/drivers/can/api/src/classic.c Outdated Show resolved Hide resolved

nordic-segl commented Oct 21, 2024

View reviewed changes

nordic-segl force-pushed the NRFX-6088_Extend_CAN_API_test branch from b9786a6 to 9f6aeea Compare October 22, 2024 06:14

henrikbrixandersen requested changes Oct 22, 2024

View reviewed changes

nordic-segl force-pushed the NRFX-6088_Extend_CAN_API_test branch from 9f6aeea to dd20708 Compare October 22, 2024 07:56

nordic-segl added 2 commits October 22, 2024 10:47

tests: drivers: can: api: Add negative test for can_add_rx_filter()

7a3a1d0

Check that error is reported when CAN filter is added without callback function. Signed-off-by: Sebastian Głąb <[email protected]>

tests: drivers: can: api: Add negative test for can_set_bitrate()

13c80c4

There is negative test for too high bitrate. Add test that checks too low bitrate. Signed-off-by: Sebastian Głąb <[email protected]>

nordic-segl added 2 commits October 22, 2024 10:47

tests: drivers: can: api: Add negative test for can_set_bitrate_data()

03755b9

There is negative test for too high data bitrate. Add test that checks too low data bitrate. Signed-off-by: Sebastian Głąb <[email protected]>

tests: drivers: can: api: Add negative test for can_send()

801d7e4

Check error codes when sending invalid frames: - too big data payload; - wrong set of flags. Signed-off-by: Sebastian Głąb <[email protected]>

nordic-segl force-pushed the NRFX-6088_Extend_CAN_API_test branch from dd20708 to 801d7e4 Compare October 22, 2024 08:47

henrikbrixandersen approved these changes Oct 22, 2024

View reviewed changes

gmarull approved these changes Oct 23, 2024

View reviewed changes

aescolar merged commit 8023a58 into zephyrproject-rtos:main Oct 23, 2024
18 checks passed

nordic-segl deleted the NRFX-6088_Extend_CAN_API_test branch October 23, 2024 09:53

nordic-segl added a commit to nordic-segl/sdk-nrf that referenced this pull request Oct 23, 2024

manifest: Update sdk-zephyr

2b6de4b

Bring in additional CAN tests: nrfconnect/sdk-zephyr#2151 zephyrproject-rtos/zephyr#80063 Signed-off-by: Sebastian Głąb <[email protected]>

nordic-segl mentioned this pull request Oct 23, 2024

manifest: Update sdk-zephyr nrfconnect/sdk-nrf#18119

Closed

nordic-segl added a commit to nordic-segl/sdk-nrf that referenced this pull request Oct 24, 2024

manifest: Update sdk-zephyr

99643bd

Bring in additional CAN tests: nrfconnect/sdk-zephyr#2151 zephyrproject-rtos/zephyr#80063 Signed-off-by: Sebastian Głąb <[email protected]>

nordic-segl added a commit to nordic-segl/sdk-nrf that referenced this pull request Oct 29, 2024

manifest: Update sdk-zephyr

78ab238

Bring in additional CAN tests: nrfconnect/sdk-zephyr#2151 zephyrproject-rtos/zephyr#80063 Signed-off-by: Sebastian Głąb <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: drivers: can: api: Extend can api test #80063

tests: drivers: can: api: Extend can api test #80063

nordic-segl commented Oct 18, 2024 •

edited

Loading

nordic-segl commented Oct 18, 2024

henrikbrixandersen commented Oct 18, 2024

nordic-segl commented Oct 18, 2024

nordic-segl commented Oct 18, 2024

henrikbrixandersen commented Oct 18, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

nordic-segl Oct 21, 2024

henrikbrixandersen Oct 22, 2024 •

edited

Loading

nordic-segl Oct 22, 2024

nordic-segl commented Oct 21, 2024

henrikbrixandersen commented Oct 21, 2024 •

edited

Loading

henrikbrixandersen Oct 22, 2024 •

edited

Loading

henrikbrixandersen commented Oct 24, 2024

nordic-segl commented Oct 25, 2024

nordic-segl commented Oct 25, 2024 •

edited

Loading

tests: drivers: can: api: Extend can api test #80063

tests: drivers: can: api: Extend can api test #80063

Conversation

nordic-segl commented Oct 18, 2024 • edited Loading

nordic-segl commented Oct 18, 2024

henrikbrixandersen commented Oct 18, 2024

nordic-segl commented Oct 18, 2024

nordic-segl commented Oct 18, 2024

henrikbrixandersen commented Oct 18, 2024

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

henrikbrixandersen Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-segl commented Oct 21, 2024

henrikbrixandersen commented Oct 21, 2024 • edited Loading

henrikbrixandersen Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

henrikbrixandersen commented Oct 24, 2024

nordic-segl commented Oct 25, 2024

nordic-segl commented Oct 25, 2024 • edited Loading

nordic-segl commented Oct 18, 2024 •

edited

Loading

henrikbrixandersen Oct 22, 2024 •

edited

Loading

henrikbrixandersen commented Oct 21, 2024 •

edited

Loading

henrikbrixandersen Oct 22, 2024 •

edited

Loading

nordic-segl commented Oct 25, 2024 •

edited

Loading