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

Dynamic FIFO Buffering #27841

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Aug 15, 2024

Split out from #27029

This normalizes the usage of the "primary" gyro in the IMU drivers and gives an opportunity to take action when it changes. We were doing this before with random calls into the AHRS, but really the IMU needs to know this information. This makes it possible to take specific required action in future PRs.

I've turned this into a demo of what this can be used for, most notably to slow down FIFO read rates on non-primary gyros.
On a CubeOrange+ with INS_GYRO_RATE=2 (4KHz):
Before:

idle          PRI=  1 sp=0x30021CF8 STACK= 296/ 504 LOAD=22.0%
SPI4          PRI=181 sp=0x3002AF58 STACK= 896/1464 LOAD=23.2%
SPI1          PRI=181 sp=0x3002B668 STACK= 824/1464 LOAD=25.2%

After:

idle          PRI=  1 sp=0x30021D00 STACK= 296/ 504 LOAD=34.7%
SPI4          PRI=181 sp=0x3002AF58 STACK= 816/1464 LOAD=22.0%
SPI1          PRI=181 sp=0x3002B668 STACK= 816/1464 LOAD=14.1%

The saving is not as great as you might expect, probably down to SPI overhead which can't be avoided but still substantial.

On my fast rate copter with 2x ICM42688 running at 16Mhz or so I get:
Before:

idle          PRI=  1 sp=0x3001F668 STACK= 296/ 504 LOAD= 4.7%
SPI1          PRI=181 sp=0x20003538 STACK= 848/1464 LOAD=17.7%
SPI4          PRI=181 sp=0x20003B50 STACK= 848/1464 LOAD=16.3%

After:

idle          PRI=  1 sp=0x3001F670 STACK= 296/ 504 LOAD=12.8%
SPI1          PRI=181 sp=0x30021838 STACK= 816/1464 LOAD=19.1%
SPI4          PRI=181 sp=0x20002720 STACK= 824/1464 LOAD= 8.3%*

Which is the difference between on the edge and comfortable

With @bugobliterator 's SPI PR #28093 I get further benefit:

idle          PRI=  1 sp=0x3001F670 STACK= 296/ 504 LOAD=15.4%
SPI1          PRI=181 sp=0x30022188 STACK= 816/1464 LOAD=15.9%
SPI4          PRI=181 sp=0x30021B70 STACK= 824/1464 LOAD= 7.6%*

@andyp1per
Copy link
Collaborator Author

Closing because I can live without changing the SPI delay and needs restructuring to fix the layering

@peterbarker
Copy link
Contributor

"Not a hard no"... need to be careful

precursor for poking the primary sensor into the library, rather than fetching from AP_AHRS would be good

@andyp1per andyp1per changed the title Normalize primary gyro usage in IMU Dynamic FIFO Buffering Sep 12, 2024
@andyp1per andyp1per force-pushed the pr-primary-gyrp branch 2 times, most recently from eda3cb6 to b530458 Compare September 16, 2024 10:57
@andyp1per andyp1per force-pushed the pr-primary-gyrp branch 2 times, most recently from 7bacefb to a56660e Compare September 19, 2024 14:28
@andyp1per andyp1per removed the WIP label Sep 19, 2024
@andyp1per
Copy link
Collaborator Author

Flown on my 5". This really works very well indeed! Giving the fast rate loop extra CPU cycles results in even smoother flying - it was amazing!

@andyp1per
Copy link
Collaborator Author

@tridge I have changed this so that the IMU changes are only active on copter with fast rate enabled. Hopefully this goes some way to alleviate your concerns!

@andyp1per andyp1per force-pushed the pr-primary-gyrp branch 2 times, most recently from ed63bc6 to 2a06cb8 Compare December 12, 2024 15:58
@andyp1per andyp1per requested a review from tridge December 19, 2024 10:18
@andyp1per
Copy link
Collaborator Author

Test flown, EKF innovations look the same. CPU on second IMU way down.

@andyp1per
Copy link
Collaborator Author

Full fight test on two vehicles. EKF happy, CPU load great. Good to go.

Copy link
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.

Looks pretty good to me.

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

i think with the accel changes removed we should merge this, I don't love it, but we need to get to the finish line :-)

AP::ins().set_primary_gyro(primary_gyro);
}

if (primary_accel != state.primary_accel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we don't need to do anything with accel in this PR, we tie the accel and gyro index in AP_AHRS::_get_primary_IMU_index() now anyway

@tridge tridge removed the DevCallEU label Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants