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.

Sign up for GitHub

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

Jump to bottom

Add a parameter-free unicycle car system and agent #720

Merged
merged 3 commits into from
Dec 21, 2020
Merged

Add a parameter-free unicycle car system and agent #720

merged 3 commits into from
Dec 21, 2020

Conversation

jadecastro
Copy link
Contributor

No description provided.

@jadecastro jadecastro force-pushed the jadecastro/unicycle_car branch 3 times, most recently from bd22e35 to c0c3a6a Compare November 12, 2020 18:59
@stonier stonier mentioned this pull request Nov 16, 2020
Merged
@stonier
Copy link
Collaborator

stonier commented Nov 18, 2020

@jadecastro I rebased and then removed the unused gmock header in 1a3ccb0 since it failed to compile after the rebase (there was a change to use system gtest in #717 yesterday. Was there any reason you were including gmock?

@jadecastro
Copy link
Contributor Author

@stonier I think gmock was a stray include (carried over from SimpleCar's test). It can be removed since we're not actually mocking anything here.

stonier
stonier reviewed Nov 18, 2020
View reviewed changes
Copy link
Collaborator

@stonier stonier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Two major bullets with this PR:

  • It seems to be operating on a flat world assumption, that's not going to be true for this use case. Perhaps though we can take this first pass with that assumption and then work on that in the second pass. Let's discuss in slack.
  • See if you can break the PR into two - one for system/command code and one for delphyne code. That way it will be easy to point Charles to PRs/commits that he needs to care about.

And a few minor bullets inlined in the changes.

FYI @andrewbest-tri

stonier
stonier reviewed Nov 18, 2020
View reviewed changes
src/agents/unicycle_car.cc Outdated
* Unicycle Car System
*********************/
typedef UnicycleCar<double> UnicycleCarSystem;
UnicycleCarSystem* simple_car_system =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy/pasta artifact? simple_car_system -> unicycle_car_system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

stonier
stonier reviewed Nov 18, 2020
View reviewed changes
src/protobuf/automotive_angular_rate_acceleration_command.proto Outdated
/// \brief Optional header data
optional Header header = 1;

optional Time time = 2; // Time when the data was captured
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be compulsory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (Removed this proto file)

stonier
stonier reviewed Nov 18, 2020
View reviewed changes
src/protobuf/automotive_angular_rate_acceleration_command.proto Outdated
import "ignition/msgs/header.proto";
import "ignition/msgs/time.proto";

message AutomotiveAngularRateAccelerationCommand
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the Automotive prefix? Insanely verbose and I don't grokk how prefixing Automotive makes it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. (Removed this proto file)

@jadecastro jadecastro force-pushed the jadecastro/unicycle_car branch 4 times, most recently from 664aad7 to 5023957 Compare December 9, 2020 14:35
@jadecastro
Copy link
Contributor Author

jadecastro commented Dec 9, 2020 •
edited
Loading

In addition to the inline changes, I've reworked the agent version of UnicycleCar, UnicycleCarAgent, so that it doesn't rely any longer on ignition messaging. The benefit for this PR is that all the ignition-related translators for the AngularRateAccelerationCommand aren't needed anymore, which sharply reduces the infrastructure added in this PR and the one in delphyne_gui (maliput/delphyne_gui#322).

@jadecastro jadecastro force-pushed the jadecastro/unicycle_car branch 2 times, most recently from d20a8b4 to 8e385c4 Compare December 17, 2020 18:38
stonier
stonier approved these changes Dec 21, 2020
View reviewed changes
Copy link
Collaborator

@stonier stonier left a comment

Choose a reason for hiding this comment

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

Pending CI problem-fix. LGTM.

@jadecastro jadecastro merged commit 1f3ceff into master Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stonier stonier stonier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jadecastro @stonier