hugo/feature/Update MotionKit Rotation to Fusion #1314
Merged
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
Contributor
HPezz
commented
Mar 6, 2023
HPezz
commented
- 🏗️ ((interface)): Add IMUKit interface
- ⬆️ (IMUKit): IMUKit derive from interface
- ✨ (IMUKit): Add onEulerAnglesReady function
- ✨ (RotationControl): Replace PID by ControlRotation
- ♻️ (MotionKit): Refactor MotionKit
HPezz
requested review from
ladislas and
YannLocatelli
and removed request for
ladislas
This was
linked to
issues
Mar 6, 2023
🔖 Version comparison
|
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-Rotation-to-Fusion
branch
from
3088d4a to
9bd7ea9
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
|
Codecov Report
@@ Coverage Diff @@
## develop #1314 +/- ##
===========================================
- Coverage 98.73% 98.73% -0.01%
===========================================
Files 145 145
Lines 3729 3726 -3
===========================================
- Hits 3682 3679 -3
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-Rotation-to-Fusion
branch
from
9bd7ea9 to
f60a856
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
|
2 tasks
| @@ -85,13 +82,17 @@ TEST_F(ReinforcerkitTest, playBlinkGreen) | |||
| expectedCallsMovingReinforcer(&led::animation::blink_green); | |||
|
|
|||
| reinforcerkit.play(ReinforcerKit::Reinforcer::BlinkGreen); | |||
|
|
|||
| mock_imukit.call_angles_ready_callback(angles); | |||
There was a problem hiding this comment.
Well reading these tests before implementation, I am surprise to see that it's IMUKit that drive the control (and not MotionKit itself). Should it be? I don't know, just notify that tests might bring some confusion by themselves.
Comment on lines
+18
to
+19
| MotionKit(interface::Motor &motor_left, interface::Motor &motor_right, interface::IMUKit &imu_kit, | ||
| interface::Timeout &timeout) |
There was a problem hiding this comment.
https://github.com/leka/LekaOS/pull/1292/files#r1123885091
What is the futur definition of MotionKit? Is it just a kind of ControlKit handling a bunch of "Control", with 2 methods start and stop with eventually an enum to select them?
Another question is for timeout, does each "Control" should include them or is it handled by MotionKit/ControlKit?
There was a problem hiding this comment.
Some reflexion
IMUKit supports the process of control. Since it is suggested to move out IMUKit from MotionKit, you kinda lose the control of processes.
What ensure you that the callback are freed before using another control? For example,
- 2 controls A and B use imukit (
MotionKitdoes not know that). - A is start by
MotionKit, A register a callback inimukit.onEulerAnglesReady. - Then B is start by
MotionKit, A is requested to stop then B register its callback inimukit.onEulerAnglesReady. Different possibilities can happen- A stop does not reset callback in
imukit.onEulerAnglesReady - A stop reset callback
imukit.onEulerAnglesReadybefore B register its own - A stop reset callback
imukit.onEulerAnglesReadyafter B register its own (deferred process using EventQueue in A)
- A stop does not reset callback in
I am not sure of the behavior of callback if it is removed during a process (case i) but a scenario like case iii can happen and that can be source of confusion.
There was a problem hiding this comment.
in the future, MotionKit will not depend on IMUKit -- the controls will depend on what they need, including IMUKit, Motors if needed
Controls will be like "dynamic" motion behaviors.
when a control starts, it will register the needed callback to IMUKit, replacing the old one.
when stopping from MotionKit, the callback is to be removed.
There was a problem hiding this comment.
in the future, MotionKit will not depend on IMUKit -- the controls will depend on what they need, including IMUKit, Motors if needed
I know that MotionKit should not depend on IMUKit and I agree with that. Now consider MotionKit is responsible of Motion of the robot, what ensure that when MotionKit is asked to be stopped the robot effectively stopped? What if a Control still calling in IMUKit callback a new speed in motors (we forget to remove the callback at stop in the Control file for instance)?
Since it is not a problematic in this PR, we can resolve this conversation here, just keep it in mind when the next refactor of MotionKit will come.
There was a problem hiding this comment.
Now consider MotionKit is responsible of Motion of the robot, what ensure that when MotionKit is asked to be stopped the robot effectively stopped? What if a Control still calling in IMUKit callback a new speed in motors (we forget to remove the callback at stop in the Control file for instance)?
Welcome to callback hell! :)
This is the kind of issue that can arise when callbacks are not used well.
It's true for a callback but it's also true for other dependencies.
Lets take an example with ExampleControl: (it's an example, some things might not exist yet/at all)
- it depends on IMUKit for sensor data
- it depends on two motors for motion
- it also depends on LedKit so also change the color while moving
- it depends on BatteryKit to change the intensity and motor speed depending of the battery level
There's no reason for MotionKit to depend on all those things to make sure that:
- IMUKit callback as been set to an empty std::function
- the two motors are really stopped
- leds are turned off
- BatteryKit callback has also been removed
it's a bit like not using Smart Pointers: if you use raw C pointers and , you need to make sure you clean up after yourself.
Following the TDD methodology, this is what we should be doing:
- specify how
ExampleControlwill work - specify it's dependencies
- review specifications as a team to spot this kind of issues and add a specific task or checklist to make sure we don't forget
- write the tests needed to enforce that on
stop()the dependencies are really cleared/turned off - have the tests fail
- implement
stop()to pass the tests
There's no simple way to do that "automatically" without complex engineering.
it's something that might be possible to do with concepts and templates. Surely an amazing engineering experience but I'm not sure it's worth the time and effort right now.
Again, we master all our code. We don't depend on something opaque and don't know what it does, nor do we provide these libraries as public API for outside users.
| } | ||
|
|
||
| void MotionKit::executeSpeed(float speed, Rotation direction) | ||
| void MotionKit::setMotorsSpeedAndDirection(float speed, Rotation direction) |
There was a problem hiding this comment.
I was a bit confused with motors out of the Control but it is make sense with some definition.
There are 2 different notions: Control (system) and Controller.
- Controller is, in broad term, the computation part of a Control (system). It is a PID for instance.
- Control (system) is composed of a Controller with Actuators and Sensors.
Below a diagram to explain difference between both
There was a problem hiding this comment.
Now if RotationControl is a control system then yes, motors should be included in it.
If RotationControl is a controller, an algorithm (e.g. PID) then motors and IMU can be set out of it and MotionKit is the control system.
There was a problem hiding this comment.
as explained above and discussed previously with @HPezz, motors + IMUKit will be moved to Control in the future.
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-Rotation-to-Fusion
branch
2 times, most recently
from
ead1e0d to
9f9fa0c
Compare
YannLocatelli
requested changes
Mar 9, 2023
Comment on lines
+18
to
+19
| MotionKit(interface::Motor &motor_left, interface::Motor &motor_right, interface::IMUKit &imu_kit, | ||
| interface::Timeout &timeout) |
There was a problem hiding this comment.
in the future, MotionKit will not depend on IMUKit -- the controls will depend on what they need, including IMUKit, Motors if needed
I know that MotionKit should not depend on IMUKit and I agree with that. Now consider MotionKit is responsible of Motion of the robot, what ensure that when MotionKit is asked to be stopped the robot effectively stopped? What if a Control still calling in IMUKit callback a new speed in motors (we forget to remove the callback at stop in the Control file for instance)?
Since it is not a problematic in this PR, we can resolve this conversation here, just keep it in mind when the next refactor of MotionKit will come.
HPezz
force-pushed
the
hugo/feature/Update-MotionKit-Rotation-to-Fusion
branch
from
b3f6b91 to
4cf6ce9
Compare
|
Kudos, SonarCloud Quality Gate passed! |
ladislas
approved these changes
Mar 9, 2023
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.