8000 socketcand Backend for python-can by domologic · Pull Request #1140 · hardbyte/python-can · GitHub
[go: up one dir, main page]

Skip to content

socketcand Backend for python-can #1140

8000
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Dec 6, 2021

Conversation

domologic
Copy link
Contributor

We are using a Raspberry PI with MCP2515-based CAN-HAT, running a a socketcand as "IP-to-CAN-Gateway damon". For this, we have implemented a new backend.
socketcand uses an TCP-based ASCII protocol to represent the CAN frames. The most relevant advantage against an UDP-based protocol is that the order of messages is guaranteed.

It would be nice if you could merge this into your repository. Please let me know if we have something to add before you can merge the code.

@mergify mergify bot requested a review from hardbyte October 17, 2021 18:37
@codecov
Copy link
codecov bot commented Oct 22, 2021

Codecov Report

Merging #1140 (0493e32) into develop (71580d8) will decrease coverage by 0.25%.
The diff coverage is 16.96%.

@@             Coverage Diff             @@
##           develop    #1140      +/-   ##
===========================================
- Coverage    70.49%   70.23%   -0.26%     
===========================================
  Files           79       81       +2     
  Lines         7679     7920     +241     
===========================================
+ Hits          5413     5563     +150     
- Misses        2266     2357      +91     

@domologic
Copy link
Contributor Author

The test (macos-latest, false, pypy3) seems to be a problem with the environment, not with the patch.
Furthermore, we have not implementet tests so far. Is this a precondition for you to merge this pull requests into the master?

@felixdivo
Copy link
Collaborator

I have personally not used the deamon yet, but this looks useful.

The issue with the test environment should be fixed by now (thanks to @projectgus in #1143). We do not strictly enforce tests or test coverage, but some basic tests are very very welcome! Just a copy of BasicTestSocketCan as shown here would suffice. It would execute the general tests in Back2BackTestCase and assure basic functionality.

What IS required, however, is at least a small page of documentation, as described in the Developer’s Overview#Creating a new interface/backend. It should document the supported platforms and also the hardware/software it requires (in this case, just socketcand?). A small snippet of how to install the dependencies would also be useful get people started without much friction.

@felixdivo
Copy link
Collaborator

ping @domologic

@domologic
Copy link
Contributor Author
domologic c 8000 ommented Nov 23, 2021

Felix, thanks for your ping and sorry that I've lost this scope.
You are right - it does not make sense to set the logging level here. I've changed this in our branch.

Documentation will follow shortly.

@domologic
Copy link
Contributor Author

Felix, please check again. Some doc is also available, you may merge now.

@domologic
Copy link
Contributor Author

Felix, please check again

@felixdivo
Copy link
Collaborator

Why did you add the license in bed0ade? I don't think that is needed and it can be removed. It's already in the project root.

@domologic
Copy link
Contributor Author

Ah ok. Its removed now.

Copy link
Collaborator
@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

Looks good!

One could add tests (see comment) (especially appealing here, as this driver/backend does not need hardware), but it's not required.

@domologic
Copy link
Contributor Author

Yes, I would also like to see tests here. This would need a kind of "virtual remote station", implementing the same protocol. I hope we can contribute this in future.

@felixdivo
Copy link
Collaborator

Yes, I would also like to see tests here.

So shall we wait with merging until then?

@domologic
Copy link
Contributor Author
domologic commented Dec 6, 2021

No, please merge. We'll contribute tests later, but I can not say when.

@felixdivo felixdivo added this to the 4.0.0 Release milestone Dec 6, 2021
@felixdivo felixdivo merged commit acc2ebd into hardbyte:develop Dec 6, 2021
@felixdivo
Copy link
Collaborator

Great, @domologic! Looking forward to more contributions like this :)

cowo78 pushed a commit to cowo78/python-can that referenced this pull request Dec 9, 2021
* merge most recent version of socketcand-interface branch

* don't change loggine level

* do not request python 3.7

* parse < hi > & < ok > messages

* Initial upload: Doc for interfaces/socketcand

* Python code example was not displayed

* indent issues

* minor changes

* code optimization as proposed by felix

* Typo

* Monor doc improvement

* license information added

* reformatted using https://black.vercel.app/

* license removed

Co-authored-by: Gerrit Telkamp <g.telkamp@domologic.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0