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 fuse_models::GraphIgnition sensor model #196

Merged

Conversation

efernandez
Copy link
Collaborator

  • Add fuse_models::GraphIgnition sensor model

This complements #195, so we can configure a fixed_lag_smoother that ignites when it receives a recorded graph and then apply recorded transactions into it.

This should help motivate #183 a bit more, and also help to test things out, if we agree on using these two sensor models to play back recorded graphs and transactions. This is certainly open to discussion, mostly because #183 is required, and it also looks that even with #183 there are still some sharp edges, so things don't always work because we don't process the correct transaction after the graph. Remember the intention is to throttle the graphs while recording, because recording all graphs takes a lot of disk and bandwidth.

@efernandez efernandez requested review from svwilliams and ayrton04 and removed request for svwilliams May 22, 2020 18:34
@efernandez efernandez self-assigned this May 22, 2020
@efernandez efernandez added the enhancement New feature or request label May 22, 2020
@efernandez efernandez force-pushed the graph-ignition-sensor-model branch 4 times, most recently from 291ec96 to 5eb8bdb Compare May 25, 2020 08:42
@efernandez efernandez force-pushed the graph-ignition-sensor-model branch from 5eb8bdb to 0302cac Compare May 25, 2020 10:52
@efernandez
Copy link
Collaborator Author

@ayrton04 @svwilliams Do you mind to also copy-paste here the check errors for this one, please?

#195 Is passing now. Maybe this needs to be on top of it, but I don't think so. It could be another test failing, since I only made sure it compiles. Note that I moved both the Transaction and GraphIgnition sensor models from an internal repository, and I've already been using them w/o problems. I probably missed something else while moving things here.

@efernandez efernandez force-pushed the graph-ignition-sensor-model branch from 0302cac to bdab0cf Compare May 25, 2020 16:43
* @brief Compare all the properties of two Variable objects
* @return True if all the properties match, false otherwise
*/
bool compareVariables(const fuse_core::Variable& expected, const fuse_core::Variable& actual)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'll make gtest macros for these one day. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if you'd create a header in fuse_core/include/fuse_core for that, or create a new fuse_test package for that. 🤔

I don't think we can access headers define in fuse_core/test, if we move them there. If we actually can, then that would be another option. Probably the best approach.

@efernandez
Copy link
Collaborator Author

The tests passed now. 🎉

So it looks like this is ready to merge. 😃

@svwilliams svwilliams merged commit da477ad into locusrobotics:devel Jun 17, 2020
@efernandez efernandez deleted the graph-ignition-sensor-model branch June 18, 2020 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

3 participants