can: isotp: Add CAN-FD support to ISO-TP #60323

gramsay0 · 2023-07-13T05:30:22Z

Add CAN-FD support to ISO-TP
Update ISO-TP sample to work with CAN-FD
Update existing ISO-TP unit tests to work with CAN-FD
Add CAN-FD specific unit tests
- Mandatory padding of frames > 8 bytes to next DLC
- Validation that RX_DL in consecutive frames matches first frame

Thanks @dale-herman for doing most of the work

alexanderwachter · 2023-07-14T04:54:11Z

Just scrolled over it. Looks good at a first glance! You may extend the copyright notice, because it is a bigger extension of functionality.

gramsay0 · 2023-07-18T21:31:07Z

Just scrolled over it. Looks good at a first glance! You may extend the copyright notice, because it is a bigger extension of functionality.

@alexanderwachter thanks!

If you have time, could you please have a look over this RFC #59896 and give some feedback?
Apologies for not tagging you in it sooner, I would appreciate your input

tests/subsys/canbus/isotp/conformance/testcase.yaml

martinjaeger

Thanks for the contribution, only got some minor comments.

May I ask you to move the changes related to CONFIG_ISOTP_ENABLE_TX_PADDING and CONFIG_ISOTP_REQUIRE_RX_PADDING into a dedicated commit? As far as I can see they are not related to CAN-FD support. This will make it easier to understand why the change was made in the future.

samples/subsys/canbus/isotp/src/main.c

samples/subsys/canbus/isotp/Kconfig

tests/subsys/canbus/isotp/conformance/testcase.yaml

subsys/canbus/isotp/isotp.c

gramsay0 · 2023-07-20T04:38:08Z

May I ask you to move the changes related to CONFIG_ISOTP_ENABLE_TX_PADDING and CONFIG_ISOTP_REQUIRE_RX_PADDING into a dedicated commit? As far as I can see they are not related to CAN-FD support. This will make it easier to understand why the change was made in the future.

@martinjaeger Okay, will do

gramsay0 · 2023-07-21T02:33:01Z

@martinjaeger here is the padding fix PR #60661
I'll rebase this PR once that is merged

henrikbrixandersen

Is ISO-TP via CAN-FD part of ISO 15765-2?

subsys/canbus/isotp/Kconfig

martinjaeger · 2023-08-07T12:39:35Z

Is ISO-TP via CAN-FD part of ISO 15765-2?

Yes, it's part of the third edition of the spec (from 2016).

martinjaeger · 2023-08-22T10:50:38Z

Please ignore the first two commits. The will be removed through rebasing after #61732 is merged.

henrikbrixandersen

This looks good, @martinjaeger. I'd prefer if the last commit could be squashed into the previous commits to avoid the odd history, though.

henrikbrixandersen · 2023-08-22T12:04:07Z

tests/subsys/canbus/isotp/conformance/src/main.c

@@ -1106,7 +1124,7 @@ void *isotp_conformance_setup(void)
 	zassert_true(device_is_ready(can_dev), "CAN device not ready");

 	ret = can_set_mode(can_dev, CAN_MODE_LOOPBACK |
-			   (IS_ENABLED(CONFIG_ISOTP_USE_CAN_FD) ? CAN_MODE_FD : 0));
+			   (IS_ENABLED(CONFIG_CAN_FD_MODE) ? CAN_MODE_FD : 0));


I think this needs to fallback to classic CAN if the CAN controller does not support CAN-FD.

In that case IS_ENABLED(CONFIG_CAN_FD_MODE) should evaluate to 0 and FD would not be enabled.

Or do you have a case in mind where we'd have one controller with FD capabilities and one without on the same board/SOC, so that we have CONFIG_CAN_FD_MODE=y even though one controller doesn't support it?

CONFIG_CAN_FD_MODE can be enabled whether the CAN controller supports CAN-FD or not.

Yeah, that's exactly how the test is run at the moment, e.g. with native_posix. The first case is with default setup CONFIG_CAN_FD_MODE=n (even though it would be supported) and the two other cases with extra config CONFIG_CAN_FD_MODE=y and some different max data length settings.

Or maybe I misunderstand your question/request?

The tests need to be able to run on any CAN controller, regardless of CONFIG_CAN_FD_MODE being y or n and whether the CAN controller driver supports CAN-FD or not.

I'd suggest using can_get_capabilities() to determine if CAN_MODE_FD should be passed to can_set_mode() . That does not require any #ifdef on CONFIG_CAN_FD_MODE.

It was slightly more complicated than just checking with can_get_capabilities and applying the mode accordingly. Now we have the existing main test suite, which is skipped if FD is enabled for a controller that does not have FD capabilities. To check that correct errors are returned in such cases, I added another test suite canfd_notsup.c.

