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

Add GPS_RTK and GPS2_RTK MAVLink messages support in Emlid reach #6541

Conversation

amilcarlucas
Copy link
Contributor

@amilcarlucas amilcarlucas commented Jul 5, 2017

This requires #6534 and #6424 PRs in that order

This is just a small part of it (been working and testing this since May 9th) , the rest of the related work is at #6534 #6424 ArduPilot/mavlink#48 ArduPilot/mavlink#49 ArduPilot/pymavlink#88 emlid/reach-docs#26 emlid/RTKLIB#21 emlid/RTKLIB#28

@rrr6399
Copy link
Contributor

rrr6399 commented Jul 11, 2017

Any chance of this pull request getting merged soon? These changes are very important for RTK users to help understand the solution progress of the RTK in terms of the AR ratio and the relative distance from the base.

The changes will hopefully prompt the updates to support the M8P and other RTK GPS's as well.

@WickedShell
Copy link
Contributor

@amilcarlucas Please don't fracture PR's like this into so many parts. It's very difficult to review when presented with a pile of PR's that are interdependent like this, but by themselves can't be merged. It's made worse when as presented you don't even have the later PR's based off the previous ones.

For example #6534 doesn't make any sense to bring in until a patch that also does something with the work is present. If you could have this patch based on that so we can handle it as a single item then it can be reviewed decently. But I'm finding it difficult to actually trace down these interactions as is. For example I can't actually find anywhere you use rtk_age_ms, so all the storage of it is pointless. But I might just not be able to find the PR that does work with it.

The effort of reviewing these PR's on my side is stalled for this reason at the moment.

@amilcarlucas
Copy link
Contributor Author

amilcarlucas commented Jul 17, 2017

Michael, #6534 is a standalone patch. It does make some work already and does not depend on this PR.

Please aprove and merge that one first. It will cause merge conflicts that I need to solve before getting the other PRs in.

The division has to do with me being shure that you guys complain about Emlid stuff. So having a monolithic patch that never goes in makes no sense to me. hence the division.

@amilcarlucas
Copy link
Contributor Author

Rebased to solve conflicts

@egorf
Copy link

egorf commented Jul 20, 2017

Hey guys,

Reach dev here. We've discussed @amilcarlucas's work with our ArduPilot team, mainly @staroselskii and @AlexeyBulatov. Seeing Amilcar port MAVLink messages to ERB makes us think that instead of supporting and extending ERB, it would be much more sane to use MAVLink.

Is this move something ArduPilot team wants to see from external devices like Reach, or is it a bad idea?

@amilcarlucas
Copy link
Contributor Author

I think it is not a good idea. How do we configure the GPS ?
On Emlid there is no need for configuration, but on other GPSs we need that.
And to force other GPS manufacturers to support Mavlink is not really possible.

So I think you should simply merge my pull request emlid/RTKLIB#21 and improve on top of that :)

But let's wait for what @WickedShell thinks about this

@WickedShell
Copy link
Contributor

@egorf It would probably turn into supporting custom MAVLink messages quickly. While it gives you a subset of information you end up having to live inside a lower priority task, and it's harder to maintain the needed information in it.

For example something that is great to see from a GPS driver is an estimate of it's internal processing lag, (see https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_GPS/AP_GPS_UBLOX.cpp#L1312 and https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_GPS/AP_GPS_SBF.h#L47 ). This data helps the EKF create a better result.

There also isn't a paved code in ArduPilot for accepting 2 different GPS_INPUT messages, which would result in problems with having 2 GPS's on the aircraft. This is of course solvable, but it's not actually going to reduce the work required for really properly supporting your GPS.

You also would have to accept the limit of 7 decimal places on lat/lon support as thats all the message offers, which if/when we support a higher level of precision inside the autopilot would then become a problem.

Overall I'd recommend sticking with the approach of maintaining the ERB format over using the MAVLink one.

@egorf
Copy link

egorf commented Jul 25, 2017

@WickedShell Thanks for your input!

@amilcarlucas amilcarlucas force-pushed the pr-GPS_RTK-and-GPS2_RTK-emlid-reach-support branch from a02c1ff to 03c47be Compare August 21, 2017 14:37
@amilcarlucas
Copy link
Contributor Author

amilcarlucas commented Aug 21, 2017

Rebased on master to solve conflicts. I still think that it is better to expand an existing message, then to create a new message with more overhead, and no added value. It will just slow things down by forcing the ardupilot to parse and validate an extra message header.

@amilcarlucas
Copy link
Contributor Author

#6858 is based on this one, but adapted to the Emlid official 2.7.4 FW release.

Closing this one now

@amilcarlucas amilcarlucas deleted the pr-GPS_RTK-and-GPS2_RTK-emlid-reach-support branch October 25, 2017 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants