ardupilot
ardupilot copied to clipboard
ArduPilot
Add support for sensors outside VN-100 and VN-300
Expands usability beyond VN-100 and VN-300, and clarifies language between the IMU-only mode and full INS mode.
@IamPete1 The only point of uncertainty on my end for this PR is that all VN-2X0 (which only have 1 GNSS) surrounds the
if (last_ins_pkt2->GPS2Fix < 3)
check.
The change I made just keeps the current behavior of "every expected GNSS receiver should report a fix", but this should probably be an INS Status check. I'm not sure what normal behavior for this is in AP; is it more normal to prevent arming because everything isn't perfect, or more normal to allow arming when AP is good to fly (i.e. has valid attitude estimates, but perhaps not a GNSS compass lock) and trust the user to check statuses for any problems?
trust the user to check statuses
On the whole its better to not trust the user ;). The key thing is that the pre-arm check makes a text report that they have half a chance of understanding. They can then force arm and bypass the check if they are comfortable with it.
@IamPete1 Fair enough! In that case, the only other remaining question before I am content for this to be merged:
I'd expect that GPS_TYPE2 = 21 (ExternalAHRS) shouldn't be set when using a single GNSS system. Do you know the impact of this parameter, or why it's documented as required? If we decide this parameter should not be set when using single-GNSS VN, I'm content to somehow use this flag (or perhaps the gps_count parameter) instead of checking the model string to set a has_dual_gnss flag. What do you think is the most correct solution?
Once I can resolve this "gyros not healthy" preflight check, I can test the change and make sure it works as expected on various VN units.
I'd expect that GPS_TYPE2 = 21 (ExternalAHRS) shouldn't be set when using a single GNSS system.
That could be a valid setup if you have a normal GPS first. The GPS checks should catch the case that the GPS is set to a type that does not exist.
I was able to bench test using a sensor other than VN-100 or VN-300, validating expected outputs and that the system can arm.
@IamPete1 What do you think is necessary before merging this branch? More generally, should you be the one to review these PR's (I have a few more coming) or is someone on the team closer to this area?
What do you think is necessary before merging this branch? More generally, should you be the one to review these PR's (I have a few more coming) or is someone on the team closer to this area?
Commits need squashing and re-baseing. We don't use merge commits. They should also be prefixed with the library eg: AP_ExternalAHRS: VectprNav: ..... It looks to me that this is mostly of the diff is none functional renaming? It would make it easier to review if the renameing stuff was in a separate commit to the functional changes. Other than that the changes look fine.
@IamPete1 Great, I've updated the git history and rebased from AP/master. Is anything else necessary? I apologize, the naming changes weren't easily separable after the face.
@lashVN I've split the commits on the two subsystems, and force pushed. marked for merge
@lashVN also please note that you should not use 'master' branch in your repo for PRs
@tridge
Noted. Thanks for doing that; I'm still familiarizing myself with open-source committing etiquette.
@IamPete1 @tridge
It looks like there is a unit test failure that, as far as I can tell, is totally unrelated to my changes. How should I proceed to get this merged?
@IamPete1 @tridge
It looks like there is a unit test failure that, as far as I can tell, is totally unrelated to my changes. How should I proceed to get this merged?
I have restarted it.
@IamPete1 @tridge
Looks like the CI passed. Should I expect this to automerge in or is some other intervention necessary?
Sorry if this is already queued for merge and it just needs to be manually done, no problem. I'm not procedurally familiar, so I'm not sure if it's my job to be bugging y'all each status change until my branch is merged in
Looks like the CI passed. Should I expect this to automerge in or is some other intervention necessary?
It will have to be merged manually.
I see you pushed again, what were the changes?
My push was only to update master. It looked like the commit I had updated to before may have been causing the CI failures.
On Wed, Jul 3, 2024, 17:17 Peter Hall @.***> wrote:
Looks like the CI passed. Should I expect this to automerge in or is some other intervention necessary?
It will have to be merged manually.
I see you pushed again, what were the changes?
— Reply to this email directly, view it on GitHub https://github.com/ArduPilot/ardupilot/pull/27285#issuecomment-2207398954, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWIIT6VFX5CT5HCZ7U2D523ZKRZ53AVCNFSM6AAAAABJFHBYISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBXGM4TQOJVGQ . You are receiving this because you were mentioned.Message ID: @.***>
