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

PGN_200 and PGN_202 have fixed "CRC" checksum of 71 #546

Open
Pynee opened this issue Jan 6, 2025 · 6 comments · May be fixed by #585
Open

PGN_200 and PGN_202 have fixed "CRC" checksum of 71 #546

Pynee opened this issue Jan 6, 2025 · 6 comments · May be fixed by #585
Assignees

Comments

@Pynee
Copy link

Pynee commented Jan 6, 2025

PGN_200 and PGN_202 packets don't calculate the checksum and it's fixed to a constant value 71.

@Pynee
Copy link
Author

Pynee commented Jan 13, 2025

Yeah those are from the Arduino firmware so I'm not sure if AgOpenGps is checking checksum when receiving packets from the boards. AIO Teensy code has similar problems when I looked through its code. It has hard coded checksums on those same packets(Excluding PGN_123 as there is no machine code) and it doesn't do any checksum checking on received packets either.

PGN_121 and PGN_126: https://github.com/AgOpenGPS-Official/Boards/blob/a73d617000ac951aa77909b61ba317d0ccdbe1df/TeensyModules/AIO%20v4%20RVC%20Firmware/AIO_v4_Firmware/Autosteer.ino#L77-L78
and UDP receive code: https://github.com/AgOpenGPS-Official/Boards/blob/a73d617000ac951aa77909b61ba317d0ccdbe1df/TeensyModules/AIO%20v4%20RVC%20Firmware/AIO_v4_Firmware/Autosteer.ino#L554

But at least on PANDA sentences checksums are checked on AgOpenGPS side as I have tested.

I don't know how critical these checksums really are at least on the wired connections but I thought to better to report them.

@Pynee
Copy link
Author

Pynee commented Jan 13, 2025

If some one needs a checksum calculator function for the firmware you can use this.

uint8_t calculateChecksum(uint8_t *packet, int length,int startPosition = 2)//startPos is usually 2
{
    uint8_t CK_A = 0;
    for (int i = startPosition; i < length-1; ++i)//this automatically excludes the last byte that is used for the checksum so we can use packet length as the stopPos
    {
        CK_A = (CK_A + packet[i]);
    }
    return (uint8_t)CK_A;//returns the checksum byte to be used to compare it against the received packet or to add it to be sent packet
}

Although it could be better if we used a real CRC calculation for the checksum for a better error detection.

@richardklasens
Copy link
Collaborator

Correct we don't check for checksums for our hardware, not on hardware. Not on software, it's just not fully implemented yet. If we do, we need to make sure everyone need to update their hardware so it will be a major thing. It's not planned for now. There are some other major things which needs more attention now. Hopefully this will explain it for now.

@Pynee
Copy link
Author

Pynee commented Jan 14, 2025

Yeah, that's true. But it's possible to fix those hard coded check-sums on the sending side to start slowly converting the software/firmware towards error checking and add error checking on the receiving side later after most of the people have updated their software/firmware to send calculated checksums.

@hagre
Copy link
Contributor

hagre commented Jan 14, 2025

Yeah, that's true. But it's possible to fix those hard coded check-sums on the sending side to start slowly converting the software/firmware towards error checking and add error checking on the receiving side later after most of the people have updated their software/firmware to send calculated checksums.

On my new hardware (still in development) I am doing this hybrid thing to make a “non breaking” transition possible.
Calculating the correct CRC, but for comparing to the received CRC it is using both ( calculated CRC || 71 ) to accept the data as true.
That is no real good solution, but a possible way ahead for transition the AOG output one day (e.g Version 7.x).

@richardklasens richardklasens linked a pull request Jan 18, 2025 that will close this issue
@richardklasens richardklasens moved this from Todo to In Progress in develop Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

5 participants