OpenCyphal-Garage / libcyphal

Portable reference implementation of the Cyphal protocol stack in C++ for embedded systems and Linux.
http://opencyphal.org
MIT License
299 stars 500 forks source link

[WIP ]Experimental NuttX SocketCAN based driver #303

Closed PetervdPerk-NXP closed 4 years ago

PetervdPerk-NXP commented 4 years ago

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:

What's not supported:

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

thirtytwobits commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

sonarcloud[bot] commented 4 years ago

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 commented 4 years ago

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

pavel-kirienko commented 4 years ago

Closing due to #307.