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.

Sign up for GitHub

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

Jump to bottom

added rough mavlink 2.1 proposal #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added rough mavlink 2.1 proposal #20

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions text/mavlink2_1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
* Start date: 2022-10-12
* Contributors: Andrew Tridgell

# Summary

Rough proposal for MAVLink 2.1

# Motivation

Key features wanted:

* support for more than 256 system IDs
Copy link

Choose a reason for hiding this comment

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

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.

* support for checking CRCs in routers that don't have all xml
* better target_system/target_component support
* maximum compatibility with existing mavlink2 systems

# Detailed Design

The design takes advantage of the incompat_flags and compat_flags
fields in the MAVLink2 header.

New incompat_flags flags would be:

* MAVLINK_IFLAG_SYSID16 : system ID in header is 16 bits wide
tridge marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

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

* MAVLINK_IFLAG_TARGETTED : header has target_system and target_component appended
* MAVLINK_IFLAG_SPLIT_CRC : 16 bit crc is as described below

New compat_flags flags would be:

* MAVLINK_FLAG_21_CAPABLE : sender is capable of handling MAVLink 2.1

Note that for the SYSID16 and TARGETTED flags the sender should only
use the flag if is it needed. So if the systemID in the message is
<=255 then the flag should not be set and the existing 8 bit field
tridge marked this conversation as resolved.
Show resolved Hide resolved
should be used. For the TARGETTED bit this should only be used if the
target systemID is >255 (needing 16 bits) or if the message that needs
targetting doesn't have a target_system or target_component field.

Following these rules maximises compatibility with existing MAVLink2
capable systems.

Note that when SYSID16 flag is set then the target_system would be 16
bits when TARGETTED is set.

For messages with existing target_system/target_component fields, if
the TARGETTED bit is set then the existing fields are ignored by the
recipient. The sender should set the old fields to zero.
Comment on lines +45 to +47
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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".


The 16 bit size of the expanded systemID could perhaps be 32 bits,
allowing for an IPv4 address to be used, which could be useful for
many use cases. The target audience is drone light shows which often
use WiFi.

## Split CRC
Copy link
Collaborator

@hamishwillee hamishwillee Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@hamishwillee hamishwillee Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

The idea of the split CRC is to address a concern from people making
Copy link

@nexton-winjeel nexton-winjeel Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not ignoring, hoping that @tridge responds.

MAVLink routing tools that they can't CRC new messages that they
didn't have the XML for at the time they built the router firmware.

The solution is to do what we should have done for MAVLink 1.0, which
is to make 8 bits of the 16 bit CRC depend on the structural seed, and
8 bits not depend on the seed.