Gsof bitmask flexible parser

[Ryanf55]

Purpose

Refactor the GSOF parser to instead return a bitmask of which packets were parsed. Compared to before, where we used a counter for packets, which was not robust to duplicate packets, or flexible to different sets of packets. It now mirrors more commonly accepted ways of parsing such as in the inertial labs EAHRS by using a Bitmask to keep track of received packets.

The upcoming EAHRS uses this to keep track of the time_ms of each required packet, and only reports healthy when all tracked packets have arrived recently.

Details

• Removed unused debug macro
• Clean up docs
• The GPS driver's expected packets now has to be set at runtime, since you can't initialize an AP bitmask at compile time

Behavioral changes

The GSOF GPS driver expects packets 1,2,8,9,12 enabled.

Previously, if it received packets [1,1,1,1,1] all in a row, the parser would return success, which is terrible behavior because it's missing important data. Now, it fails. This is the main improvement.
Previously, if received packets [1,2,8,9,12, 18] (ie, unexpected unparsed data), the parser would fail. It still fails, but now we can easily change the behavior.

Because the driver is in control of configuration, we only expect data that was configured.

Next Up

The upcoming Trimble External AHRS will re-use this parser, but with different packets.
See https://github.com/ArduPilot/ardupilot/compare/master...Ryanf55:ardupilot:27033-gsof-add-eahrs-driver?expand=1 for a preview.

Testing Performed

• Autotest covers GSOF quite well, including configuration
• Run the SITL driver per the instructions in AP_GPS_GSOF.cpp and fly around. Ensure the EKF tracks as expected by observing a stable loiter.

Run the unit test, uncommenting the skip line.

ryan@ryan-thinkpad:~/Dev/ardupilot_ws/src/ardupilot$ ./build/sitl/tests/test_gsof
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from AP_GSOF
[ RUN      ] AP_GSOF.incomplete_packet
[       OK ] AP_GSOF.incomplete_packet (0 ms)
[ RUN      ] AP_GSOF.packet1
[       OK ] AP_GSOF.packet1 (0 ms)
[----------] 2 tests from AP_GSOF (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 2 tests.

[andyp1per]

It looks ok to me but the logic of the changes is a little hard to follow

[Ryanf55]

It looks ok to me but the logic of the changes is a little hard to follow

Thanks for the review! I'm open to changes to improve readability too - if you had any recommendations, I'm happy to implement.

[tpwrules]

The code looks okay. But it's not clear that the original problem exists, or that this solves it properly? The links have rotted but I looked through Trimble's documentation and this is what I surmise happens each "epoch" (assuming all the messages come at the same rate which we do configure):

1. Do GPS data processing
2. Build and concatenate the configured GSOF messages into a blob
3. Split the blob into 1-n GENOUT packets
4. Send all the packets

The documentation says "There can be various records in one GENOUT packet. There could be multiple GENOUT packets per epoch. Records may be split over two consecutive packets.". I'm further assuming that one "transmission number" is used for each time through this loop.

The current code assumes the above loop, but it only assumes 1 GENOUT packet. Can the transmitter put multiple messages of the same type into one packet? I assume the problematic case would fail because it should, according to my model, get 5 different GENOUT packets with 1 message each. The new code still assumes this model is true, because it won't work if the receiver chooses to send 3 messages in one packet and 2 in the next, and it also will have problems if the receiver chooses to send the 5 correct messages, then duplicate one of them again in the same packet.

This also will fall apart if the set of requested messages ever exceeds 248 bytes (one GENOUT packet). I think this needs a deeper rework. The memory overflows should be fixed too.

[Ryanf55]

New plan:

• Remove the code that passes in the expected packets -> ie void AP_GSOF::process_message()
• Move the configuration request into AP_GSOF (now or later)
• Consider wrapping the parser packets in ifdefs for which is needed for which by which driver
• Note - the new the new packets all have timestamps
• Need to confirm with Trimble - Records may be split over two consecutive packets Should be protect against this by adding a large buffer with max expected bytes? If we don't need to do this, the parser can likely remain very similar.

class GsofParser {
public:
     bool OnData(char c, ) // returns true if you got a message

    bool run_parser( MsgTypes& types_parsed)
    next_msg(&id, uint8 len) // uses packet_ptr to parse the buffer
    parse_gsof1(len, GSOF1& msg)
    parse_gsof2(len, GSOF2& msg)
    
   private:
        int* packet_ptr
    )
 }

[Ryanf55]

I've reworked the PR a bit to make it a bit simpler and functionally robust to extranneous data. I tested it by enabling a few extra packets, and verified the parser still is happy.

I also tested disabling one of the required packets, and it was no longer healthy as designed.

[andyp1per]

LGTM

[tridge]

prefer compile time assert if possible, happy to have new constructor for AP_Bitmask that takes const array

[Ryanf55]

Done!

[Ryanf55]

I've spend 45 minutes trying to get a constructor working so we can static assert the size. I can't get the Bitmask to have a constructor that can take in an array and then another statement call .count for a static_assert. Here's how far I got with the patch, but I haven't pushed.

There is no way a user will ever hit this because we have CI and unit tests will catch it before merge.

Are we ok merging as-is with the internal error? I could wrap it in defines only for SITL if we are worried.

[tpwrules]

I understood that the desire was to call static_assert on the array rather than bitmask.count(). You also don't necessarily need to add a constructor to AP_Bitmask, that could be done in a followup though.

[Ryanf55]

I understood that the desire was to call static_assert on the array rather than bitmask.count(). You also don't necessarily need to add a constructor to AP_Bitmask, that could be done in a followup though.

You can't do a static assert if you don't have a constexpr constructor, unless I'm missing something.

[tpwrules]

I think the idea was to keep the array and static_assert like they were before this PR, then add a little loop in the GSOF constructor which reads that array and sets up the bitmask.

[Ryanf55]

I think the idea was to keep the array and static_assert like they were before this PR, then add a little loop in the GSOF constructor which reads that array and sets up the bitmask.

Got it, sorry, was being dense. Take a look now.