Skip to content

Gsof nfc copilot fixes#32790

Open
Ryanf55 wants to merge 3 commits intoArduPilot:masterfrom
Ryanf55:gsof-nfc-copilot-fixes
Open

Gsof nfc copilot fixes#32790
Ryanf55 wants to merge 3 commits intoArduPilot:masterfrom
Ryanf55:gsof-nfc-copilot-fixes

Conversation

@Ryanf55
Copy link
Copy Markdown
Contributor

@Ryanf55 Ryanf55 commented Apr 15, 2026 •
edited
Loading

Summary

Address the useful things from Copilot's review
#30232 (comment)

I don't think any of these are functional changes.
The bitmasks are known to work (flight tested on hardware), but initialization in this manner is good defensive programming.

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

tested with SITL hardware connected to verify parser doesn't blow up.

Description

  • Fix comments
  • Make comparisons consistent in health
  • Initialize bitmasks
  • Use consistent naming for defines in AP_EXTERNAL_AHRS.

@Ryanf55 Ryanf55 requested a review from rmackay9 April 15, 2026 20:46
rmackay9
rmackay9 approved these changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@rmackay9 rmackay9 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks!

We should test it on real hardware if possible

peterbarker
peterbarker approved these changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

peterbarker
peterbarker previously requested changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I just looked at the commit list.

"I did stuff" is not a valid commit message, and neither are these.

@Ryanf55 Ryanf55 force-pushed the gsof-nfc-copilot-fixes branch from 8f97f28 to 21fedde Compare April 17, 2026 00:44
@Ryanf55
Copy link
Copy Markdown
Contributor Author

Ryanf55 commented Apr 17, 2026 •
edited
Loading

Sorry, but I just looked at the commit list.

"I did stuff" is not a valid commit message, and neither are these.

Fixed. Each comment now has bullets for exactly what was done on that commit.
I rebased on master again. No code changes.

image

@Ryanf55 Ryanf55 requested a review from peterbarker April 17, 2026 00:44
@rmackay9 rmackay9 dismissed peterbarker’s stale review April 17, 2026 00:55

PeterB's requests addressed

@peterbarker
Copy link
Copy Markdown
Contributor

peterbarker commented Apr 17, 2026

Sorry, but I just looked at the commit list.
"I did stuff" is not a valid commit message, and neither are these.

Fixed. Each comment now has bullets for exactly what was done on that commit. I rebased on master again. No code changes.

Sorry, not fixed.

This is the problem, not the body of the commit message:

image

peterbarker
peterbarker requested changes Apr 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit messages need fixing.

Ryanf55 added 3 commits April 16, 2026 23:04
@Ryanf55
AP_ExternalAHRS: consistent naming, init, comparisons
826f382 
* Use consistent underscore for AP_EXTERNAL_AHRS flags
* Fix spelling typos
* Initialize AP_GSOF:MsgTypes
* Use consistent < vs <= in times_healthy

Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com>
@Ryanf55
AP_GPS: Brace initialize AP_GSOF:MsgTypes
729341e 
Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com>
@Ryanf55
AP_GSOF: brace initialization and fix unit comment
2e30d14 
* Initialize AP_GSOF:MsgTypes with braces
* Fix units in ang_rate_z comment

Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com>
@Ryanf55 Ryanf55 force-pushed the gsof-nfc-copilot-fixes branch from 21fedde to 2e30d14 Compare April 17, 2026 05:07
peterbarker
peterbarker approved these changes Apr 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmackay9 rmackay9 rmackay9 approved these changes

@peterbarker peterbarker peterbarker approved these changes

Assignees

No one assigned

Labels

DevCallTopic ExternalAHRS GPS MergeOnCIPass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Ryanf55 @peterbarker @rmackay9
⚡