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-30987 - Support for ISO-TP protocol in SocketCAN by pylessard · Pull Request #2956 · python/cpython

Conversation

@pylessard

@pylessard pylessard commented Jul 31, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This Pull Request add the support of CAN ISO-TP address extensions in the socket module by enhancing getsockaddrarg and getsockname.

Note to core developpers : I believe this pull request should be backported back to Python 3.3 where SocketCAN support have been added.

https://bugs.python.org/issue30987

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pylessard added 3 commits July 25, 2017 17:54
@pylessard
Added support for CAN_ISOTP protocol
4d609c0
@pylessard
Added unit tests for CAN ISOTP
ccd65b8
@pylessard
Updated documentation for ISO-TP protocol
0017156
@the-knights-who-say-ni

the-knights-who-say-ni commented Jul 31, 2017

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 your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jul 31, 2017
@pylessard pylessard changed the title Fix issue 30987 bpo-30987 Jul 31, 2017
@pylessard pylessard changed the title bpo-30987 bpo-30987 - Support for ISO-TP protocol in SocketCAN Jul 31, 2017
pylessard added 4 commits July 30, 2017 21:55
@pylessard
Removed trailing whitespace in documentation
85a1545
@pylessard
Added blurb NEWS.d file
32399d4
@pylessard
updated Misc/ACKS
fc058f8
@pylessard
Fixed broken unit test that was using isotp const outside of skippabl…
2aa760d 
…e section
@Mariatta Mariatta removed the CLA not signed label Jul 31, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 31, 2017
@pylessard pylessard closed this Aug 1, 2017
@pylessard

pylessard commented Aug 1, 2017

Copy link
Copy Markdown
Contributor Author

After second thoughts, only the getsockaddress interface should be included in Python.
The constant in can-isotp should com from a third party module.
Will do the change and reopen the pull request
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pylessard added 3 commits August 2, 2017 19:22
@pylessard
Removed dependecy over third party project
fba09e6
@pylessard
Added implementation for getsockname + unit tests
17c58d7
@pylessard
Missing newline at end of ACKS file
7da8fed
@pylessard pylessard reopened this Aug 3, 2017
@pylessard

pylessard commented Aug 3, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Pull request is re-open. All dependencies over the third party project have been removed.
getsockname interface has been enhanced with ISO-TP protocol support to be consistent with the bind method input format.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pylessard pylessard mentioned this pull request Aug 3, 2017
Added isotp information to output struct for getsockname syscall hartkopp/can-isotp#3
Merged
@pylessard
Accidentally inserted a type in ACKS file
1365fca
tiran
tiran suggested changes Aug 26, 2017
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

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.

Thanks for your contribution. Your patch looks mostly fine, good work! I added notes for some style nit picks and a missing ifdef check.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.c
PyModule_AddIntMacro(m, CAN_EFF_MASK);
PyModule_AddIntMacro(m, CAN_ERR_MASK);

PyModule_AddIntMacro(m, CAN_ISOTP);

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

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.

This needs a #ifdef CAN_ISOTP.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.c Outdated
}
#endif

#if defined(CAN_ISOTP)

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

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.

#ifdef CAN_ISOTP

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/socketmodule.c Outdated
#endif

#if defined(AF_CAN) && defined(CAN_RAW) && defined(CAN_BCM)
#if defined(AF_CAN)

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

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.

Please use ifdef for simple checks: #ifdef AF_CAN

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated
def testBind(self):
try:
with socket.socket(socket.PF_CAN, socket.SOCK_DGRAM, socket.CAN_ISOTP) as s:
addr = (self.interface,0x123,0x456)

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

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.

Style nit pick: addr = self.interface, 0x123, 0x456

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_socket.py Outdated
# most systems limit IFNAMSIZ to 16, take 1024 to be sure
with socket.socket(socket.PF_CAN, socket.SOCK_DGRAM, socket.CAN_ISOTP) as s:
self.assertRaisesRegex(OSError, 'interface name too long',
s.bind, ('x' * 1024,1,2))

@tiran tiran Aug 26, 2017

Copy link
Copy Markdown
Member

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.

Please use

with self.assertRaisesRegex(OSError, 'interface name too long'):
    s.bind(('x' * 1024, 1, 2))

to make it more readable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Aug 26, 2017

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Aug 26, 2017
@pylessard
Followed tiran changes review #1 recommendations
280141c
@pylessard

pylessard commented Aug 26, 2017

Copy link
Copy Markdown
Contributor Author

I didn't expect the Spanish Inquisition!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Aug 26, 2017

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

@tiran: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Aug 26, 2017
@tiran

tiran commented Aug 27, 2017

Copy link
Copy Markdown
Member

Your PR is still missing a space after comma in test_socket.py, 280141c#diff-30065f853d7944a19b3e17b55db67733 . Let's follow PEP 8 for new code.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pylessard
Added spaces after comma
e2cc6f3
@pylessard

pylessard commented Aug 27, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

@tiran Changes done sir!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tiran
tiran approved these changes Aug 28, 2017
View reviewed changes

@tiran tiran left a comment

Copy link
Copy Markdown
Member

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.

perfect! 🥇

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Aug 28, 2017
@tiran
tiran merged commit a30f6d4 into python:master Aug 28, 2017
@Mariatta Mariatta removed the awaiting merge label Aug 29, 2017
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
@pylessard @GadgetSteve
bpo-30987 - Support for ISO-TP protocol in SocketCAN (python#2956)
04f20ba 
* Added support for CAN_ISOTP protocol

* Added unit tests for CAN ISOTP

* Updated documentation for ISO-TP protocol

* Removed trailing whitespace in documentation

* Added blurb NEWS.d file

* updated Misc/ACKS

* Fixed broken unit test that was using isotp const outside of skippable section

* Removed dependecy over third party project

* Added implementation for getsockname + unit tests

* Missing newline at end of ACKS file

* Accidentally inserted a type in ACKS file

* Followed tiran changes review #1 recommendations

* Added spaces after comma
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@tiran tiran tiran approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

5 participants

@pylessard @the-knights-who-say-ni @bedevere-bot @tiran @Mariatta
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.