martinjaeger · 2023-08-22T13:12:54Z

This looks good, @martinjaeger. I'd prefer if the last commit could be squashed into the previous commits to avoid the odd history, though.

I added a new commit so that the authorship of the different changes is clear. But I'm fine with squashing it if @gramsay0 agrees.

subsys/canbus/isotp/isotp.c

gramsay0

Good work!

include/zephyr/canbus/isotp.h

subsys/canbus/isotp/isotp.c

tests/subsys/canbus/isotp/conformance/src/main.c

ISO-TP CAN-FD support can be enabled at runtime. Signed-off-by: Grant Ramsay <gramsay@enphaseenergy.com> Signed-off-by: Martin Jäger <martin@libre.solar>

Enable this sample to be used with CAN-FD Signed-off-by: Grant Ramsay <gramsay@enphaseenergy.com>

Allow existing tests to run with CAN-FD. Add new CAN-FD specific tests. Signed-off-by: Grant Ramsay <gramsay@enphaseenergy.com> Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger · 2023-09-07T15:28:41Z

I hope that all review comments are addressed now and unrelated CI failures are gone.

The ISO-TP test suites and sample were successfully tested on the following boards:

native_posix
nucleo_g474re (CAN-FD support)
nucleo_f072rb (classical CAN only)

henrikbrixandersen

Looks good. A couple of minor observations:

tests/subsys/canbus/isotp/conformance/src/mode_check.c

henrikbrixandersen · 2023-09-12T19:19:37Z

tests/subsys/canbus/isotp/conformance/testcase.yaml

+# | Case # | Controller type        | Selected mode        |  Example board |
+# +--------+------------------------+----------------------+----------------+
+# |    1   | Classical CAN only     | CONFIG_CAN_FD_MODE=n | nucleo_f072    |
+# |    2   | Classical CAN only     | CONFIG_CAN_FD_MODE=y | nucleo_f072    |
+# |    3   | Classical CAN + CAN-FD | CONFIG_CAN_FD_MODE=n | native_posix   |
+# |    4   | Classical CAN + CAN-FD | CONFIG_CAN_FD_MODE=y | native_posix   |


These cases significantly increase the time required for running the ISO-TP conformance test suite on real hardware. Can the tests be optimized in time somehow?

Example of time spent running this test suite on five random boards on my desk:

INFO - 6/35 frdm_k64f tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_64 PASSED (device: 0240000047784e45004e9003d9170022e561000097969900, 8.151s) INFO - 7/35 lpcxpresso55s36 tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_64 PASSED (device: 001069887263, 14.602s) INFO - 8/35 stm32f3_disco tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_64 PASSED (device: 066EFF555052836687113518, 4.123s) INFO - 9/35 mimxrt1060_evkb tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_64 PASSED (device: 0229000033f4cd1100000000000000000000000097969905, 33.479s) INFO - 10/35 nucleo_g474re tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_64 PASSED (device: 0041001B3438510B34313939, 13.539s) INFO - 11/35 frdm_k64f tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_32 PASSED (device: 0240000047784e45004e9003d9170022e561000097969900, 10.311s) INFO - 12/35 lpcxpresso55s36 tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_32 PASSED (device: 001069887263, 11.119s) INFO - 13/35 stm32f3_disco tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_32 PASSED (device: 066EFF555052836687113518, 3.966s) INFO - 14/35 mimxrt1060_evkb tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_32 PASSED (device: 0229000033f4cd1100000000000000000000000097969905, 37.112s) INFO - 15/35 nucleo_g474re tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.txdl_32 PASSED (device: 0041001B3438510B34313939, 13.679s) INFO - 16/35 frdm_k64f tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.unused PASSED (device: 0240000047784e45004e9003d9170022e561000097969900, 16.917s) INFO - 17/35 lpcxpresso55s36 tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.unused PASSED (device: 001069887263, 10.552s) INFO - 18/35 stm32f3_disco tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.unused PASSED (device: 066EFF555052836687113518, 14.584s) INFO - 19/35 mimxrt1060_evkb tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.unused PASSED (device: 0229000033f4cd1100000000000000000000000097969905, 33.130s) INFO - 20/35 nucleo_g474re tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance.fd.unused PASSED (device: 0041001B3438510B34313939, 14.217s) INFO - 21/35 frdm_k64f tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance PASSED (device: 0240000047784e45004e9003d9170022e561000097969900, 17.851s) INFO - 22/35 lpcxpresso55s36 tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance PASSED (device: 001069887263, 11.913s) INFO - 23/35 mimxrt1060_evkb tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance PASSED (device: 0229000033f4cd1100000000000000000000000097969905, 31.035s) INFO - 24/35 stm32f3_disco tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance PASSED (device: 066EFF555052836687113518, 14.670s) INFO - 25/35 nucleo_g474re tests/subsys/canbus/isotp/conformance/canbus.isotp.conformance PASSED (device: 0041001B3438510B34313939, 14.054s)

Do we really need to test with different TX_DLs on all boards or could a simple unit-like test (e.g. on POSIX) be sufficient for the different configurations? I mean, these tests validate the protocol implementation, not as much as the protocol interaction with the particular CAN controller?

Another option could be to tag some of these tests as "slow".

There will be some more upcoming ISO-TP updates. Let's leave the optimization of test time for a future PR.

dale-herman

Looks good.

martinjaeger · 2023-09-13T14:53:47Z

Fixed the typo in the test naming identified by @henrikbrixandersen. Please re-approve once again, @alexanderwachter . Thanks!

martinjaeger

Approval and thanks for the parts of the work @gramsay0 and @dale-herman have done. Also thanks for all reviews.

Add a test to check error codes if attempting to use ISO-TP with CAN FD mode even though the controller supports classical CAN only. Signed-off-by: Martin Jäger <martin@libre.solar>

Even though it was leading to the same result, the function should use its parameter and not the global variable for desired frames. Signed-off-by: Martin Jäger <martin@libre.solar>

alexanderwachter

Thanks Martin!

gramsay0 requested review from nashif, alexanderwachter and henrikbrixandersen as code owners July 13, 2023 05:30

zephyrbot added the area: CAN label Jul 13, 2023

zephyrbot requested review from karstenkoenig, martinjaeger and str4t0m July 13, 2023 05:30

zephyrbot assigned henrikbrixandersen Jul 13, 2023

gramsay0 force-pushed the isotp-canfd branch from f58a3dd to 4881353 Compare July 13, 2023 05:38

gramsay0 mentioned this pull request Jul 13, 2023

ISO-TP revamp #59896

Open

gramsay0 force-pushed the isotp-canfd branch from 4881353 to d208f0c Compare July 13, 2023 05:58

gramsay0 force-pushed the isotp-canfd branch from d208f0c to 04ae103 Compare July 17, 2023 02:03

gramsay0 commented Jul 18, 2023

View reviewed changes

tests/subsys/canbus/isotp/conformance/testcase.yaml Outdated Show resolved Hide resolved

martinjaeger requested changes Jul 19, 2023

View reviewed changes

gramsay0 force-pushed the isotp-canfd branch 2 times, most recently from ad26165 to d5be785 Compare July 30, 2023 04:14

martinjaeger previously approved these changes Aug 2, 2023

View reviewed changes

henrikbrixandersen requested changes Aug 7, 2023

View reviewed changes

subsys/canbus/isotp/Kconfig Outdated Show resolved Hide resolved

gramsay0 dismissed martinjaeger’s stale review via 6a799e2 August 8, 2023 02:55

gramsay0 force-pushed the isotp-canfd branch from d5be785 to 6a799e2 Compare August 8, 2023 02:55

martinjaeger force-pushed the isotp-canfd branch from 2ee0235 to e0f1f01 Compare August 22, 2023 10:46

henrikbrixandersen requested changes Aug 22, 2023

View reviewed changes

dale-herman reviewed Aug 22, 2023

View reviewed changes

subsys/canbus/isotp/isotp.c Outdated Show resolved Hide resolved

alexanderwachter reviewed Sep 6, 2023

View reviewed changes

subsys/canbus/isotp/isotp.c Outdated Show resolved Hide resolved

martinjaeger force-pushed the isotp-canfd branch from fb28a86 to c31f938 Compare September 7, 2023 07:57

benediktibk mentioned this pull request Sep 7, 2023

CI broken: samples/tfm_integration has invalid twister settings #62380

Closed

gramsay0 commented Sep 7, 2023

View reviewed changes

include/zephyr/canbus/isotp.h Outdated Show resolved Hide resolved

include/zephyr/canbus/isotp.h Outdated Show resolved Hide resolved

subsys/canbus/isotp/isotp.c Show resolved Hide resolved

tests/subsys/canbus/isotp/conformance/src/main.c Outdated Show resolved Hide resolved

martinjaeger force-pushed the isotp-canfd branch from c31f938 to b8b7625 Compare September 7, 2023 14:59

gramsay0 added 3 commits September 7, 2023 17:11

canbus: isotp: add CAN-FD support

da66772

ISO-TP CAN-FD support can be enabled at runtime. Signed-off-by: Grant Ramsay <gramsay@enphaseenergy.com> Signed-off-by: Martin Jäger <martin@libre.solar>

