Review #6

[pavel-kirienko]

Now it's my turn to apologize for slowness, certain things ended up being harder than they appeared. I'd like to iterate faster in the future, because large changes slow things down.

1.2 is makes me belive that datatypes can be changed without incrementing minor version. It should be changed to make it clear that datatypes are immutable. Section 2. explains this well.

Fixed.

There are DSDL definitions that break the nice layering. The most important is the NodeStatus message. The protocol is in fact dependent on this definition and it can not be updated without breaking the protocol. It should be mentioned that the NodeStatus message is special somwhere (towards the end perhaps) of section 2.
What is the status of changing up namespaces to reflect functions/domain (aero, ground, etc)? This is not done in your draft. We should probably create an issue or similar to discuss this, right?

Things pertaining to the application layer are to be reviewed in the chapter 5, which is next. However, before we get to that, I'd like to sort out two things:

1. Whether we really want to get all generic and split the transport layer specification in two parts: abstract and CAN-specific. There aren't any non-CAN transport on the horizon currently, so I'm not sure if it makes sense doing that now. We can always do that later shall the need for a new transport arise. Please share your thoughts or even open a new ticket for that. (re tickets: it's getting really messy; do we not want to launch an instance of Discourse for discussions? Clearly the mailing list is not working).

2. Sort out the new set of standard data type definitions. I should start looking into that now.

3.6.2.2 are unions fixed or variable sized data structures. I think dynamic? In that case, there's an error in the text.

Fixed.

3.6.2.1 Valid encoded representation needs defining. One of the things im unsure of: Is a void field encoded to a non-zero value valid encoded?

This was a tough one. I think it's much more sensible now although it's still imperfect. I am very keen on keeping the definition as generic as possible (this closely corresponds to your "relaxed" option from #9).

3.6.3.2 If we allow fields to change name we will break code by incrementing minor version. I'm not sure if this is something we want to allow. Especially since there often exists none breaking ways to work around it. I think we should create an issue and discuss this.

I think we must allow that; see discussion at #9.

4.1.1.2 Perhaps mention here or later that when creating a datatype that is intended to be used for anonymous frame transfer it must fit inside a single frame transfer for the transport protocol it is intended to be used with.

Um, not quite. A message data type does not have to always use single-frame transfers to be usable with anonymous transfers; a good counter-example is the standard node ID allocation message. Clarifications added, better wording welcome.

4.1.2.1 I'm not sure the "• The server should be able to process any request in under 0.5 seconds." requirement makes sense to impose that strongly. Should perhaps be replaced with something more general like "If the server uses a significant part of the timeout period to process the request, the client might drop the request before receiving the response. It's good practice to make sure that the server will be able to process any request in less than 0.5 seconds."

Fixed.

4.1.3 I'm not sure that these mnemonics makes sense. In my mental model there are really only two type of priority levels. Realtime and non-realtime (or background if you want). All real-time transfers needs to finish before it's deadline. And the way to achieve this is by giving high priority to messages with close deadlines, not by some importance value. Words like "Emergency" and "Critical" is thus misleading. Another problem is that these priorities only make sense relative to other and need some sort of globabl analysis to make sense.

This is a specification not a textbook on real-time system design, so I suggest keeping it brief. The mnemonics I chose were supposed to convey urgency, not importance; e.g. "critical" stands for "critically urgent", not "critically important", although I see now that this is not sufficiently clear. Let's pick better names. I think the following names should be kept because they are rather unambigous: Foreground, High, Normal, Low, Background. If these are to be kept, we need three more. How about Über. Two more?

Even though it's not incorrect to call CAN 2.0 legacy, as CAN-FD is both a better and newer version of CAN 2.0, I would like to avoid this term due to possible negative conotations when used for software. The masses are not going to make the change over night, and neither should we. I think we should show understanding of users not able to upgrade at this point by referaining from refering to it as legacy. For now, I think we should treat these protocols equal in terms of support and wording, except that we acknowledge CAN-FD to be an technical advancement.

Fixed.

Consider remarking that the set of single/multi -frame transfers are not equal between different transport protocols.

Fixed?

In table 4.1, row "Source Node ID" - should it say that anonymous transfers should have ID 0 instead of saying "excepting"?

Source Node ID is not defined for anonymous message transfers, so I used that wording on purpose. I think it's clear enough, but we can discuss this.

Should message/service

Fixed.

4.1.1.1 Consider to tie (4.1.1) Message broadcasting closer (more clearly) together with the earlier defined concept of "message transfer".

Currently, the text says "A broadcast message is carried by a single message transfer that contains the serialized message data structure."
Would you like to change or amend that?

4.1.1.1 Consider changing/removing the word Regular in "Regular message broadcasting". Is "non-anonymous" meant by regular? I don't think it's safe to assume that the readers will understand that.
4.1.1.1 "An exception applies to anonymous message broadcasts." If regular is supposed to mean "non-anonymous" then this is contradicting as it indicateds that anonymous transfers also are regular.

Removed the sub-sub-section to avoid the confusion. Having "Message broadcasting" inside "Message broadcasting" seems odd.

4.1.1.1 Consider moving the (new info) fact that node IDs needs to be unique to "Basic concepts" (section 2) and only state that a Node ID is required for "regular message broadcasting".

This info is not new; here's an excerpt from Basic concepts: "A UAVCAN network is a decentralized peer network, where each peer (node) has a unique numeric identifier - \emph{node ID}.".

4.1.1.2 Consider changing "from a node that does not have a node ID" to "can be sent from a node that does not have a node ID". Since it's not prohibited for a node with Node ID to send an anonymous transfer.

Fixed.

4.1.1.2 Should passive mode definition instead live in section 2 together with the Node ID definition introduction?

I'm not sure it's necessary. The concept of the passive mode is not referred to before this point, and it's not particularly important, so I see little value in moving it all the way up to the beginning.

4.2.2 and 4.2.3 - It's very suprising that single/multi frame transfers are defined here when they are introduced in 4.1, the text is so short that I believe everything relating to this concept should be put in the same place. I think the introduction should be removed from 4.1 and then the full definition could be moved directly after the message/service definitions (before transfer ID computation).

What we have in 4.1 is the actual definition. 4.2 is intended to describe implementation. I think this separation makes sense in that case, especially if we are to split the transport layer spec into generic and CAN-specific parts?

Consider phrasing the compatibility concerns more pessimistically. For instance "Even though UAVCAN currently can share the bus with other protocols based on standard (non-extended) CAN frames with 11-bit identifiers it is not guaranteed that this property will hold in the future. Doing so is not endorsed and would be done on the users own account. It would require the user to follow protocol updates closely not to break anything."

I'm not sure I like the excessive wordiness in this part. The conveyed meaning seems identical in both cases.

Table 4.6 - I find it much harder to read when vertical and not color coded. Consider using a landscape (horizontal) oriented page and/or use similar color coding as the web page.

I wanted to avoid importing this critical diagram as an image for ease of maintenance, so I coded it up in LaTeX. I tried adding the colors and that turned out to be harder than I'd like to admit. While I agree that the new diagram is harder to read, it seems to be just good enough, so I'm not going to change it as we have more pressing matters to address. That said, feel free to contribute a better solution.

4.3.2.2 didn't we end up on recommending padding bytes to be 0x55 instead of requiring this? This is really just a nit, and I'm totally fine with requiring it if you want to. What about the padding bits? Wouldn't the same consideration apply to them?

I intended to raise a discussion about this but it kind of slipped out of my mind. The reason I made it a hard requirement is to add more determinism; that might be useful in the future if backward-incompatible changes are to be made to the protocol. Same reason why we require the toggle bit to always start at a particular value even though the protocol doesn't need to care.

Comments from Scott have also been addressed.

[pavel-kirienko]

Whether we really want to get all generic and split the transport layer specification into two parts: abstract and CAN-specific. There aren't any non-CAN transport on the horizon currently, so I'm not sure if it makes sense doing that now. We can always do that later shall the need for a new transport arise.

I just went over the spec and realized that this abstraction should be implemented, after all. I'm going to move it into a new branch.

[kjetilkjeka]

I'm away until 18. August I'm only able to look at the computer every now and then when traveling but will try to look through it before I'm home. I will be able to give a full review when I'm back home.

Don't let this block your work, If you need to merge before I'm able to give a review I will create issues/PR on the parts I'm not completely satisfied with once I'm back home.

[pavel-kirienko]

Attaching the latest PDF: UAVCAN_Specification.pdf