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

Navigator benchmark framework #47

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

Conversation

mifrandir
Copy link
Member

No description provided.

Copy link
Member

@maxguy2001 maxguy2001 left a comment

Choose a reason for hiding this comment

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

Looks like nav team is gonna have some good fun this year!

lib/navigation/benchmark/benchmark.cpp Outdated Show resolved Hide resolved
lib/core/time.hpp Outdated Show resolved Hide resolved
lib/core/types.hpp Outdated Show resolved Hide resolved
lib/navigation/benchmark/benchmark.hpp Show resolved Hide resolved
lib/navigation/preprocess_imu.hpp Outdated Show resolved Hide resolved
lib/navigation/types.hpp Outdated Show resolved Hide resolved
Copy link
Member

@maxguy2001 maxguy2001 left a comment

Choose a reason for hiding this comment

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

I really like this! The only functionality I think it's maybe missing is something to report what sensors "failed" (became classified as unreliable) and when as this feels like another important metric to use when comparing/benchmarking navigation algorithms?

lib/core/time.cpp Show resolved Hide resolved
lib/navigation/benchmark/benchmark.cpp Show resolved Hide resolved
lib/navigation/benchmark/data.cpp Outdated Show resolved Hide resolved
@mifrandir
Copy link
Member Author

Some tests are a bit iffy still... should be fixable, though.

@mifrandir
Copy link
Member Author

I've also fixed a plethora of bugs and just bad code along the way so we kind of want to get this merged.

Though we should pay special attention to the types because changing them around isn't fun.

In particular, I have opted to give the sensor values without timestamps no names (if possible) so that users will be forced to use the timestamped types unless they want to spell out the whole thing.

Comment on lines +61 to +63
virtual core::Result configure();
virtual std::optional<core::Measurement<core::RawAcceleration>> read();
virtual std::uint8_t getChannel() const;
Copy link
Contributor

@SnickeyX SnickeyX Feb 8, 2023

Choose a reason for hiding this comment

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

i think there are plenty methods currently in derived classes that omit virtual. dont think it changes anything if we omit it or not? but reckon we should decide on the standard, im okay with adding this to derived method from now on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say that we should definitely include virtual. It just makes it clearer that the signature comes from the interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, will start enforcing this

@mifrandir mifrandir requested a review from SnickeyX February 11, 2023 11:00
@SnickeyX SnickeyX mentioned this pull request Feb 11, 2023
11 tasks
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