Skip to content
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

sbgecom: Implement sbgECom INS driver #24137

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tolesam
Copy link

@tolesam tolesam commented Dec 19, 2024

New Feature

Implement sbgECom messages handling to provide IMU sensors, GNSS and EKF data to the autopilot
Be able to parametrize the serial port, baudrate and the communicating mode
Clone sbgECom library only if sbgecom support is enabled and apply a patch
Be able to send SBG Systems INS settings in several ways when starting sbgecom driver

Bug Fixes

Fix sensor airspeed simulator units
Fix HIGHRES_IMU pressure unit

Remarks

Users of external INS (vectornav and sbgecom) need to enable the drivers in the config. As all external INS are currently enabled on some board config, CI is failing.

@tolesam tolesam changed the title Implement sbgECom INS driver sbgecom: Implement sbgECom INS driver Dec 20, 2024
@saengphet
Copy link

Hello @tolesam I have a question. Does this PR intend to be used with the Ellipse Series, Ellipse 2 Micro Series, or both?

I'm interested in SBG products.

@tolesam
Copy link
Author

tolesam commented Dec 21, 2024

Hello @tolesam I have a question. Does this PR intend to be used with the Ellipse Series, Ellipse 2 Micro Series, or both?

I'm interested in SBG products.

Hello @saengphet,
It's intended to be used with all SBG Systems' INS products.
Feel free to give us any feedback about it, we'll be happy to help you with our products' integration with this driver.
Regards,
Samuel Toledano

@tolesam tolesam force-pushed the dev/sbgecom_driver branch 2 times, most recently from 72a92c0 to ffe24c4 Compare January 9, 2025 09:16
@tolesam
Copy link
Author

tolesam commented Jan 10, 2025

Hi @dagar ,

Can you take a look at this PR please?
I see you've already integrated another INS driver and we would be happy to have sbgecom driver available on PX4 firmware as well.

Moreover, it seems the job "Build and Push Container" doesn't work on PRs, do you have any idea why?
I tried to run it locally on ubuntu-latest and it worked well.

Thanks in advance.

@tolesam tolesam force-pushed the dev/sbgecom_driver branch 3 times, most recently from 862fbd7 to 7627a34 Compare January 14, 2025 17:01
@tolesam tolesam requested a review from bresch January 15, 2025 14:31
@tolesam tolesam force-pushed the dev/sbgecom_driver branch 5 times, most recently from 5a85ddd to a70bb76 Compare January 27, 2025 16:00
@tolesam tolesam force-pushed the dev/sbgecom_driver branch from a70bb76 to 7545084 Compare January 31, 2025 16:04
@tolesam tolesam force-pushed the dev/sbgecom_driver branch 4 times, most recently from 667bca1 to ffeede9 Compare February 7, 2025 15:17
@tolesam tolesam force-pushed the dev/sbgecom_driver branch 4 times, most recently from b227586 to 550b04a Compare February 17, 2025 09:46
@tolesam
Copy link
Author

tolesam commented Feb 17, 2025

Hello @bresch ,
Do you have any feedback about this PR or is there anything I can do to help move it forward?
Thanks in advance for your review.

@@ -318,11 +318,6 @@ bool VehicleIMU::UpdateAccel()
PX4_ERR("%d - accel %" PRIu32 " timestamp error timestamp_sample: %" PRIu64 ", previous timestamp_sample: %" PRIu64,
_instance, accel.device_id, accel.timestamp_sample, _accel_timestamp_sample_last);
}

if (accel.timestamp < accel.timestamp_sample) {
Copy link
Member

Choose a reason for hiding this comment

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

why do you remove those checks?

Copy link
Author

Choose a reason for hiding this comment

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

When using an external INS that is not powered by the autopilot, if it is started beforehand, it causes errors.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but that shouldn't be fixed by ignoring the problem. From the point of view of the autopilot, it's not possible that the sample was received before it was created. If this is the case, it's because both systems are not time-synced.

If proper time-sync is not possible, I think we should rather override the timestamp_sample in the driver, as having an incorrect timestamp_sample is worse than assuming 0 delay between both timestamps.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I understand. We can't have a proper time-sync for now so let's assume we have 0 delay between timestamps.
I 've pushed the change in the driver and revert the one in Vehicle_IMU.

@@ -21,7 +21,6 @@ CONFIG_DRIVERS_IMU_ANALOG_DEVICES_ADIS16448=y
CONFIG_DRIVERS_IMU_INVENSENSE_ICM20602=y
CONFIG_DRIVERS_IMU_INVENSENSE_ICM20649=y
CONFIG_DRIVERS_IMU_INVENSENSE_ICM20948=y
CONFIG_COMMON_INS=y
Copy link
Member

Choose a reason for hiding this comment

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

This would break existing setups using the vector nav driver. I understand you cannot leave it enabled as both drivers won't fit flash but maybe add a note in the PR description for the release notes that users of external INS (vector nav and sbgecom) need to enable the drivers in the config.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I revert it and change the PR description.

Copy link
Author

Choose a reason for hiding this comment

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

@bresch By reverting this change the issue is I have 3 jobs that are failing due to flash overflowing.

Copy link
Member

Choose a reason for hiding this comment

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

@dagar do we then just disable all INS drivers?

@tolesam tolesam force-pushed the dev/sbgecom_driver branch 4 times, most recently from 8802af5 to 7d87025 Compare February 18, 2025 17:19
@mrpollo
Copy link
Contributor

mrpollo commented Feb 21, 2025

@tolesam can you please rebase your pr with latest from main? I want to see if the failed CI tests are real

@tolesam tolesam force-pushed the dev/sbgecom_driver branch 2 times, most recently from fe7debc to 3697ec7 Compare February 22, 2025 11:22
@tolesam
Copy link
Author

tolesam commented Feb 22, 2025

@mrpollo
Build jobs were failing due to temperature removal from airspeed message.
I've changed it.
Now remaining failing jobs are due to flash overflow.

@tolesam tolesam force-pushed the dev/sbgecom_driver branch 2 times, most recently from b709a8e to 8f5ed0e Compare February 25, 2025 09:39
Samuel Toledano added 4 commits February 26, 2025 10:27
Implement sbgECom messages handling to provide IMU sensors, GNSS and EKF data to the autopilot
Be able to parametrize the serial port, baudrate and the communicating mode
Clone sbgECom library only if sbgecom support is enabled and apply a patch
Be able to send SBG Systems INS settings in several ways when starting sbgecom driver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants