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

Issue #222: v1 platform layer #223

Merged
merged 10 commits into from
May 29, 2019

Conversation

thirtytwobits
Copy link
Contributor

@thirtytwobits thirtytwobits commented May 22, 2019

Interim PR here but I wanted to get this through review before it got
too big. Things of interest:

  1. Coverage build logic is complete.
  2. Better definition around the LVS (Libuavcan Validation Suite)
  3. Start of the "media" layer for CAN.
  4. More sadness that it isn't five years from now and we could just use
    C++14.
  5. Also just added a compile-time test to define how we will do these.

Interim PR here but I wanted to get this through review before it got
too big. Things of interest:

1. Coverage build logic is complete.
2. Better definition around the LVS (Libuavcan Validation Suite)
3. Start of the "media" layer for CAN.
4. More sadness that it isn't five years from now and we could just use
C++14.
Copy link
Member

@pavel-kirienko pavel-kirienko left a comment

Choose a reason for hiding this comment

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

Some of my comments are not meant for fixing in this PR but should be looked at eventually (in the near future perhaps?). For example, my notes about dropping support for standard frames and branch annotations.

Just to make sure I recall our earlier discussions properly: we're not going to implement any toString() methods or streaming << operators, right? Because I don't think they were needed in the first place and discarding them is actually a good thing.

libuavcan/include/libuavcan/transport/media/can.hpp Outdated Show resolved Hide resolved
libuavcan/include/libuavcan/transport/media/can.hpp Outdated Show resolved Hide resolved
ci/native-gcc-build-and-test.sh Outdated Show resolved Hide resolved
libuavcan/include/libuavcan/transport/media/can.hpp Outdated Show resolved Hide resolved
libuavcan/include/libuavcan/transport/media/can.hpp Outdated Show resolved Hide resolved
libuavcan/include/libuavcan/transport/media/can.hpp Outdated Show resolved Hide resolved
libuavcan/include/libuavcan/transport/media/can.hpp Outdated Show resolved Hide resolved
test/compile/compile_tests.cmake Show resolved Hide resolved
@thirtytwobits
Copy link
Contributor Author

Just to make sure I recall our earlier discussions properly: we're not going to implement any toString() methods or streaming << operators, right? Because I don't think they were needed in the first place and discarding them is actually a good thing.

Correct.

@thirtytwobits
Copy link
Contributor Author

So as I work on finishing this PR I begin to wonder about this picture:

image

Here is the journey of a CAN frame up to the transport layer. What we see is we've incurred two copies into and then across system memory before the application gets the data. This seems sub-optimal.

Perhaps CAN::Frame shouldn't have storage and should be a view into memory owned by the driver?

@pavel-kirienko
Copy link
Member

What I learned recently by reading a mildly related FAA publication is that contrary to my expectations, zero-copy stacks are not considered unsafe or otherwise undesirable:

The communication stack should minimize the copying of data when passing a packet up or down through the layers of the stack. This feature is called zero copy. This feature essentially reduces the overall time taken for processing the packet. The incoming frame is just copied once to a generic buffer, which can be accessed by all the layers of the network stack.

So we should definitely explore this design option. Frankly I don't yet know how to approach this in our hard real-time setting. My PyUAVCAN rewrite is zero-copy, but that is trivial to implement because I can use the free store as much as I want. In libuavcan, we may need to coordinate data allocation and deallocation with the driver, which may be tricky.

@thirtytwobits
Copy link
Contributor Author

So we should definitely explore this design option. Frankly I don't yet know how to approach this in our hard real-time setting. My PyUAVCAN rewrite is zero-copy, but that is trivial to implement because I can use the free store as much as I want. In libuavcan, we may need to coordinate data allocation and deallocation with the driver, which may be tricky.

I'm becoming convinced that this will be a significant improvement to the library but I don't think we are ready to build this just yet. I'm going to open a task to revisit this once I have NXP's driver since I can look at my MCAN driver for Atmel and compare it to the NXP Flex-CAN implementation and finally consider socketCAN to come up with a design that will be relevant and portable. Not only will this improve performance for microcontrollers but it may actually simplify implementing libuavcan for them. My assumption is that, by defining how to coordinate DMA access, we'll provide a stronger media layer contract that will make it obvious how to port it to a given peripheral. But we'll see if I'm right sometime after this review is posted. For now I'm providing careful guidance on and visibility into the library's use of CPU for moving things though system memory.

(Perhaps this will become a menu of options for porting the media layer where linux wants to use what we have here and microcontrollers want something else?)

@thirtytwobits
Copy link
Contributor Author

Is there anything blocking your signoff on this PR now @pavel-kirienko ?

doc_source/media.md Outdated Show resolved Hide resolved
doc_source/media.md Outdated Show resolved Hide resolved
@thirtytwobits thirtytwobits merged commit 3ef30c4 into OpenCyphal-Garage:uavcan-v1.0 May 29, 2019
@pavel-kirienko
Copy link
Member

I am working on Libcanard atm and I felt like going back to this discussion and mentioning that this part is trivially resolvable if there is an O(1) memory allocator available:

Frankly I don't yet know how to approach this in our hard real-time setting. My PyUAVCAN rewrite is zero-copy, but that is trivial to implement because I can use the free store as much as I want. In libuavcan, we may need to coordinate data allocation and deallocation with the driver, which may be tricky.

The library can allocate a buffer using the constant-complexity deterministic free store, and then pass the buffer up and down the protocol stack as needed without copying. Such free store can be accessed from an interrupt context as well, although I don't immediately see how that can be practical yet. This is also compatible with the FAA considerations about aviation databuses. Stand by for my Libcanard PR.

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

Successfully merging this pull request may close these issues.

2 participants