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

Support RTCM injection to UAVCAN GPS #12612

Merged
merged 11 commits into from Nov 9, 2019

Conversation

@tridge
Copy link
Contributor

tridge commented Oct 21, 2019

This adds RTCM injection for AP_Periph based UAVCAN GPS modules, and for ArduPilot to send the new GNSS Inject message
ping @jschall

@tridge tridge added GPS CAN labels Oct 21, 2019
@tridge tridge force-pushed the tridge:pr-uavcan-rtk-gps branch from 90b0910 to 18f979b Oct 21, 2019
@tridge tridge added the DevCallTopic label Oct 21, 2019
@tridge tridge requested a review from WickedShell Oct 21, 2019
@jschall

This comment has been minimized.

Copy link
Contributor

jschall commented Oct 23, 2019

Can we please use the RTCM message included in the uavcan.equipment.gnss namespace?

I have already implemented support for that in Here Pro.

@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Oct 23, 2019

@jschall the one in the existing gnss namespace has 2 problems:

  • it doesn't readily distinguish between RTCM out from a GPS and RTCM being injected into a GPS by the ground station
  • it doesn't handle the fragmentation we need to use the MAVLink GPS_RTCM_DATA message, which sends up to 4*180 bytes, and handles out of order pkts
@proficnc

This comment has been minimized.

Copy link
Contributor

proficnc commented Oct 23, 2019

@jschall , correct me if I’m wrong, but on Can, out of order packets are not even slightly of concern to us...

And inbound or outbound is also irrelevant as can is not a master slave bus...

@jschall

This comment has been minimized.

Copy link
Contributor

jschall commented Oct 23, 2019

If the stream is coming from a node known to be a GPS, it is coming out of a GPS. Otherwise it is probably coming from the ground. IMO, consumers of RTCM streams should not just pick up random RTCM streams from the bus. They should know to expect RTCM from some specific node that the user will configure.

Out of order packets are not a concern with UAVCAN.

@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Oct 23, 2019

the seq number matters as RTCM blocks for multi-constellation GPS can be bigger than 180 bytes. So they don't fit in one of the RTCMStream msgs. As we don't want to actually have RTCM parsing in either ArduPilot or inside the CAN node we need to try to ensure that a full RTCM block gets into the GPS, and not a partial one which leads to the GPS modules parser getting out of sync. That is why the MAVLink msg has a seq number - it tries to ensure we give complete RTCM blocks to the GPS without us having to actually parse the RTCM stream.
That is why we have the re-assembly code inside AP_GPS. See handle_gps_rtcm_data(). It uses the flags in GPS_RTCM_DATA to ensure we give a complete block to the GPS.
We cannot guarantee this will happen when we use the existing UAVCAN msg.

consumers of RTCM streams should not just pick up random RTCM streams from the bus.

we could certainly have a parameter on the CAN GPS node which says which node it will accept the RTCM data from. I think for average users the default should be to accept from any however. We want NTRIP/RTK to be easy for the average person to use. Right now they plug a UART GPS into their vehicle, press the button in MissionPlanner and it all works like magic. The user experience for UAVCAN should be the same. If the user is more advanced then setting controls in the CAN node to set how it handles RTCM streams would be nice, especially for things like moving baseline.

@jschall

This comment has been minimized.

Copy link
Contributor

jschall commented Oct 23, 2019

RTCM parser is not complicated and may be useful for other things later.

This is all already done on Here Pro. Happy to send you the RTCM parser code.

@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Oct 23, 2019

RTCM parser is not complicated and may be useful for other things later.

that is just another thing to maintain, especially for different RTCM versions, plus adds complexity to CAN nodes and ArduPilot and likely would be too much flash for the f103 based GPS nodes.
It really should be pretty easy for you to support this new Inject msg in the pro.
We went through this same process in MAVLink. We had the old GPS_INJECT_DATA msg, and when we found the issues with it we created a new GPS_RTCM_DATA msg which solved the problem.

@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Oct 23, 2019

See more discussions here:
#4945

Copy link
Contributor

WickedShell left a comment

I take it a larger CAN message for sending this isn't feasible? (IE can we end up with a single message that doesn't need to be fractured so often).

A reassembly mechanism does make sense as you can otherwise lose 2 packets of correction data instead of just one, although I don't think we should be using the MAVLink flags directly, that was a bad hack done to cope with messages.

@tridge tridge added the DevCallTopic label Oct 28, 2019
@tridge tridge removed the DevCallTopic label Nov 5, 2019
tridge added 5 commits Nov 6, 2019
this allows for much more reliable CAN comms in AP_Periph
the Drotek F9 GPS ships with TMODE enabled, which means we don't get a
3D fix.

This also adds the VALGET/VALSET msgs we will need for automatic
moving baseline config
@tridge tridge force-pushed the tridge:pr-uavcan-rtk-gps branch from 18f979b to cf86ab7 Nov 6, 2019
@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Nov 6, 2019

after testing both ways I've ended up going back to RTCMStream as it uses less memory on the can node, and is easier to do rate limiting on the bus, which is needed for the f103

tridge added 2 commits Nov 6, 2019
@tridge tridge force-pushed the tridge:pr-uavcan-rtk-gps branch from cf86ab7 to 5d4dbab Nov 6, 2019
@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Nov 6, 2019

@jschall

This comment has been minimized.

Copy link
Contributor

jschall commented Nov 6, 2019

Why would there be a race condition? Redundant uavcan works just like non-redundant uavcan...

@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Nov 6, 2019

@jschall So the problem is if one of the buses is busier and the CAN message has to be retransmitted/lags. The GPS driver only has one reassembly buffer, so if item 1 fragment 2 is delayed on one of the buses but the other bus starts on item 2 fragment 1 then we discard the partial data in the driver. Then item 1 fragment 2 comes in on the second bus we discard the buffer of item 2 again. Effectively this can lead to dropping 2 messages, instead of just one message. If you send a single large message that gets directly injected to the GPS then you can avoid this scenario (although now you need to manage the fact that you got feed the same data twice and not shove it both times).

@jschall

This comment has been minimized.

Copy link
Contributor

jschall commented Nov 7, 2019

The second bus would be transmitting in-order, so the messages will arrive in-order and the delayed messages on the first bus will be ignored as they have already been received on the second bus.

@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Nov 8, 2019

@WickedShell I think @jschall is right, but I don't actually have a dual-CAN GPS here to test with (well, I do, the ZubaxGNSS, but I haven't enabled the 2nd CAN port yet in that port)

@tridge

This comment has been minimized.

Copy link
Contributor Author

tridge commented Nov 8, 2019

@jschall have you been able to test this branch with your work on the pro?

@tridge tridge merged commit 8aa7812 into ArduPilot:master Nov 9, 2019
4 checks passed
4 checks passed
ArduPilot.ardupilot Build #20191106.21 succeeded
Details
ArduPilot.ardupilot (Cygwin SITL build) Cygwin SITL build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Plane 4.0 Backports
  
Awaiting triage
6 participants
You can’t perform that action at this time.