Vector NAV GPS driver #5459
Vector NAV GPS driver #5459
Conversation
this driver is broken and could cause a vehicle to crash. It does busy waits on a UART from the main thread. It will be re-enabled when fixed
It's not needed and sometimes waf.io is offline, leaving people with the impression they need to read it for compiling.
fixed PPM R/C input on pixracer
…ith VN 200 module.
There was a problem hiding this comment.
I'm actually very excited to see someone playing with a vector nav and ardupilot. Have you given any thought as to adding it as a AHRS backend?
On the actual change set itself though I have a couple of questions, why are you using the group 6 messages? They are all the INS processed versions on the vectornav, which would mean feeding the EKF with data that has already been through a EKF once. If this is just a GPS driver, then this should only be using the GPS message, and the group 6 message shouldn't be used at all.
Have you flown this yet?
I made a couple of other style/implementation comments (and haven't looked at any of the time helper stuff yet).
| @@ -341,6 +343,8 @@ AP_GPS::detect_instance(uint8_t instance) | |||
| if (dstate->current_baud == ARRAY_SIZE(_baudrates)) { | |||
| dstate->current_baud = 0; | |||
| } | |||
|
|
|||
| @@ -298,6 +299,7 @@ AP_GPS::detect_instance(uint8_t instance) | |||
| } | |||
|
|
|||
| if (_port[instance] == nullptr) { | |||
|
|
|||
| @@ -468,6 +476,7 @@ AP_GPS::update_instance(uint8_t instance) | |||
| } | |||
|
|
|||
| // we have an active driver for this instance | |||
|
|
|||
| memset(dataPl,0,600); | ||
| if(c == 0xFA){ | ||
| packetsize = 0; | ||
| packetsize++; |
There was a problem hiding this comment.
This manipulation of packetsize is extremely confusing, you only use it as an index into the array, so packetsize shouldn't be incremented here, as there is no data in the array yet, and that way you don't need to do a - 1 every time you try to write to it.
| case 0: | ||
| // Checking for header | ||
| count = 0; | ||
| memset(dataPl,0,600); |
There was a problem hiding this comment.
Use the array length macro, rather then a magic number.
| //state.horizontal_accuracy = msg4.PosU[0]*100; | ||
| //state.vertical_accuracy = msg4.PosU[2]*100; | ||
| state.have_horizontal_accuracy = false; | ||
| state.have_vertical_accuracy = false; |
There was a problem hiding this comment.
These are available in message 4.
| state.have_speed_accuracy = true; | ||
| state.speed_accuracy = msg6.VelU; | ||
| state.num_sats = msg4.NumSats; | ||
| state.hdop = 130; |
There was a problem hiding this comment.
If the DOP's aren't available (which they aren't) this should be 9999 rather then a fake/incorrect DOP.
| state.ground_course = wrap_360(degrees(atan2f(state.velocity.y, state.velocity.x))); | ||
| state.ground_speed = norm(state.velocity.y, state.velocity.x); | ||
| state.have_speed_accuracy = true; | ||
| state.speed_accuracy = msg6.VelU; |
There was a problem hiding this comment.
This should be group 4.
| state.have_vertical_accuracy = false; | ||
|
|
||
| state.have_vertical_velocity = true; | ||
| state.velocity.x = msg6.VelNed[0]; |
| uint16_t CRC = 0; | ||
| int packetsize = 0; | ||
|
|
||
| uint8_t dataPl[600]; |
There was a problem hiding this comment.
Rather then always being 600 bytes, this should be sized to the expected payload length, using a #define
|
Yes. I've flown this with a pixhawk + DJI S1000. I was thinking of using it as the AHRS backend. I'll incorporate the suggested changes. Thanks! |
|
This needs to be rebased on master, with commits updating submodules removed as well as the commits already in master. Also, your commit messages aren't following our contribution guidelines (ardupilot.org/dev/docs/submitting-patches-back-to-master.html). And I think all those commits can be squashed into one. We are available to help with anything in Gitter chat (http://gitter.im/ArduPilot/ardupilot). |
|
@SweeWarman @WickedShell |
|
@Kiwa21 someday when I finally get the time! <sigh!> |
|
Closing for now, as this needs to be updated. Feel free to reopen if you get the time to work on this again. |
Added a new driver to read Vector NAV GPS modules. http://www.vectornav.com/products/vn200-rugged