generator/C: Don't parse messages outside CRC_EXTRA #990
Conversation
|
Any comment @peterbarker? FYI @auturgy |
|
@peterbarker Is probably at the conference and @auturgy is away this week. So probably they won't get back to you this week. |
|
Bumping this. I would appreciate a comment @tridge or @peterbarker. |
|
I put a message to get a review on next call next week |
|
@julianoes we need to test that this is OK with a router that wants to pass through unknown messages. That is an important use case |
| mavlink_update_checksum(rxmsg, crc_extra); | ||
| if (c != (rxmsg->checksum & 0xFF)) { | ||
| if (e == NULL) { | ||
| // Message not found in CRC_EXTRA table. | ||
| status->parse_state = MAVLINK_PARSE_STATE_GOT_BAD_CRC1; |
There was a problem hiding this comment.
you also need to set rxmsg->ck[0] to the byte from the packet (c) so that this code works correctly for forwarding the message:
if (status->msg_received == MAVLINK_FRAMING_BAD_CRC) {
/*
the CRC came out wrong. We now need to overwrite the
msg CRC with the one on the wire so that if the
caller decides to forward the message anyway that
mavlink_msg_to_send_buffer() won't overwrite the
checksum
*/
if (r_message != NULL) {
r_message->checksum = rxmsg->ck[0] | (rxmsg->ck[1]<<8);
}
}
There was a problem hiding this comment.
I see, so you can still forward even if you can't fully parse a message. Do you have an example of such an implementation?
There was a problem hiding this comment.
There was a problem hiding this comment.
Got it, thanks. That's interesting.
I will update as suggested to fix that case.
|
@tridge thanks for the review!
I think a router generally should not parse a message. If it did try to parse every message, it wouldn't forward unknown messages as it doesn't have the correct CRC_EXTRA. So the way to forward messages is to just look for a correct looking header. |
A router could choose to forward a message that has a "bad CRC" and use the MAVLink library as a convenient way to decode the framing. |
There was a problem hiding this comment.
Please rebase on master then squash to one commit and we will merge!
I checked and it looks like the sample data (thanks for sharing) did get through because the CRC happened to be correct if CRC_EXTRA was 0. I think it makes sense to flag a message we know we can't prove the CRC is correct as having a bad CRC, then let the consumer deal with it.
It looks like this might not interact correctly with signing so a forwarder might have trouble there but that's a separate pre-existing problem and not in scope for this PR.
I have been wondering for a while why I sometimes see MAVLink
components "appear" with random sysid/compid when I leave a system
running for some time.
Today I finally caught the bytes of such a case, the offending bytes
were:
{253, 0, 1, 0, 0, 3, 1, 1, 42, 0, 185, 186,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
This is msgid 10753 from sysid 3, compid 1 with payload len 0.
It's presumably signed, hence the trailing zeros.
It turns out the msgid is way out of the CRC_EXTRA table and hence the
mavlink_get_msg_entry(msgid) function returns NULL.
In that case 0 is ignored and we carry on parsing.
My suggestion is to reset parsing when we don't know the CRC_EXTRA of
the message to avoid flagging noise as a valid message.
Included fixup:
In case a message is forwarded (even with unknown/bad CRC) we need to
copy the first CRC byte.
julianoes
force-pushed
the
pr-fix-msg-without-crcextra
branch
from
9ab6354
to
2ab791b
Compare
I have been wondering for a while why I sometimes see MAVLink components "appear" with random sysid/compid when I leave a system running for some time.
Today I finally caught the bytes of such a case, the offending bytes were:
{253, 0, 1, 0, 0, 3, 1, 1, 42, 0, 185, 186, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};This is msgid 10753 from sysid 3, compid 1 with payload len 0. It's presumably signed, hence the trailing zeros.
It turns out the msgid is way out of the CRC_EXTRA table and hence the mavlink_get_msg_entry(msgid) function returns NULL. In that case 0 is ignored and we carry on parsing.
My suggestion is to reset parsing when we don't know the CRC_EXTRA of the message to avoid flagging noise as a valid message.