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

add common math types and cleanup printf includes #44

Merged
merged 2 commits into from
Jan 4, 2025
Merged

Conversation

celery6
Copy link
Collaborator

@celery6 celery6 commented Dec 20, 2024

#41
idk if this makes things too convoluted, otherwise we could make stateest module define these types and everyone else (ie just controller rn i think) import from there. but i feel like that's kinda eh cuz state est didnt invent the idea of vectors...?

Copy link

@Joe-Joe-Joe-Joe Joe-Joe-Joe-Joe left a comment

Choose a reason for hiding this comment

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

Doesn't C already have a math.h? Is that handled by including <> vs ""? Cause if so that feels like a lot of unnecessary confusion

src/common/math/math.h Outdated Show resolved Hide resolved
@celery6
Copy link
Collaborator Author

celery6 commented Dec 20, 2024

Doesn't C already have a math.h? Is that handled by including <> vs ""? Cause if so that feels like a lot of unnecessary confusion

handled by making our include path to src/common, so you have to import using include "math/math.h". Or could remove that include path and just include src/ which would make it #include "common/math/math. h"?

@Joe-Joe-Joe-Joe
Copy link

Joe-Joe-Joe-Joe commented Dec 21, 2024

The lazy part of me thinks we should give all our headers unique names so that we can just add all include paths and include directly. Regardless, I think "math/math.h" is still too confusing, if not in code (where there is a distinction if you put them beside each other), then in documentation.

@celery6
Copy link
Collaborator Author

celery6 commented Dec 22, 2024

making the header name is one solution. I think using directory paths as diy namespacing is more elegant and maybe more common.

Latest change: everything is inlucded relative to src/, so #include "drivers/adc/adc.h" or #include "common/math/math.h"
Yes its fatter but makes clear what is coming from where (drivers vs app vs common etc) in a class-based style which we're trending towards already

@FinnBreu
Copy link

The EKF also uses matrices of multiple sizes, and the state/measurement/input vectors are also larger than the 3d or quaternion. Do they need to be defined as well? Most of this is just internal in the task

@FinnBreu
Copy link

Ohh and as I said once or twice before, you should really really really look at CORDIC for the STM32. The H7 has an accelerator onboard and it would speed up a lot of computations in the model by a lot, all the trig and exponents etc...

And: maybe FMAC also helps, again the H7 has an accelerator. Don't know exactly what it does, if only convolution it doesn't really help, but maybe it can do matrix multiplication/transpose/inverse, which would be awesome

@celery6
Copy link
Collaborator Author

celery6 commented Dec 23, 2024

The EKF also uses matrices of multiple sizes, and the state/measurement/input vectors are also larger than the 3d or quaternion. Do they need to be defined as well? Most of this is just internal in the task

those all seem very stateestimation-specific so idt its common enough to be part of this common lib? I was thinking abt making a general "N size vector" type but thats just more complicated than necessary

@celery6
Copy link
Collaborator Author

celery6 commented Dec 23, 2024

Ohh and as I said once or twice before, you should really really really look at CORDIC for the STM32. The H7 has an accelerator onboard and it would speed up a lot of computations in the model by a lot, all the trig and exponents etc...

And: maybe FMAC also helps, again the H7 has an accelerator. Don't know exactly what it does, if only convolution it doesn't really help, but maybe it can do matrix multiplication/transpose/inverse, which would be awesome

doing this will be part of stateest implementation, thanks for reminidng (again lol)

@celery6 celery6 linked an issue Dec 23, 2024 that may be closed by this pull request
Copy link

@Joe-Joe-Joe-Joe Joe-Joe-Joe-Joe left a comment

Choose a reason for hiding this comment

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

LGTM

@celery6 celery6 merged commit a9b7163 into main Jan 4, 2025
2 checks passed
@celery6 celery6 deleted the common-folder branch January 4, 2025 20:28
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.

add common folder?
3 participants