hugo/feature/Update MotionKit to Fusion #1292
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Feb 17, 2023
🔖 Version comparison
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
Codecov Report
@@ Coverage Diff @@
## develop #1292 +/- ##
===========================================
+ Coverage 98.73% 98.75% +0.01%
===========================================
Files 145 147 +2
Lines 3729 3765 +36
===========================================
+ Hits 3682 3718 +36
Misses 47 47
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
3 times, most recently
from
e94e355 to
b789b93
Compare
ladislas
requested changes
Feb 20, 2023
ladislas
requested changes
Feb 20, 2023
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
4 times, most recently
from
643434f to
4d298f3
Compare
ladislas
requested changes
Feb 28, 2023
libs/MotionKit/include/PID.hpp
Outdated
Comment on lines
39
to
43
| float _error_position_total = 0.F; | ||
| float _error_position_last = 0.F; | ||
| float _proportional = 0.F; | ||
| float _integral = 0.F; | ||
| float _derivative = 0.F; |
There was a problem hiding this comment.
could be made a struct as they are all related, at least the last 3
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
from
64e7a48 to
970fdb2
Compare
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
from
970fdb2 to
d6e7b3a
Compare
📈 Changes Impact Analysis Report📌 Info
🤖 Firmware impact analysis
Click to show memory sections🔬 Detailed impact analysisClick to show detailed analysis for all targets
🗺️ Map files diff output
|
📈 Changes Impact Analysis Report📌 Info
🤖 Firmware impact analysis
Click to show memory sections🔬 Detailed impact analysisClick to show detailed analysis for all targets
🗺️ Map files diff output
|
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
2 times, most recently
from
0ebd679 to
9f109ee
Compare
ladislas
approved these changes
Mar 3, 2023
There was a problem hiding this comment.
thanks for the refactor 👍
I think we're good for the time being.
I've made some suggestions/refactors for 1.5.0 to allow us to do what we talked about in our weekly #1308 and provide a lot of different controls
MotionKit won't need to depend on IMUKit and Motors, only the controls will depend on those
Comment on lines
+44
to
+45
| RotationControl _rotation_control; | ||
| StabilizationControl _stabilization_control; |
There was a problem hiding this comment.
for now, we'll say this is okay.
for 1.5.0, it must be refactored to instanciate control classes outside and register/start them when needed.
on the same model as CommandKit
This will allow us to add more controls without the need to recompile MotionKit
libs/MotionKit/include/MotionKit.hpp
Outdated
Comment on lines
36
to
37
| void calculateTotalYawRotation(float yaw); | ||
| void setMotorsSpeedAndDirection(float speed, Rotation direction); |
There was a problem hiding this comment.
do both controls need those?
if not, they could be move to the control class that needs them.
Comment on lines
+33
to
+34
| void processAngleForRotation(const EulerAngles &angles, Rotation direction); | ||
| void processAngleForStabilization(const EulerAngles &angles); |
There was a problem hiding this comment.
in a future PR, this should replaced by start(control_name)
| _imukit(imu_kit), | ||
| _event_loop(event_loop), | ||
| _timeout(timeout) | ||
| MotionKit(interface::Motor &motor_left, interface::Motor &motor_right, interface::IMUKit &imu_kit, |
There was a problem hiding this comment.
in a futur PR/refactor, I think MotionKit will not depend on IMUKit but instead, each control class that needs it will depend on what is needed.
just like CommandKit
same for the motors, etc.
MotionKit will just need to start and stop the control without knowing what the control actually does.
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
from
9f109ee to
0f61fec
Compare
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-to-Fusion
branch
from
0f61fec to
280f03a
Compare
ladislas
requested changes
Mar 3, 2023
Comment on lines
37
to
43
| auto RotationControl::calculateYawRotation(float previous_yaw, float yaw) -> float | ||
| { | ||
| auto abs_yaw_delta = std::abs(previous_yaw - yaw); | ||
| if (abs_yaw_delta >= 300.F) { | ||
| return 360.F - abs_yaw_delta; | ||
| } | ||
| return abs_yaw_delta; |
There was a problem hiding this comment.
Suggested change
| auto RotationControl::calculateYawRotation(float previous_yaw, float yaw) -> float | |
| { | |
| auto abs_yaw_delta = std::abs(previous_yaw - yaw); | |
| if (abs_yaw_delta >= 300.F) { | |
| return 360.F - abs_yaw_delta; | |
| } | |
| return abs_yaw_delta; | |
| auto RotationControl::calculateYawRotation(float previous, float current) -> float | |
| { | |
| auto abs_delta = std::abs(previous - current); | |
| if (abs_delta >= 300.F) { | |
| return 360.F - abs_delta; | |
| } | |
| return abs_delta; |
libs/MotionKit/source/MotionKit.cpp
Outdated
Comment on lines
79
to
82
| _angle_rotation_sum += _rotation_control.calculateYawRotation(_euler_angles_previous.yaw, angles.yaw); | ||
|
|
||
| while (_stabilisation_requested || _target_not_reached) { | ||
| auto [pitch, roll, yaw] = _imukit.getEulerAngles(); | ||
| auto [speed, rotation] = _pid.processPID(pitch, roll, yaw); | ||
| if (_rotate_x_turns_requested && _target_not_reached) { | ||
| auto speed = _rotation_control.processRotationAngle(_angle_rotation_target, _angle_rotation_sum); |
There was a problem hiding this comment.
why do you need this to be here?
the RotationControl should handle this logic, not MotionKit
There was a problem hiding this comment.
the whole function should be in RotationControl
you push the raw yaw value as input and get speed + direction as output
(before we move all that to the control class completely)
libs/MotionKit/source/MotionKit.cpp
Outdated
Comment on lines
29
to
48
| _euler_angles_previous = _imukit.getEulerAngles(); | ||
|
|
||
| _target_not_reached = true; | ||
| _stabilisation_requested = false; | ||
| _rotate_x_turns_requested = true; | ||
|
|
||
| _rotations_to_execute = number_of_rotations; | ||
|
|
||
| _motor_left.spin(direction, kPwmMaxValue); | ||
| _motor_right.spin(direction, kPwmMaxValue); | ||
| _angle_rotation_sum = 0; | ||
| _angle_rotation_target = number_of_rotations * 360.F; | ||
|
|
||
| auto on_timeout = [this] { stop(); }; | ||
|
|
||
| _timeout.onTimeout(on_timeout); | ||
| _timeout.start(10s); | ||
|
|
||
| _event_loop.start(); | ||
| auto on_euler_angles_rdy_callback = [this, direction](const EulerAngles &euler_angles) { | ||
| processAngleForRotation(euler_angles, direction); | ||
| }; | ||
|
|
||
| _imukit.onEulerAnglesReady(on_euler_angles_rdy_callback); | ||
|
|
||
| _on_rotation_ended_callback = on_rotation_ended_callback; |
There was a problem hiding this comment.
All of this should be part of RotationControl
|
SonarCloud Quality Gate failed. |
|
PR split into:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MotionKit refactor following the previous IMUKit refactor with the Fusion algorithm and an asynchronous behavior.