added rough mavlink 2.1 proposal

[tridge]

whipped up rather quickly, apologies for low detail

[hamishwillee]

Can we strip the ids out out of the payload in this case by default in the generator?

[tridge]

no, I don't think so, I think that would complicate the parsers too much

[hamishwillee]

Sending the data twice, forever, seems like something we should try avoid. Is there a future where we can remove the ids from the XML then?

Jumping back a little, currently a message has the target ids in the payload, and knows to add them from the message definition. Other fields like sender ids are always present.

How will the library know that it should add target ids to the header? You might be assuming they will always be added for systems after 256 or you might be assuming these will still be added conditionally to the header based on the message definition.

[tridge]

they are added to the header when the value is >255 and the incompat flag is set
the receiver uses the value from the header when the incompat flag is set

[hamishwillee]

Where I'm coming from is that in a current network the target ids are only added if you have explicitly added them in the payload.

In the proposal if a message is sent to a system with id >255 from a sender that supports this approach you will always include the target and system id in the header, at a cost of 2 bytes for the system id + 1 byte for the component id.

I'm not arguing the sense of this, I'm confirming "always sent under above rules" not "sent based on something a user of the library does or the message definition specifies".

[hamishwillee]

For terminology, for "structural seed" do you mean the CRC_EXTRA - i.e. the CRC created by running the calculation over the name of the message and the and types of the field in over-the-wire order.

So is the proposal that you have run the whole CRC for the message contents (16 bits) then apply the CRC_EXTRA somehow to just the last 8 bits? Or something else? I'm just trying to get my head around how you can know whether the CRC passed/failed.

[tridge]

yes, CRC_EXTRA is the structural seed, it aims to checksum the structure
unfortunately my proposal means you'd need to calculate the CRC twice

[hamishwillee]

OK. I still don't understand the proposed algorithm, but perhaps others will.

Right now I think it is it is

• to send: run CRC algorithm over each field in order to calculate the CRC of the message, then accumulate the CRC_EXTRA over the top of that. Result a 2 byte CRC
• on receive: run CRC over incoming data, then apply your CRC Extra. Numbers should match.
• Problem is that you can't separate CRC and CRC_EXTRA - you don't know if a failure is message being corrupt or incompatible and you can't work that out without having the CRC_EXTRA matching the library.

What I think you are suggesting is that you can separate these by applying the CRC_EXTRA just on the final 8 bits of the CRC, but I don't understand the encode/decode algorithm.

[tridge]

i think this works:

• calculate crc16 over message bytes as transmitted on wire. call this 16 bit value c1
• b1 = c1&0xFF
• continue crc16 over the CRC_EXTRA bytes call the resulting crc c2
• b2 = c2 >> 8
• c3 = b1<<8 | b2

crc used in header is c3

[hamishwillee]

Thanks. Hopefully someone who knows what they are doing can sanity check that :-)

[hamishwillee]

Trying to confirm I "grok" this ...

Presumably this is all assuming a >16 bit data type the code is working on (hence the need to do c1&0xFF) so when you send c3 you just take the bottom 2 bytes?

My understanding of this is:

• A router on receiving could calculate b1 and compare to the "b1 byte" of c3 it got in the message. If this matched this would indicate a pass of the CRC check so the router could forward the message; if not it would know the message was corrupt and could drop it.
• At a final end points the receiver can compare the first byte to check for bit corruption, and the second byte to check for either bit corruption or message mismatch.

If so, I can see this will algorithm works. Is there a loss of data compared to the current situation, and is it a concern? I mean, you now have a single byte of data that derives from the CRC_EXTRA calculation. That means that if you have more than 256 messages defined won't they have the same number - and does this increase the chance of deciding that the sender/receiver match when they don't?

[hamishwillee]

Can we support omitting omit the CRC altogether? It isn't needed if running over a network that already does such checks - such as TCPIP.

[tridge]

yes, that could be an incompat flag, but saying it "isn't needed" ignores the way we use it to ensure both ends of the link agree on the message structure

[hamishwillee]

I've been thinking of the router in an IP network - it doesn't need to check the CRC_EXTRA because it doesn't care what the message is, and it doesn't need to check the CRC because the IP network does that underneath.

But of course the message still needs the CRC_EXTRA at the endpoints. A router in this circumstance would benefit from being able to avoid running any CRC checks at all.

[WickedShell]

I think we should strongly avoid dropping CRC's here. A corrupt partial packet can still be generated at the encoder, or network corruption (while rare) can actually happen. I'm not sure the bandwidth win is large enough to justify this.

[hamishwillee]

The CRCs are required in any case for the checks on the endpoint, so we can't remove them.

