AP_ExternalAHRS: Actualize handler and simulator of the Inertial Labs EAHRS #27850

valbv · 2024-08-15T15:20:08Z

Fix bugs and add the new data handling

valbv · 2024-08-15T15:25:58Z

Hello guys, we also tested this changes in the real flight.
If you need some logs, we will try to provide it.

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

peterbarker · 2024-08-17T23:58:45Z

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

+                CHECK_SIZE(u.gnss_time_ms);
+                nav_ins_data.ms_tow = u.gnss_time_ms;


I mean, yes, this is a correct change.

But it also occludes the history of the file content.

Do we really need this change?

I'll take note to try to stop this creeping in in the future....

Sorry, you mean this shift about switch-case or something else?
I returned it back to avoid this big diff

Sorry, you mean this shift about switch-case or something else? I returned it back to avoid this big diff

Yes, minimising the diff is important. It makes review much faster, and if a PR needs reviewing multiple times it can be very frustrating.

In this case you appear to have changed 'gps' to 'gnss' - and while I understand that's a correct change, it's also unnecessary as the two are often used interchangably.

Oh, now I understood, sorry. I renamed this because other structures related to GNSS were added to the processor and to the simulator.
And in order to keep all the names consistent with the documentation, I decided to use GNSS everywhere. To make it easier to find in documentation in the future.

Given this moment, will it better to rename all gnss (including new variables and structures) to gps?

Please remove the renames from gps to gnss. New stuff in this PR can use gnss, for now we treat them synonymously in ArduPilot.

This and the whitespace change significantly reduced my enthusiasm to review this PR!

If you want to come back and do a separate PR just to rename things in this new driver for consistency, I think I'd merge it. The code in here is very young, and such a change is provably correct by ensuring compiler output is identical before/after.

Don't bother reverting the simulator changes - I'm happy enough merging those.

Done, I removed all renaming changes

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

peterbarker · 2024-08-20T20:55:26Z

This PR is actually looking pretty good. I'm really just arguing about the shape of the patches, not the additions and fixes you're making!

valbv

Commit with whitespace changes was removed

Should I resolve conversations after fixing or conversation starter will make it if it's done?

peterbarker · 2024-09-02T01:31:18Z

Commit with whitespace changes was removed

Thanks.

Should I resolve conversations after fixing or conversation starter will make it if it's done?

We don't have a solid attitude on this. I like to resolve the conversations that I've started, but I've seen plenty of people close them as they're done.

peterbarker · 2024-09-02T01:32:34Z

libraries/AP_ExternalAHRS/AP_ExternalAHRS.h

