-
Notifications
You must be signed in to change notification settings - Fork 17.2k
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
Publish SBP number of integer ambiguity hypotheses in GPS_RTK and GPS2_RTK mavlink messages #6424
Publish SBP number of integer ambiguity hypotheses in GPS_RTK and GPS2_RTK mavlink messages #6424
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had time to look at it properly yet, but a couple of things stood out on a quick skim:
-
Why maintain a separate RTK state struct, instead of just keeping the required fields in the GPS state struct. The addition of the struct (and passing it to every GPS backend) has created a lot of noise in the PR for now value I can see. (It looks like 50% of this PR is just tweaking the constructors to correctly pass the extra struct around)
-
Fields that aren't going to be update (about half of the fields in the RTK state struct it appears) shouldn't be in the struct. If we aren't going to use them they shouldn't be eating up space, and we can just hard code sending 0's for those fields.
All the other inline comments are pretty small nitpick/style things, but I think these first 2 issues do need to be addressed.
libraries/AP_GPS/AP_GPS_ERB.h
Outdated
@@ -66,6 +66,13 @@ class AP_GPS_ERB : public AP_GPS_Backend | |||
uint8_t fix_type; | |||
uint8_t fix_status; | |||
uint8_t satellites; | |||
// Introduced in ERB version 0.2.0 | |||
int8_t numLeapSeconds; ///< leap seconds (0xFF indicates invalid) | |||
int32_t baselineN__mm; ///< distance between rover and base - North - in millimeters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the double underbar? This actually makes it harder to type/interact with IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a units separator. It allows one to do "inertial_speed__cm_p_s" (centimeters per second) It separates variable name from units name, because variable name can contain _ and units can also contain a _. So using a double __ makes it clear where the separation is. This example uses pretty simple units.... but there are complicated units out there. This is a standard code guideline at the company that I work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My complaint with the approach is that it is introducing an approach that isn't used within the entire rest of the code base, and I don't see how inertial_speed_cm_p_s is any harder to decompose into what the units are. At the moment with the current style guide as well I'd take it as an error, however if others are happy with the proposed syntax I'll shut up and move out of the way (preferably if someone also updates the style guide).
@OXINARF @peterbarker @rmackay9 Any opinions on the usage of a double underbar _ separating units from the rest of the var name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. On one way, it looks ugly and harder to code using it, on the other I do think it allows an easier view of the units. I'm pending more against it though as I don't think the benefit outweighs the disadvantages.
libraries/AP_GPS/AP_GPS_ERB.cpp
Outdated
@@ -189,6 +189,16 @@ AP_GPS_ERB::_parse_gps(void) | |||
state.time_week_ms = _buffer.stat.time; | |||
state.time_week = _buffer.stat.week; | |||
} | |||
if ( _buffer.ver.ver_high > 0 | |||
|| (_buffer.ver.ver_high == 0 && _buffer.ver.ver_medium >= 2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole expersion would be valid as ( _buffer.ver.ver_high > 0 || _buffer.ver.ver_medium >= 2)
there is no need to check for ver_high == 0
as a uint8_t must be in order to hit this part of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I was coding for a more general case like version 2.1 vs. version 1.9.
But you are correct it can be simplified in this particular case.
libraries/AP_GPS/AP_GPS.h
Outdated
uint8_t rtk_rate; ///< Rate of baseline messages being received by GPS, in Hz | ||
uint8_t nsats; ///< Current number of sats used for RTK calculation. | ||
uint8_t baseline_coords_type; ///< Coordinate system of baseline. 0 == ECEF, 1 == NED | ||
int32_t baseline_a_mm; ///< Current baseline in ECEF x or NED north component in mm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nitpick: Regardless of the mavlink name from a code perspective can we stick with calling these baseline_x_mm (X/Y/Z rather then A/B/C)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like the a, b, c as well. I'll change that.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused data is still unused data. Given that there is no required ordering, I think adding it to the state struct later when you actually need the fields is absolutely fine (and shouldn't pose any challenge), but the presence of unused fields poses a conundrum for future readers, as they must determine if the field is still needed, and determining it isn't in use I would expect to see a PR come in that removes the field. (This has happened in the past).
I will say until I saw this PR I thought the GPS RTK mavlink stuff was actually being sent, and realizing it wasn't made me immediately want to delete all the associated (and wasted) code :) (Which obviously we won't do now that you are actually using it)
libraries/AP_GPS/GPS_Backend.cpp
Outdated
rtk_state.baseline_c_mm, | ||
rtk_state.accuracy, | ||
rtk_state.iar_num_hypotheses); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should ensure that the requested instance is instance 1. (I can see that at a future data someone might try and refactor this to just be for all gps backends call send gps rtk data)
libraries/AP_GPS/AP_GPS_ERB.h
Outdated
@@ -66,6 +66,13 @@ class AP_GPS_ERB : public AP_GPS_Backend | |||
uint8_t fix_type; | |||
uint8_t fix_status; | |||
uint8_t satellites; | |||
// Introduced in ERB version 0.2.0 | |||
int8_t numLeapSeconds; ///< leap seconds (0xFF indicates invalid) | |||
int32_t baselineN__mm; ///< distance between rover and base - North - in millimeters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My complaint with the approach is that it is introducing an approach that isn't used within the entire rest of the code base, and I don't see how inertial_speed_cm_p_s is any harder to decompose into what the units are. At the moment with the current style guide as well I'd take it as an error, however if others are happy with the proposed syntax I'll shut up and move out of the way (preferably if someone also updates the style guide).
@OXINARF @peterbarker @rmackay9 Any opinions on the usage of a double underbar _ separating units from the rest of the var name?
I only gave a brief look at the PR, but why have the RTK state be part of the frontend at all? Looks like a waste of space, when it is something that can be saved only in the specific drivers that support sending these messages. By the way, why does the backend method get the instance number? It already knows what instance it is. |
@OXINARF RTK state will be a part of SWIFT, ERB and probably a couple of other backends. That will duplicate the code, hence added to the base class. Do you want me to move it from AP_GPS into GPS_Backend ? I missed that the backend can access instance number via |
Oh man, I'd be a multimillionaire if I got 1€ for every time I've read that something will be done later 😜 |
8564d08
to
4b340f6
Compare
I've addressed all the comments and improved some stuff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still maintain my previous comment that it looks a waste of space to have all this info in the frontend just for one or two drivers, but like I said before, it is up to @WickedShell what to do.
Why are the variables and methods all called dpgs? Isn't this RTK information?
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty line
libraries/AP_GPS/AP_GPS_ERB.cpp
Outdated
state.horizontal_accuracy = _buffer.pos.horizontal_accuracy * 1.0e-3f; | ||
state.vertical_accuracy = _buffer.pos.vertical_accuracy * 1.0e-3f; | ||
state.horizontal_accuracy = _buffer.pos.horizontal_accuracy * 1.0e-3f * 0.83f; // multiply by 0.83 to convert RMS value into CEP value | ||
state.vertical_accuracy = _buffer.pos.vertical_accuracy * 1.0e-3f * 0.83f; // multiply by 0.83 to convert RMS value into CEP value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they both RMS? Even the horizontal one? The protocol PDF doesn't say anything about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is part of the problem. Every manufacturer has a slightly different way of calculating this. But it seams that CEP is the most common one. AFAIK uBlox uses CEP as well. The RTKLIB source code calculates RMS values. So in order to make these things comparable convert RMS to CEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the source code for our device drivers it seams that RMS is actually more common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RTKLIB source code calculates RMS values. So in order to make these things comparable convert RMS to CEP.
I'm confused. The ERB protocol PDF file doesn't mention if this is CEP, RMS, 2DRMS, etc., so how do you know what it is? I assumed you got it directly from Emlid, but you talk about RTKLib output values...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, THAT is the problem our code does not mention this as well. But as you know comparing Potatos with oranges does not make any sense. Hence my quest to add unit information all over the place. OK, now back to topic...
Emlid reach uses the open source library RTKLIB, and I know the place in the code where this calculation gets done, and it is RMS. So ... no documentation, but source code is available that is how I know it.
This patch adds RMS in some places in our device drivers were I checked the .pdfs to see what kind of values was produced.
c556a6c
to
72219bc
Compare
Rebased and cleanup more, Added minimal Novatel, SBF and SBP support |
72219bc
to
ecb8976
Compare
Minimized the patch to simplify review process |
Maybe this is foolish question, but I do not understand why do we need to send this info from AP at all? |
A good question. In ERB for instance it increases the range of the vehicle, because no WLAN is needed to monitor if all is OK, And yes, with ERB things go wrong a lot of times :( |
Can you please be more specific on real benefits that we get with this? |
I need to know how much I can trust the GPS at any given point. That data is available inside the receiver. One needs to know if the correction data is actual or outdated. One needs to know the exact position of the rover respecticve to base to validate if the GPS solution converged correctly, One needs to know the AR ratio in order to know if one can trust the GPS position (it is more fine granular than the fix status) This patch will not use any bandwidth unless a RTK GPS is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I'm fine with it, I can see massive value to the operator side, and there are a number of other previous cases of only one GPS doing something that would be optimal (only u-blox audits it's configuration and rates for example). The information has value, and it paves the path to other backends implementing it.
I have not reviewed the actual information extracted from ERB, I don't seem to have the reference sheet for that tucked away anywhere. I assume this has been tested on real hardware?
Other then that I made some comments below that should be addressed.
libraries/AP_GPS/AP_GPS.h
Outdated
int32_t rtk_baseline_x_mm; ///< Current baseline in ECEF x or NED north component in mm | ||
int32_t rtk_baseline_y_mm; ///< Current baseline in ECEF y or NED east component in mm | ||
int32_t rtk_baseline_z_mm; ///< Current baseline in ECEF z or NED down component in mm | ||
uint32_t rtk_accuracy; ///< Current estimate of baseline accuracy (receiver dependent, typical 0 to 999.9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a unit and dimension in the comments? My first guess would have been meters, but that wouldn't make sense in a uint32_t, and the 999.9 in the comments is unrepresentable in a uint32_t so I have no clue what unit this is expected to be in.
We also need to comment if this is considered the 2D or 3D accuracy? I'm guessing it's the 3D one, but without a comment that's an unfounded suspicion.
I realize these complaints are partly because the MAVLink spec wasn't clear enough, but we should do better on our internal documentation (and send a PR upstream to improve the comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES, yes, and yes. Agree with all.
libraries/AP_GPS/AP_GPS_ERB.cpp
Outdated
@@ -189,6 +189,20 @@ AP_GPS_ERB::_parse_gps(void) | |||
state.time_week_ms = _buffer.stat.time; | |||
state.time_week = _buffer.stat.week; | |||
} | |||
if ( _buffer.ver.ver_high > 0 | |||
|| _buffer.ver.ver_medium >= 2) { | |||
state.rtk_baseline_coords_type = 1; // baseline uses NED coordinates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No hold up on merging this, but it would be great if we had this as a MAVLink enum instead of having to read the comments for the values 0/1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will add that in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, ArduPilot/mavlink#48
libraries/AP_GPS/GPS_Backend.cpp
Outdated
{ | ||
const uint8_t instance = state.instance; | ||
// send status | ||
if (instance == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a switch instead, there is a bit of a long term desire to support more then just 2 GPS instances, which will result in all the additional GPS's being reported as gps2_rtk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
gps.send_mavlink_gps_rtk(chan); | ||
} | ||
|
||
if (HAVE_PAYLOAD_SPACE(chan, GPS_RTK)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only send this if there is a GPS instance to minimize telemetry consumption for someone operating without an RTK capable GPS.
I'm also on the fence about the removal of the highest_supported_status() check, as it makes no sense to send the message to a GCS if the GPS can't support a RTK fix. I realize that at this point we almost have more instances that can then can't (particularly since u-blox is now capable of it), but it would be good to minimize messages that aren't applicable.
In fact my biggest hesitation on this whole PR is that it increases the link budget a bit for the most common use case. However short of adding a helper that says has_mavlink_rtk_payload()
I don't see a huge advantage. (And the helper would be messy, so I guess I'm just pointing it out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That check is done in the driver code already, hence the removal of a duplicated check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh! Sorry your right, I thought for some reason you had left it on GPS2 here, which is why I was pointing at this... I appear to be color blind before my first cup of coffee....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have the sending of this data to be disabled through parameters?
Maybe flag or bitmask value?
I think that even when using RTK, not everybody will need this data on telemetry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plesa take a look at ArduPilot/pymavlink#88 . Does it really hurt you to have GPS2_RTK active when you have a RTK GPS ?
I do not think that a enable parameter is needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check could be raised to only send the information when the status is > DGPS, (I realize this drops the message for the case of actually trying to operate DGPS only, but I think thats acceptable.
Knowledge is on ground is good, but what will happen then? |
@EShamaev from a photogrammetry perspective if I'm relying on the RTK link for my positioning and that impacts the end result it determines if I continue flying, or if I cancel and fix a problem. Better to find out immediately then fly for 3 hours and have problems after landing, especially when the information was already available to the autopilot. |
Ah, OK. |
The information extracted from ERB is not on a datasheet. I did that myself and did a pull request for them. Yes, it is tested in Emlid Reach hardware. |
Adressed your comments. But please merge #6534 before this one. |
libraries/AP_GPS/GPS_Backend.cpp
Outdated
@@ -190,7 +190,8 @@ void AP_GPS_Backend::send_mavlink_gps_rtk(mavlink_channel_t chan) | |||
{ | |||
const uint8_t instance = state.instance; | |||
// send status | |||
if (instance == 0) { | |||
switch (instance) { | |||
case 0: | |||
mavlink_msg_gps_rtk_send(chan, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to be the style obsessed person but either everything (including the break) needs to be tabbed in one more so that it's in from the case, or the case needs to be untabbed one. http://ardupilot.org/dev/docs/style-guide.html#case-statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, but github has not refreshed yet.
268801c
to
8b1c39c
Compare
ERB moved to another PR. This can go in, regardless if/when Emlid merges my stuff or not |
@amilcarlucas please don't split PR's apart this aggressively, it's not simplifying the review process or merge time at all, as everytime you resplit the PR both resulting PR''s must be entirely reviewed again, and in this case it makes this PR look much more pointless without the ERB support. To do a review of this the reviewer must also reference the other PR in tandem. |
This one can be improved once the mavlink Patch gets merged and the mavlink dependency updated. |
8b1c39c
to
746ab04
Compare
Niels can you add support for the other fields as well ? and to SBP2 too ? Please add something like:
But take care, some of them might already be there. |
@njoubert ping. |
746ab04
to
022ae07
Compare
Rebased |
…with a flexible one
…tion with a flexible one
022ae07
to
a43cf7d
Compare
rebased again. |
Wrong button on review didn't actually review just now, hit the tab the wrong way. Closing on the principle that #6858 contains this and more, and that is the PR that is moving forward. If that's not true then this can be reopened at that time. |
Looks like it has not changed since 19.07.2017. It was meant to be merged before EMLID merged this feature into their repository. They merged their side on 25.08.2017 emlid/RTKLIB@6a70f6e |
Add IAR information to SBP (Swift Binary Protocol). ERB support moved to #6541 .
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 #6541 ArduPilot/mavlink#48 ArduPilot/mavlink#49 ArduPilot/pymavlink#88 emlid/reach-docs#26 emlid/RTKLIB#21 emlid/RTKLIB#28