Skip to content

Conversation

@dwalton76
Copy link
Collaborator

@dwalton76 dwalton76 self-assigned this Jan 4, 2020
Copy link
Member

@WasabiFan WasabiFan left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review this!

I like consolidating and making the various Move* classes consistent. My main worry is just making sure that we don't introduce confusion about when and how the gyro is used.

current_angle = self.angle
delta = abs(current_angle - self._init_angle) % 360

if delta == 0:
Copy link
Member

Choose a reason for hiding this comment

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

This seems over-complicated. Shouldn't the transformation from gyro angle to appropriate angle always be some_constant_offset - current_angle?

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 thought so too, figured it would be relatively simple. I spent a few hours coming up with this function though...maybe I am missing something.

@dwalton76
Copy link
Collaborator Author

Still some more work to go on this, I should be able to get to the other comments this weekend.

@dwalton76 dwalton76 merged commit 69e7bde into ev3dev:ev3dev-stretch Jan 15, 2020
@dwalton76 dwalton76 deleted the odometry-gryo branch January 15, 2020 00:43
@WasabiFan
Copy link
Member

I had been waiting to review this until I had more time... I still have some concerns which I'll comment on when I can work through it more thoroughly.

@dwalton76
Copy link
Collaborator Author

My bad...I had resolved all of the comments so thought we were set. We can revert the commit or I can tackle the new comments via a new PR.

@WasabiFan
Copy link
Member

In the same vein as we discussed in #707 and shown in #715, this PR makes breaking changes, and it will cause confusion. I think this should be reverted so we can fix the breaking changes.

@dwalton76
Copy link
Collaborator Author

@WasabiFan ack let me revert this and open a new PR. My bad for jumping the gun on the commit.

dwalton76 pushed a commit that referenced this pull request Jan 17, 2020
@dwalton76 dwalton76 changed the title MoveDifferential: use gyro support for better accuracy MoveDifferential: use gyro support for better accuracy (reverted this commit) Jan 17, 2020
@dwalton76
Copy link
Collaborator Author

Reverted this commit and opened #718

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.

MoveDifferential: use gyro support for better accuracy

2 participants