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 libraries for observers #27

Merged
merged 94 commits into from
Feb 7, 2024
Merged

Conversation

ArnaudDmt
Copy link
Contributor

No description provided.

Copy link
Member

@gergondet gergondet left a comment

Choose a reason for hiding this comment

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

These are mostly high level comments, I'll have a closer look after this first round.

Within reason try to avoid long functions and try to avoid duplicating large swat of codes between different functions, this makes it harder to follow the logic and to understand the difference in implemented behavior for some of them without paying close attention to the details which makes it harder to reason about and to debug

src/observersTools/measurementsTools.cpp Outdated Show resolved Hide resolved
src/observersTools/leggedOdometryTools.cpp Outdated Show resolved Hide resolved
src/observersTools/leggedOdometryTools.cpp Outdated Show resolved Hide resolved
src/observersTools/leggedOdometryTools.cpp Outdated Show resolved Hide resolved
src/observersTools/leggedOdometryTools.cpp Outdated Show resolved Hide resolved
src/observersTools/leggedOdometryTools.cpp Outdated Show resolved Hide resolved
Copy link
Member

@gergondet gergondet left a comment

Choose a reason for hiding this comment

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

As discussed in person, the main remaining changes is around the building of ContactSet(s) while updating contacts, which can be circumvented by calling the callbacks as the contacts are detected.

CMakeLists.txt Outdated Show resolved Hide resolved
include/mc_state_observation/measurements/Contact.h Outdated Show resolved Hide resolved
@gergondet gergondet marked this pull request as ready for review January 19, 2024 05:40
Copy link
Member

@gergondet gergondet left a comment

Choose a reason for hiding this comment

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

Thanks again for the work you put into this. I think we will be able to merge this soon.

I have pushed something closer to what I add in mind with the introduction of the contacts' callbacks, feel free to cherry-pick them on the relevant branches or adapt them otherwise

Please also address the (relatively few) remaining points and we'll be good to go :)

CMakeLists.txt Show resolved Hide resolved
include/mc_state_observation/measurements/Contact.h Outdated Show resolved Hide resolved
include/mc_state_observation/measurements/Contact.h Outdated Show resolved Hide resolved
return out.str();
}

} // namespace mc_state_observation::measurements
Copy link
Member

Choose a reason for hiding this comment

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

I have pushed something closer to what I had in mind about passing callbacks to the update function: gergondet@ad5f447

(matching observer code change: gergondet@1dbe830 without considering whether going through the contact list is absolutely needed or it can be done in the callback)

@@ -0,0 +1,705 @@
#include <mc_state_observation/conversions/kinematics.h>

#include <mc_state_observation/odometry/LeggedOdometryManager.h>
Copy link
Member

Choose a reason for hiding this comment

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

Not done yet

@ArnaudDmt
Copy link
Contributor Author

I have adressed your latest requests

@gergondet gergondet merged commit 198b811 into jrl-umi3218:main Feb 7, 2024
11 checks passed
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.

3 participants