bpo-42653: Add constants to socket module for IsoTp CAN protocol #26174

menschel · 2021-05-16T18:55:31Z

This PR adds constants to socket module for IsoTp CAN protocol.

IsoTp CAN protocol is mainline since Kernel v5.10.
The basic socket constant for IsoTp has been available since Python 3.7 (August 2018),
so it could be used for default use-cases without having issues.

However several constants from isotp.h have not found their way into socket module yet.
When using advanced options like padding, this flaw becomes visible.
https://gitlab.com/Menschel/socketcan/-/blob/master/socketcan/socketcan.py#L18

This PR may be related to #23794.

https://bugs.python.org/issue42653

IsoTp CAN protocol is available since Kernel v5.10 but several constants from isotp.h have not found their way into socket module. This commit adds that header file if configure finds it. Signed-off-by: Patrick Menschel <[email protected]>

the-knights-who-say-ni · 2021-05-16T18:55:34Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@menschel

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Use PyModule_AddIntMacro() to add all constants from isotp.h . Signed-off-by: Patrick Menschel <[email protected]>

Signed-off-by: Patrick Menschel <[email protected]>

This one slipped by, sorry. Signed-off-by: Patrick Menschel <[email protected]>

The issue number was found eventually. Apparently there already was a PR with this topic. Signed-off-by: Patrick Menschel <[email protected]>

menschel · 2021-05-19T17:57:35Z

Dear Reviewer,
this PR apparently targets the same topic as #23794
Please advise.

rumpelsepp · 2021-05-20T06:52:39Z

My pullrequest seems to lack these *_DEFAULT_* constants. Are these new to the kernel module?

edit: Ah okay, these are part of the inclusion in the mainline kernel. I missed these.

menschel · 2021-05-20T10:25:08Z

Yes, https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/isotp.h The one on @hartkopp 's repo is outdated.

For unknown reason, HAVE_LINUX_CAN_ISOTP_H was not set during build. After changing the order of items in that particular line of configure.ac, it now works as expected. Signed-off-by: Patrick Menschel <[email protected]>

menschel · 2021-05-22T16:05:32Z

Dear Reviewer the patch has been verified on a rpi3b.

Please also review my Unittest 1f7c2c5 . It did not fail during make run before 2f19691 .

Thanks.

This may not be necessary but it is clean. Also normalize the comment line above. Signed-off-by: Patrick Menschel <[email protected]>

menschel · 2021-05-22T20:57:15Z

The patch also works on Python 3.9.5. This was build on a rpi0w.

$ python -i
Python 3.9.5 (heads/master-dirty:00ae35b, May 22 2021, 21:28:37) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> socket.CAN_ISOTP_OPTS
1
>>> socket.SOL_CAN_ISOTP
106
>>>

CAN_ISOTP_SF_BROADCAST is new and not always available. This was the case on a recent RaspberryPi Kernel. Therefore make it optional and exclude it from test. Also add condition for testing the constants. Signed-off-by: Patrick Menschel <[email protected]>

rumpelsepp · 2021-05-23T06:54:33Z

Modules/socketmodule.c

    PyModule_AddIntMacro(m, CAN_ISOTP_WAIT_TX_DONE);
+#ifdef CAN_ISOTP_SF_BROADCAST
+    /* This constant is new and not always available */
    PyModule_AddIntMacro(m, CAN_ISOTP_SF_BROADCAST);


I assume the version of the kernel could be checked on buildtime, couldn't it?

Technically yes, but there is no guarantee that this symbol really is there when the kernel version is above a specific version.
If you install the headers yourself with linux-headers-$(uname -r), the assumption works.

But if the installation of headers has been obscured, like with Raspberry Pi 's package raspberrypi-kernel-headers, this assumption no longer works. It just adds another assumption that the packages
raspberrypi-kernel-headers and raspberrypi-kernel are in sync.

Personally, I would argue that the mainline upstream kernel is the source of truth and this must be fixed downstream. No idea how the CPython project deals with these kinds of issues.

Well, lets wait for the Reviewer to comment on this. This should theoretically be @tiran.

menschel · 2021-06-19T15:27:47Z

BUMP.

This PR is stale for about a month now. Is there a way to accelerate this?

MaxwellDupre · 2022-05-18T15:59:53Z

Need to re-target Doc/whatsnew/3.10.rst to 3.12.

hartkopp · 2022-05-18T16:05:41Z

Need to re-target Doc/whatsnew/3.10.rst to 3.12.

While you are at it:

Meanwhile another new constant CAN_ISOTP_CF_BROADCAST will hit the road in Linux 5.19 ...

pylessard/python-can-isotp#67 (comment)

menschel · 2022-05-18T18:32:25Z

Be aware, these PRs have been stale for over a year. I consider them dead.

hartkopp · 2022-05-18T18:54:32Z

Be aware, these PRs have been stale for over a year. I consider them dead.

Yes but when @MaxwellDupre makes the zombie run again ... ;-)

Add import for linux/can/isotp.h

0cf7cd7

IsoTp CAN protocol is available since Kernel v5.10 but several constants from isotp.h have not found their way into socket module. This commit adds that header file if configure finds it. Signed-off-by: Patrick Menschel <[email protected]>

bedevere-bot added the awaiting review label May 16, 2021

the-knights-who-say-ni added the CLA not signed label May 16, 2021

Add IsoTp constants in socketmodule.c

17535b5

Use PyModule_AddIntMacro() to add all constants from isotp.h . Signed-off-by: Patrick Menschel <[email protected]>

the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 19, 2021

menschel added 3 commits May 19, 2021 17:47

Add test for isotp constants into unit test.

1f7c2c5

Signed-off-by: Patrick Menschel <[email protected]>

Add entry to Doc/whatsnew/3.10.rst

bc9118c

Signed-off-by: Patrick Menschel <[email protected]>

Fix a whitespace issue in test_socket.py

523bf9f

This one slipped by, sorry. Signed-off-by: Patrick Menschel <[email protected]>

menschel changed the title ~~Add constants to socket module for IsoTp CAN protocol~~ bpo-42653: Add constants to socket module for IsoTp CAN protocol May 19, 2021

Add issue number 42653 and blurb.

eb283c2

The issue number was found eventually. Apparently there already was a PR with this topic. Signed-off-by: Patrick Menschel <[email protected]>

Fix: Setting of HAVE_LINUX_CAN_ISOTP_H by configure

2f19691

For unknown reason, HAVE_LINUX_CAN_ISOTP_H was not set during build. After changing the order of items in that particular line of configure.ac, it now works as expected. Signed-off-by: Patrick Menschel <[email protected]>

Fix: Change order of headers in configure

5860ad0

This may not be necessary but it is clean. Also normalize the comment line above. Signed-off-by: Patrick Menschel <[email protected]>

rumpelsepp reviewed May 23, 2021

View reviewed changes

rumpelsepp mentioned this pull request Mar 1, 2022

gh-86819: Add ISO-TP CAN socket constants #23794

Merged

ezio-melotti removed the CLA signed label Jul 13, 2022

rumpelsepp mannequin mentioned this pull request Apr 10, 2022

Expose ISO-TP Contants for Linux >= 5.10 #86819

Closed

This pull request was closed.

Uh oh!

bpo-42653: Add constants to socket module for IsoTp CAN protocol #26174

bpo-42653: Add constants to socket module for IsoTp CAN protocol #26174

Uh oh!

Conversation

menschel commented May 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented May 16, 2021

Recognized GitHub username

Uh oh!

menschel commented May 19, 2021

Uh oh!

rumpelsepp commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

menschel commented May 20, 2021 via email

Uh oh!

menschel commented May 22, 2021

Uh oh!

menschel commented May 22, 2021

Uh oh!

rumpelsepp May 23, 2021

Choose a reason for hiding this comment

Uh oh!

menschel May 23, 2021

Choose a reason for hiding this comment

Uh oh!

rumpelsepp May 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

menschel May 23, 2021

Choose a reason for hiding this comment

Uh oh!

rumpelsepp May 23, 2021

Choose a reason for hiding this comment

Uh oh!

menschel commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxwellDupre commented May 18, 2022

Uh oh!

hartkopp commented May 18, 2022

Uh oh!

menschel commented May 18, 2022 via email

Uh oh!

hartkopp commented May 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

menschel commented May 16, 2021 •

edited

Loading

rumpelsepp commented May 20, 2021 •

edited

Loading

rumpelsepp May 23, 2021 •

edited

Loading

menschel commented Jun 19, 2021 •

edited

Loading