What would be good though is the ability to specify whether particular CRC checks are run. A router needs to be able to not test on the CRC_EXTRA so it can route without having updated messages. An endpoint should have the option to decide it does not want to run the full CRC if it thinks the link is already doing such checks. I think that the proposal addresses that.

[tridge]

one other thing I'd like to add is the ability to support self-describing messages, like we do for logging with ArduPilot
structure would be sent every N msgs or on request. Great for graphing/logging
example from ArduPilot logging:

    // @LoggerMessage: ATDH
    // @Description: Heli AutoTune data packet
    // @Vehicles: Copter
    // @Field: TimeUS: Time since system startup
    // @Field: Cmd: current motor command
    // @Field: TRate: current target angular rate
    // @Field: Rate: current angular rate
    // @Field: TAng: current target angle
    // @Field: Ang: current angle
    AP::logger().WriteStreaming(
        "ATDH",
        "TimeUS,Cmd,TRate,Rate,TAng,Ang",
        "s-kkdd",
        "F00000",
        "Qfffff",
        AP_HAL::micros64(),
        motor_cmd,
        tgt_rate_rads*57.3,
        rate_rads*57.3f,
        tgt_ang_rad*57.3,
        ang_rad*57.3f);

[hamishwillee]

one other thing I'd like to add is the ability to support self-describing messages, like we do for logging with ArduPilot
structure would be sent every N msgs or on request. ..

Cool idea, but I wouldn't let it block these other things.

[WickedShell]

The extra complexity about this doesn't really seem worth it to me. Is there a larger problem with just being looser about sys/comp id strictness, and at an application level just treating them as a single touple? (IE we already have 65535 possible unique addresses).

I'm guessing this may break something in software I haven't thought of from the top of my head, but on first glance I'd prefer to not add the extra responsibility to everything.

[olliw42]

first off, it is nice to see that the proposal in mavlink/mavlink#1347 for a "soft" upgrade of mavlink v2 to a v2B or v2.1 by using the incompatibility flag and adding the targets to the header has been taken over, which puts the distractive response which this proposal had received into proper perspective. However, it is astonishing to see that the header crc is not considered, and instead again this type of super-smart approach, which just will leave yet again a trail of issues and pain points, is proposed.

The arguments are:

• the error detection of a crc8 for messages of sizes as large as a maximum mavlink message is really poor. The "improvement" for routers by the split crc is thus low, and especially massively smaller as compared to a header crc8. The probability of incorrect relevant info like targets is significant. It's surprising that this downside doesn't raise concerns.
• a router needs to parse the complete message, all the way until its end before it can decide about the integrity of the header, and thus the integrety of relevant info like targets. This adds significant latency, which moreover adds up from each router, which would be minimal with a header crc.
• one of the common arguments for mitigating the terrible "false-negative" behavior of current mavlink is a backtracking parser (also mentioned twice in the above proposal). However, while this sounds great on paper the actual efficient implementation of such a parser is a pain, for the simple fact that a data packet of a given size may contain several and in fact many mavlink messages. It is hardly a coincidence that backtracking parsers haven't become standard. However, with a header crc, a backtracking parser becomes trivial to implement, simply because one now can rely on that there can't be any message contained in the size of a header. Thus, a backtracking parser could become a real-world reality, and offer much improved "false-negative" behavior to mavlink.
• the seq field has been proven to not be able to serve as a means to determine link performance, see Seq in message header is useless, and it should be denoted as such :) mavlink#1821. A simple solution, discussed there, would be to define the seq value per link, which however would require recalculation of the crc, which however is prevented by the (presumably unknown) extra crc. With a header crc this would be trivially resolved, and the sequence field could be given back a significant functionality, the possibility to determine lost mavlink packets. (this would also allow per-link data flow manipulations, which can be very desirable, but that's et another topic so I don't go deeper)

In simple words: It is plain silly to not add a header crc. And it obviously doesn't need any split crc voodoo. Frankly, the
only reason I can see to not add a header crc is stuborness and unwilligness to learn from past mistakes and insights.

I understand that my few cents won't make a difference, given the characteristic super-smartness they face, but I nevertheless add them, so that nobody can then say they couldn't have known better.

It is really sad that in ArduPilot/mavlink world it are not the best arguments which win.

[nexton-winjeel]

Another option is to use a variable length encoding for sys_ids greater than 255.

[nexton-winjeel]

Probably need to clarify that the split CRC (MAVLINK_IFLAG_SPLIT_CRC ) doesn't help unless the message also has the target in the header (MAVLINK_IFLAG_TARGETTED). If the CRC_EXTRA check fails, then the target_system and target_component can't be extracted from the message payload.

[hamishwillee]

Not ignoring, hoping that @tridge responds.