GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-42653: Add constants to socket module for IsoTp CAN protocol · Pull Request #26174 · python/cpython

Conversation

@ghost

@ghost ghost commented May 16, 2021 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Patrick Menschel
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 bedevere-bot added the awaiting review label May 16, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label May 16, 2021
@the-knights-who-say-ni

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

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Patrick Menschel
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 the-knights-who-say-ni added CLA signed and removed CLA not signed labels May 19, 2021
Patrick Menschel added 3 commits May 19, 2021 17:47
Patrick Menschel
Add test for isotp constants into unit test.
1f7c2c5 
Signed-off-by: Patrick Menschel <[email protected]>
Patrick Menschel
Add entry to Doc/whatsnew/3.10.rst
bc9118c 
Signed-off-by: Patrick Menschel <[email protected]>
Patrick Menschel
Fix a whitespace issue in test_socket.py
523bf9f 
This one slipped by, sorry.

Signed-off-by: Patrick Menschel <[email protected]>
@ghost ghost 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
Patrick Menschel
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]>
@ghost

ghost commented May 19, 2021

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rumpelsepp

rumpelsepp commented May 20, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost

ghost commented May 20, 2021 via email

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Patrick Menschel
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]>
@ghost

ghost commented May 22, 2021

Copy link
Copy Markdown
Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Patrick Menschel
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]>
@ghost

ghost commented May 22, 2021

Copy link
Copy Markdown
Author

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
>>>
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Patrick Menschel
Fix: Optionally include CAN_ISOTP_SF_BROADCAST
8132cb1 
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
rumpelsepp reviewed May 23, 2021
View reviewed changes
Comment thread 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);

@rumpelsepp rumpelsepp May 23, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost ghost May 23, 2021

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rumpelsepp rumpelsepp May 23, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost ghost May 23, 2021

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rumpelsepp rumpelsepp May 23, 2021

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

aye.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ghost

ghost commented Jun 19, 2021 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

BUMP.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rumpelsepp rumpelsepp mentioned this pull request Mar 1, 2022
gh-86819: Add ISO-TP CAN socket constants #23794
Merged
@MaxwellDupre

MaxwellDupre commented May 18, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hartkopp

hartkopp commented May 18, 2022

Copy link
Copy Markdown

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost

ghost commented May 18, 2022 via email

Copy link
Copy Markdown
Author
Be aware, these PRs have been stale for over a year. I consider them dead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hartkopp

hartkopp commented May 18, 2022

Copy link
Copy Markdown

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@rumpelsepp rumpelsepp mannequin mentioned this pull request Apr 10, 2022
Expose ISO-TP Contants for Linux >= 5.10 #86819
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@the-knights-who-say-ni @rumpelsepp @MaxwellDupre @hartkopp @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.