samples: canbus: isotp: Add support for CAN-FD

ffd5ef2

Enable this sample to be used with CAN-FD Signed-off-by: Grant Ramsay <gramsay@enphaseenergy.com>

tests: canbus: isotp: conformance: Add tests for CAN-FD support

45aa503

Allow existing tests to run with CAN-FD. Add new CAN-FD specific tests. Signed-off-by: Grant Ramsay <gramsay@enphaseenergy.com> Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger force-pushed the isotp-canfd branch from b8b7625 to 4b0e31f Compare September 7, 2023 15:12

martinjaeger requested a review from henrikbrixandersen September 7, 2023 15:29

henrikbrixandersen previously approved these changes Sep 12, 2023

View reviewed changes

carlescufi requested review from dale-herman, alexanderwachter and martinjaeger September 13, 2023 14:35

dale-herman approved these changes Sep 13, 2023

View reviewed changes

alexanderwachter previously approved these changes Sep 13, 2023

View reviewed changes

martinjaeger dismissed stale reviews from alexanderwachter and henrikbrixandersen via f412a17 September 13, 2023 14:50

martinjaeger force-pushed the isotp-canfd branch from 4b0e31f to f412a17 Compare September 13, 2023 14:50

martinjaeger previously approved these changes Sep 13, 2023

View reviewed changes

martinjaeger added 2 commits September 13, 2023 17:08

tests: canbus: isotp: conformance: add mode_check test

54a8448

Add a test to check error codes if attempting to use ISO-TP with CAN FD mode even though the controller supports classical CAN only. Signed-off-by: Martin Jäger <martin@libre.solar>

tests: canbus: isotp: conformance: fix global variable usage

c108c4d

Even though it was leading to the same result, the function should use its parameter and not the global variable for desired frames. Signed-off-by: Martin Jäger <martin@libre.solar>

martinjaeger dismissed their stale review via c108c4d September 13, 2023 15:09

martinjaeger force-pushed the isotp-canfd branch from f412a17 to c108c4d Compare September 13, 2023 15:09

henrikbrixandersen approved these changes Sep 13, 2023

View reviewed changes

alexanderwachter approved these changes Sep 13, 2023

View reviewed changes

carlescufi merged commit ef77fe3 into zephyrproject-rtos:main Sep 14, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

can: isotp: Add CAN-FD support to ISO-TP #60323

can: isotp: Add CAN-FD support to ISO-TP #60323

gramsay0 commented Jul 13, 2023 •

edited

alexanderwachter commented Jul 14, 2023

gramsay0 commented Jul 18, 2023

martinjaeger left a comment

gramsay0 commented Jul 20, 2023

gramsay0 commented Jul 21, 2023

henrikbrixandersen left a comment

martinjaeger commented Aug 7, 2023

martinjaeger commented Aug 22, 2023

henrikbrixandersen left a comment

henrikbrixandersen Aug 22, 2023

martinjaeger Aug 22, 2023

henrikbrixandersen Aug 22, 2023

martinjaeger Aug 22, 2023

henrikbrixandersen Aug 22, 2023

martinjaeger Aug 28, 2023

martinjaeger commented Aug 22, 2023

gramsay0 left a comment

martinjaeger commented Sep 7, 2023

henrikbrixandersen left a comment

henrikbrixandersen Sep 12, 2023

martinjaeger Sep 13, 2023

dale-herman left a comment

martinjaeger commented Sep 13, 2023

martinjaeger left a comment

alexanderwachter left a comment

can: isotp: Add CAN-FD support to ISO-TP #60323

can: isotp: Add CAN-FD support to ISO-TP #60323

Conversation

gramsay0 commented Jul 13, 2023 • edited

alexanderwachter commented Jul 14, 2023

gramsay0 commented Jul 18, 2023

martinjaeger left a comment

Choose a reason for hiding this comment

gramsay0 commented Jul 20, 2023

gramsay0 commented Jul 21, 2023

henrikbrixandersen left a comment

Choose a reason for hiding this comment

martinjaeger commented Aug 7, 2023

martinjaeger commented Aug 22, 2023

henrikbrixandersen left a 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

martinjaeger commented Aug 22, 2023

gramsay0 left a comment

Choose a reason for hiding this comment

martinjaeger commented Sep 7, 2023

henrikbrixandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dale-herman left a comment

Choose a reason for hiding this comment

martinjaeger commented Sep 13, 2023

martinjaeger left a comment

Choose a reason for hiding this comment

alexanderwachter left a comment

Choose a reason for hiding this comment

gramsay0 commented Jul 13, 2023 •

edited