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

Chapters 1 and 2 #32

Merged
merged 28 commits into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@pavel-kirienko
Copy link
Member

commented Nov 11, 2018

The first two chapters are virtually finished. I merely updated them according to our recent discussions, nothing new here. Note that the master branch is currently a staging branch, so the changes are not to be considered final after merging.

UAVCAN_Specification.pdf

@pavel-kirienko pavel-kirienko added this to the v1.0 milestone Nov 11, 2018

@pavel-kirienko pavel-kirienko self-assigned this Nov 11, 2018

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

@kjetilkjeka @thirtytwobits I am wondering if we can somehow expedite the review here as it is almost holding me back right now as I am working on chapter 4 (transport). I realize that the sections pertaining to data type versioning may require updates later depending on the resolution of the ongoing discussion about versioning, but that probably should not prevent this PR from being merged.

@thirtytwobits
Copy link
Contributor

left a comment

Some general comments but nothing blocking this revision.

Note that I'm not doing rigorous English grammar editing since that would be best left as an activity done for the completed specification.

Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex
Show resolved Hide resolved specification/introduction/introduction.tex
@kjetilkjeka
Copy link
Member

left a comment

Looks good, I've left a few nits. I don't think any of them require blocking for more discussion (maybe except for the "regulated" wording). Feel free to do what you want with them and merge when you need 👍

Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex
Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex
@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

@kjetilkjeka
Copy link
Member

left a comment

