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

Representation of an invalid ambiguous timestamp #63

Closed
pavel-kirienko opened this issue Mar 17, 2019 · 9 comments
Closed

Representation of an invalid ambiguous timestamp #63

pavel-kirienko opened this issue Mar 17, 2019 · 9 comments
Labels

Comments

@pavel-kirienko
Copy link
Member

As the ambiguous timestamp value is overflowing, none of its values can be reserved for special purposes; hence, the following is a mistake:

https://github.com/UAVCAN/dsdl/blob/dee68f6a1ef42efb82c56e2811c3c094ce9ea2dd/uavcan/time/SynchronizedAmbiguousTimestamp.1.0.uavcan#L11-L12

We may need to sacrifice one bit to make the value optional, like:

uint27 OVERFLOW_PERIOD_us = 83886080
truncated uint23[<=1] decamicrosecond
@pavel-kirienko
Copy link
Member Author

...which would make it variable-length, breaking alignment wherever it's used. We could do this instead:

  • Option A: Make it variable-length as described but move all usages towards the end, so that the alignment of other fields is unaffected. This is not a great option because it restricts usage of this type (what if it can't be moved toward the end?).

  • Option B: Add a bit flag alongside indicating whether the value is valid. This is not type safe, not great. We could alternatively use a union, which would be bitwise identical but type safe:

@union
Void23.1.0 empty
truncated uint23 decamicrosecond

Where Void23.1.0 is defined as void23 (can't use voids as union members directly, at least not yet).

  • Option C: Resurrect the old timestamp-related debates (1, 2) and see if we can either go back to 56-bit timestamps or find some cleverer solution. I never liked the short timestamp (as explained in the linked topics) but there didn't seem to be any other solution.

@thirtytwobits
Copy link
Member

will comment on this tomorrow (May 15th)

@thirtytwobits
Copy link
Member

Option B seems the most reasonable since some uses of this type will not require special values, therefore; B seems to optimize for the simplest use case.

I don't get the union idea though. How does that solve anything?

@pavel-kirienko
Copy link
Member Author

Suppose you have a flag like this:

bool valid
truncated uint23 decamicrosecond

When you receive a message, you can go about handling it like this:

if (message.timestamp.valid)  // Danger! Easy to forget.
{
    handle(message.timestamp.decamicrosecond * 10, message.value);
}

This is an unsafe typing fallacy: the protocol of handling this value could have been expressed through the type system, which would have allowed the compiler (assuming a strongly statically typed language here) to prevent the developer from using the value without checking whether it's valid first.

With the union, you would do something like this (the exact implementation may differ, this is only to demonstrate the idea):

if (auto decamicrosecond = message.timestamp.as<uavcan::time::SynchronizedAmbiguousTimestamp::Fields::decamicrosecond>())
{
    handle(*decamicrosecond * 10, message.value);
}

The language prevents you from using the value without checking first whether it exists. A DSDL union is a way to express the optionality of the value through the DSDL type system.

@pavel-kirienko
Copy link
Member Author

Option C: Resurrect the old timestamp-related debates (1, 2) and see if we can either go back to 56-bit timestamps or find some cleverer solution. I never liked the short timestamp (as explained in the linked topics) but there didn't seem to be any other solution.

I found myself leaning towards this option. My assessment is that CAN 2.0 will mostly fall out of use in the coming years in the domain of "software-defined vehicles" (relevant discussion #35 (comment)). Given that, and considering that the use of narrow timestamps essentially amounts to the optimization of the protocol for CAN 2.0, I would suggest considering switching back to the large timestamp. Some of the data points discussed in the linked above threads (1, 2) may have become partially obsolete since.

@pavel-kirienko
Copy link
Member Author

If we chose to go the way of option C, we could somewhat weaken the adverse effects on CAN 2.0 by defining a non-timestamped alternative for every scalar and vector type in the SI namespace, allowing adopters to sacrifice timestamp for bandwidth and latency. An interesting side effect of that change would be that we will be able to encode unit of measurement in the type. For example, currently, we have a SI type uavcan.si.electric_current.Scalar, which is defined as:

uavcan.time.SynchronizedAmbiguousTimestamp.1.0 timestamp
float32 ampere
@assert _offset_ / 8 == {7}

The above definition would be renamed into uavcan.si.electric_current.ScalarTS; the new definition under the old name would simply omit the timestamp:

float32 ampere

Now, suppose that you are defining a new application-specific data type; let it contain a current setpoint for the sake of example. Normally it might look as:

uint8 index_offset
void19
float32[<=30] current_setpoint
@assert _offset_ % 8 == {0}

Having the suitable non-timestamped SI type, one could express the unit in the type:

uint8 index_offset
void19
uavcan.si.electric_current.Scalar[<=30] current_setpoint  # <-- here
@assert _offset_ % 8 == {0}

Implementations could then optionally special-case the types under uavcan.si.* as mappings to the appropriate definitions of a typesafe physical units library, like Boost Units.

@thirtytwobits
Copy link
Member

My assessment is that CAN 2.0 will mostly fall out of use in the coming years

Let me just challenge that statement first. I'm not convinced. What data do you have to support CAN-FD even overtaking CAN 2.0 in the "next few years"? Specifically, my assumption is that we'll see a lot of FD capable peripherals and transceivers deployed but that they may be used in CAN 2.0 mode until all the nodes on a vehicle have been converted and have overcome the challenges associated with the faster data rate (e.g. bus capacitance) and larger data payloads (e.g. downstream data busses might not be ready to handle the additional load).

@pavel-kirienko
Copy link
Member Author

During today's dev call we have reached a tentative consensus to implement Option C. The resulting suboptimality of the SI protocol for CAN 2.0 is considered tolerable because SI is a relatively minor and insignificant subset of the protocol, lack of good support for which should not significantly affect the adequacy of UAVCAN for CAN 2.0 deployments. The upside of this option is the removal of inherently problematic roll-over timestamp types.

@pavel-kirienko
Copy link
Member Author

Fixed in #67

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

No branches or pull requests

2 participants