Skip to content
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

[WIP ]Experimental NuttX SocketCAN based driver #303

Conversation

@PetervdPerk-NXP
Copy link

PetervdPerk-NXP commented Mar 23, 2020

Experimental NuttX SocketCAN driver for use with https://github.com/apache/incubator-nuttx/tree/SocketCAN.

I'm not sure if there's a need for this driver for libuavcan v0 however I used to verify my SocketCAN implementation.

This drivers supports:

  • Any board that implement the NuttX SocketCAN driver
  • SO_TIMESTAMP for received frames
  • CAN_RAW_TX_DEADLINE for settings deadlines on tx frames

What's not supported:

  • Loopback / recv own msgs
  • Correct clock implementation see clock.cpp
  • C++11 exceptions

For the correct clock implementation we could re-use some code from the PX4 libuavcan port.

@TSC21 TSC21 requested review from pavel-kirienko and thirtytwobits Mar 23, 2020
@thirtytwobits

This comment has been minimized.

Copy link
Contributor

thirtytwobits commented Mar 25, 2020

Sorry about the buildkite error. I think I've cleaned that up but if not we can ignore it.

Thanks for contributing back this code but I'm concerned that it's such a large amount of duplicated source from the linux driver. If we were to accept this it should be as a refactor that includes the nuttx driver as part of a unified "posix/socketcan" driver. That could be easily accomplished with some macros given the small amount of change but would ideally be more extensible. I've done just such a refactor to share this driver between linux and another RTOS I use but because we aren't continuing to develop v0 I never submitted it back.

I'm conflicted about this PR and I think @pavel-kirienko needs to decide if we want to simply take it as-is for archival purposes or if we would want to unified it before accepting it?

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Mar 25, 2020

It is great that Peter tested the new SocketCAN layer using an actual HLP implementation, but I don't think this codebase is usable in production in its current state. The main problem is that NuttX is used in real-time systems, where the approaches to resource management and error handling that work well for GNU/Linux are inapplicable. If we were to modify the driver to eliminate the reliance on the heap and exception handling, we would likely end up with a sufficiently different implementation to make its unification with the original impractical.

I think we are better off without this driver. Still, work has been done and it wouldn't be prudent to just discard it; Scott is right that there is value in having it archived. Peter, may I suggest just starting a brand new repo under your personal account or under an account controlled by NXP and pushing your code there in case we (or someone else) need it in the future? Make sure to remove the apps/ though since it's 100% identical to the original. After that is done, I would strongly suggest that we make a generic platform-agnostic SocketCAN driver available for Libcanard v1 and then Libuavcan v1. Before commencing the work on that we should plan out the approach to make sure it's sustainable enough to be included in the upstream.

@thirtytwobits

This comment has been minimized.

Copy link
Contributor

thirtytwobits commented Mar 25, 2020

I'm not exactly aligned here with Pavel in that I do think we could accept this into the v0 branch but we need to do some refactoring to reduce the amount of duplicated code with the linux driver. Let me submit a PR that includes my downstream refactor but stripping out the additional driver. You can use this as the basis for your NUTTX driver. Should be painless. I could have that sometime next week. If you want to do the refactor yourself sooner feel free to.

@thirtytwobits thirtytwobits reopened this Mar 25, 2020
@sonarcloud

This comment has been minimized.

Copy link

sonarcloud bot commented Mar 25, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Mar 25, 2020

Please ensure that the memory management and error handling issues are addressed.

@pavel-kirienko pavel-kirienko mentioned this pull request Mar 29, 2020
@pavel-kirienko

This comment has been minimized.

Copy link
Member

pavel-kirienko commented Mar 30, 2020

Closing due to #307.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.