All my concerns have been resolved (as long as we're adding the boxes in the next pass)

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

Thanks, waiting for @thirtytwobits

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

I see that Scott has approved this PR, but the change I submitted today is quite substantial, so I believe it needs another review.

@thirtytwobits
Copy link
Contributor

left a comment

I think we've veered into some dangerous territory with this latest change. Please review my comments for details.

Fixed port identifiers can be used only with regulated data type definitions or with private definitions.
Fixed port identifiers must not be used with public unregulated data types,
since that is likely to cause unresolvable port identifier collisions.
This restriction must be followed at all times by all parties that use the specification;

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

This sentence implies that this is a term of use of the specification but such terms are not enforceable outside of the license. How about this:

This restriction shall be followed at all times by all compliant implementations and systems.

Fixed port identifiers must not be used with public unregulated data types,
since that is likely to cause unresolvable port identifier collisions.
This restriction must be followed at all times by all parties that use the specification;
if a private data type definition is ever disclosed to any other party (i.e., a party that did not author it)

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

wow, yeah. The terms here would raise the ire of many an attorney. The specification cannot govern any sort of disclosures. That is strictly the realm of contract and intellectual property law.

I think what you may want is a non-normative note that discusses the dangers of using fixed port identifiers with any data type that is shared; regardless of sharing that type with the general public, a specific vendor, or just within internal groups in a large organization. We can only wield the specter of non-conformity with this specification as a stick when discouraging port identifier abuses. Given the licensing terms we have no other mechanisms.

It's just as important that manufacturers understand this point. The maintainers and developers assume no liability. Should a business decide to build a million potted ESCs that use a datatype that conflicts with another entity's (say) GPS making the two products incompatible and should that non-compliant GPS already be on two million vehicles causing nobody to buy the compliant ESCs (because they didn't want to change their GPS units) nobody is liable. To provide anything stronger would require a legal entity similar to the Bluetooth SIG which would regulate and certify compatibility.

There are no warranties and this section must be very careful to avoid implying one and should also provide warnings about how the (legally) unregulated port id address space could impact devices.

Fixed port ID are allowed; they are called \emph{``regulated port ID''}.
&
Third-party definitions distributed separately from the UAVCAN specification.\newline
Fixed port ID are \emph{not allowed}.

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

%s/Fixed port ID are/Fixed port IDs are/g

Standard and third-party definitions.\newline
Fixed port ID are allowed; they are called \emph{``regulated port ID''}.
&
Third-party definitions distributed separately from the UAVCAN specification.\newline

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

The use of the term "Third-party" makes no sense within this specification given that it is not a contract (i.e. there are no parties). I think we need to say "Definitions not distributed with the UAVCAN specification" here and use a similar strategy for the rest of this section.

Fixed unregulated port identifiers can be allocated in the same range where application-specific identifiers are.
The ranges are documented in chapter \ref{sec:application_layer}.

DSDL processing tools are recommended to prohibit unregulated fixed port identifiers by default,

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

%s/are recommended to/shall

We might as well make this a normative requirement for DSDL processing. Ambiguity doesn't provide much on this point.

Since the set of regulated definitions is maintained in a highly centralized manner,
it can be statically ensured that no identifier collisions will take place within it;
also, since the identifier ranges used with regulated definitions are segregated from the rest,
it is also guaranteed that no regulated port ID will ever be in conflict with any other compliant UAVCAN

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

No guarantees, no warranties. We need to soften this to be non-normative language that details how we can help the ecosystem do the right thing; until we don't. I.e. if you (Pavel) get hit by a bus and nobody maintains the identifier registry there is no legal entity that is liable for the collapse of this mechanism. "Use this help at your own risk." We're not even liable if we get it wrong and accidentally re-issue an already issued identifier. Please remember that while we discuss "maintainers" this role does not legally exist and maintainers cannot not provide any real services.

The motivation for the prohibition of fixed port identifiers in third-party unregulated public data types is
derived directly from the above:
since there is no central repository of unregulated definitions,
collisions would be likely to happen, which would be extremely damaging to the ecosystem.

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Nov 28, 2018

Contributor

The specification should not discuss ecosystems in any normative section. An ecosystem is something that is completely outside of the purview of this specification.

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

@thirtytwobits ready for review. The wording is still a bit clumsy and the text could use some restructuring; I suggest we postpone that for another pass after the other chapters are finished.

@pavel-kirienko

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2018

A speedier review would help so that I could propose the next chapter (transport) before the call on Wednesday.

@thirtytwobits
Copy link
Contributor

left a comment

Mostly high-level grammar comments (where I think they affect meaning) and some more nits about inferring guarantees in the specification. Other than that I approve.

Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Message exchanges belonging to the same subject use same message data type down to the major version
(minor versions are allowed to differ) and pertain to same function or process within the system.

Request/response exchanges between nodes are grouped into \emph{services} by their semantic meaning,

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Dec 5, 2018

Contributor

same comment here about "their" being ambiguous

Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
\subsubsection{Data type definitions}

Message and service data types\footnote{Data types are the core concept of UAVCAN.}
are defined using the \emph{data structure description language} (DSDL) (chapter \ref{sec:dsdl}).

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Dec 5, 2018

Contributor
Suggested change
are defined using the \emph{data structure description language} (DSDL) (chapter \ref{sec:dsdl}).
are defined using the \emph{Data Structure Description Language} (DSDL) (chapter \ref{sec:dsdl}).

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Dec 5, 2018

Author Member

This is not supposed to be a proper noun, why capitalize?

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Dec 5, 2018

Author Member

Let's get back to this during the final proofreading.

Message and service data types\footnote{Data types are the core concept of UAVCAN.}
are defined using the \emph{data structure description language} (DSDL) (chapter \ref{sec:dsdl}).
A DSDL definition specifies the name, major version, minor version, the data structure,
and an optional predefined port ID of the data type.

This comment has been minimized.

Copy link
@thirtytwobits

thirtytwobits Dec 5, 2018

Contributor

When I first encountered "data type ids are optional" it really threw me for a loop. I'd love to get footnote or link here helping the reader understand that port IDs are eventually required but can be resolved later by a system when not using predefined identifiers. It's redundant and non-normative to mention it at this point but a footnote would help the read understand that the port ID is not, ultimately, optional.

This comment has been minimized.

Copy link
@pavel-kirienko

pavel-kirienko Dec 5, 2018

Author Member

They are not required for nested types. I am going to move this question into a separate ticket.

Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/basic_concepts/basic_concepts.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex Outdated
Show resolved Hide resolved specification/introduction/introduction.tex Outdated

thirtytwobits and others added some commits Dec 5, 2018

Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/introduction/introduction.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/introduction/introduction.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/introduction/introduction.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>
Update specification/basic_concepts/basic_concepts.tex
Co-Authored-By: pavel-kirienko <pavel.kirienko@gmail.com>

@pavel-kirienko pavel-kirienko merged commit 97369ae into master Dec 5, 2018

1 check was pending

ci/gitlab/subject-id Pipeline pending on GitLab
Details

@pavel-kirienko pavel-kirienko deleted the subject-id branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.