Skip to content

Some of the IK Fixes From CIRC - #338

Open
Geeoon wants to merge 11 commits into
masterfrom
ik-fixes
Open

Some of the IK Fixes From CIRC#338
Geeoon wants to merge 11 commits into
masterfrom
ik-fixes

Conversation

@Geeoon

@Geeoon Geeoon commented Nov 2, 2024

Copy link
Copy Markdown
Member

Most of the changes from the IK fixes from CIRC 723c6fd are addressed in this PR. The exception is with the sleep function in the TunePID.cpp file, which will be addressed in another PR.

@Geeoon
Geeoon requested a review from huttongrabiel November 2, 2024 19:23

@huttongrabiel huttongrabiel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! Just a couple super minor things. I think it would be smart to test this on hardware if possible (though it may not be because things have changed).

Comment thread src/TunePID.cpp Outdated
can::motor::setMotorMode(serial, can::motor::motormode_t::pid);
can::motor::setMotorPIDConstants(serial, p_coeff, i_coeff, d_coeff);

// can::motor::setMotorPIDMaxPower(serial, 32767); // this was here from circ

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For now you can just remove this

Comment thread src/TunePID.cpp Outdated
prescaled_target = round(prescaled_target);
}
angle_target = (int32_t) round(amplitude * prescaled_target) + starting_angle;
angle_target = (int32_t)round(amplitude * prescaled_target) + starting_angle;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you make this a static_cast

Comment thread src/utils/core.h
@@ -1,5 +1,6 @@
#pragma once

#include <algorithm>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this include needed?

@Geeoon Geeoon Nov 9, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The std::transform function is from the algorithm header. It should have been there to begin with, and I assume that one of the other headers includes it, but we probably shouldn't rely on other headers to include what we want

{{motorid_t::shoulder, {70, 0, 0}},
{{motorid_t::shoulder, {200, 0, 0}},
{motorid_t::elbow, {250, 0, 0}},
// swerve constants need to be updated when firmware changes PID scaling

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you mark this a FIXME and then also state that the numbers should be in the 100s?

@Geeoon
Geeoon requested a review from huttongrabiel November 9, 2024 18:54
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.

2 participants