@@ -129,6 +129,7 @@ class AP_ExternalAHRS {
        uint8_t instance;
        float pressure_pa;
        float temperature;
+        float altitude;


What frame is this in? Above geoid? AMSL?

Comment if nothing else

Done, I added a comment

peterbarker · 2024-09-02T01:35:09Z

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

+                CHECK_SIZE(u.gnss_time_ms);
+                nav_ins_data.ms_tow = u.gnss_time_ms;


Please remove the renames from gps to gnss. New stuff in this PR can use gnss, for now we treat them synonymously in ArduPilot.

This and the whitespace change significantly reduced my enthusiasm to review this PR!

If you want to come back and do a separate PR just to rename things in this new driver for consistency, I think I'd merge it. The code in here is very young, and such a change is provably correct by ensuring compiler output is identical before/after.

Don't bother reverting the simulator changes - I'm happy enough merging those.

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

peterbarker · 2024-09-02T01:39:35Z

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

+        case MessageType::GNSS_ANGLES: {
+            CHECK_SIZE(u.gnss_angles);
+            gnss_data.heading = u.gnss_angles.heading*0.01; // deg
+            gnss_data.pitch = u.gnss_angles.pitch*0.01; // deg


pitch in gnss_data?!

I think there might be a reason it was generically called state2?

I try to use the same terms as in the device documentation
For consistency and simplifying code maintenance

*I can rename, if need it

valbv · 2024-09-02T10:51:24Z

Done, I removed all renaming changes

peterbarker · 2024-09-10T13:49:21Z

Board                    AP_Periph  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeOrange-periph-heavy  *
Durandal                            5688   *           5688    5704              5688   5680   5672
Hitec-Airspeed           *
KakuteH7-bdshot                     5336   *           5344    5344              5344   5328   5328
MatekF405                           *      *           *       *                 *      *      *
Pixhawk1-1M-bdshot                  *                  *       *                 *      *      *
f103-QiotekPeriph        *
f303-Universal           *
iomcu                                                                *
revo-mini                           *      *           *       *                 *      *      *
skyviper-v2450                                         *

That's a lot of extra bytes.

That's not necessarily a show-stopper, but for reference that's about 20% of the remaining flash space on our most popular board's bdshot build.

valbv · 2024-09-10T16:18:50Z

The comments appear to have been corrected.
What should I do to merge this PR?

peterbarker · 2024-09-10T23:01:19Z

The comments appear to have been corrected. What should I do to merge this PR?

I've marked it for discussion at our DevCall meeting this-evening.

tridge

i think we should move this to custom build server only

tridge · 2024-09-11T08:29:44Z

libraries/AP_ExternalAHRS/AP_ExternalAHRS.h

@@ -129,6 +129,7 @@ class AP_ExternalAHRS {
        uint8_t instance;
        float pressure_pa;
        float temperature;
+        float altitude; // m, above geoid


barometers don't measure altitude, they measure pressure

I removed this changes

tridge · 2024-09-11T08:30:05Z

libraries/AP_ExternalAHRS/AP_ExternalAHRS_InertialLabs.cpp

-            baro_data.pressure_pa = u.baro_data.pressure_pa2*2;
+            baro_data.pressure_pa = u.baro_data.pressure_pa2*2; // Pa
+            state2.baro_alt = u.baro_data.baro_alt*0.01; // m
+            baro_data.altitude = state2.baro_alt; // m


should remove this

Done, I removed this changes

peterbarker · 2024-09-11T08:33:31Z

We have an intent to make all ExternalAHRS drivers not built into ArduPilot by default in ArduPilot 4.7.

That would require users to use custom.ardupilot.org to include any ExternalAHRS driver.

peterbarker · 2024-09-11T08:35:12Z

We can merge this once the API change to the EXternalAHRS library has been removed

peterbarker · 2024-09-14T09:29:19Z

Merged, thanks!

valbv · 2024-09-14T15:36:20Z

Thanks for revirew!

valbv force-pushed the ilabseahrs branch from 5184636 to e452b7a Compare August 18, 2024 17:54

peterbarker requested changes Aug 19, 2024

View reviewed changes

valbv force-pushed the ilabseahrs branch from e452b7a to f5569b1 Compare August 20, 2024 18:03

valbv force-pushed the ilabseahrs branch 2 times, most recently from bc69c5c to 47eb259 Compare August 21, 2024 23:13

valbv requested a review from peterbarker August 22, 2024 11:49

valbv commented Aug 24, 2024

View reviewed changes

valbv force-pushed the ilabseahrs branch 2 times, most recently from 47ceea2 to 680b727 Compare August 30, 2024 09:49

peterbarker requested changes Sep 2, 2024

View reviewed changes

valbv force-pushed the ilabseahrs branch from 680b727 to 55e3838 Compare September 2, 2024 10:49

valbv force-pushed the ilabseahrs branch 2 times, most recently from 417d048 to 0f38a42 Compare September 3, 2024 06:29

valbv requested a review from peterbarker September 4, 2024 01:10

peterbarker added the DevCallEU label Sep 10, 2024

tridge requested changes Sep 11, 2024

View reviewed changes

peterbarker removed the DevCallEU label Sep 11, 2024

valbv force-pushed the ilabseahrs branch from 0f38a42 to a30f90f Compare September 11, 2024 17:47

Valentin Bugrov added 3 commits September 11, 2024 21:49

AP_ExternalAHRS: actualize ILabs EAHRS data processing

9931b46

AP_ExternalAHRS: add GCS mesages sending for the ILabs EAHRS

f8f70ac

SITL: Actualize InertialLabs sim

837fe98

valbv force-pushed the ilabseahrs branch from a30f90f to 837fe98 Compare September 11, 2024 17:49

peterbarker added the MergeOnCIPass label Sep 13, 2024

peterbarker approved these changes Sep 13, 2024

View reviewed changes

valbv requested a review from tridge September 13, 2024 11:44

peterbarker merged commit 79bd823 into ArduPilot:master Sep 14, 2024
95 checks passed

valbv deleted the ilabseahrs branch September 16, 2024 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_ExternalAHRS: Actualize handler and simulator of the Inertial Labs EAHRS #27850

AP_ExternalAHRS: Actualize handler and simulator of the Inertial Labs EAHRS #27850

valbv commented Aug 15, 2024

valbv commented Aug 15, 2024

peterbarker Aug 17, 2024

valbv Aug 20, 2024

peterbarker Aug 20, 2024

valbv Aug 21, 2024 •

edited

Loading

peterbarker Sep 2, 2024

valbv Sep 2, 2024

peterbarker commented Aug 20, 2024

valbv left a comment •

edited

Loading

peterbarker commented Sep 2, 2024

peterbarker Sep 2, 2024

valbv Sep 2, 2024

peterbarker Sep 2, 2024

peterbarker Sep 2, 2024

valbv Sep 2, 2024

valbv commented Sep 2, 2024

peterbarker commented Sep 10, 2024 •

edited

Loading

valbv commented Sep 10, 2024

peterbarker commented Sep 10, 2024

tridge left a comment

tridge Sep 11, 2024

valbv Sep 11, 2024

tridge Sep 11, 2024

valbv Sep 11, 2024

peterbarker commented Sep 11, 2024

peterbarker commented Sep 11, 2024

peterbarker commented Sep 14, 2024

valbv commented Sep 14, 2024

		CHECK_SIZE(u.gnss_time_ms);
		nav_ins_data.ms_tow = u.gnss_time_ms;

AP_ExternalAHRS: Actualize handler and simulator of the Inertial Labs EAHRS #27850

AP_ExternalAHRS: Actualize handler and simulator of the Inertial Labs EAHRS #27850

Conversation

valbv commented Aug 15, 2024

valbv commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valbv Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker commented Aug 20, 2024

valbv left a comment • edited Loading

Choose a reason for hiding this comment

peterbarker commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valbv commented Sep 2, 2024

peterbarker commented Sep 10, 2024 • edited Loading

valbv commented Sep 10, 2024

peterbarker commented Sep 10, 2024

tridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker commented Sep 11, 2024

peterbarker commented Sep 11, 2024

peterbarker commented Sep 14, 2024

valbv commented Sep 14, 2024

valbv Aug 21, 2024 •

edited

Loading

valbv left a comment •

edited

Loading

peterbarker commented Sep 10, 2024 •

edited

Loading