Yann/refactor/imu/standardize naming #1407
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
This comment has been minimized.
This comment has been minimized.
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
from
a9b4b9c
to
e976dc2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## yann/rename/imu/rename-corelsm6dsox-into-coreimu #1407 +/- ##
=================================================================================
Coverage 98.75% 98.76%
=================================================================================
Files 146 146
Lines 3783 3793 +10
=================================================================================
+ Hits 3736 3746 +10
Misses 47 47 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 task
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
from
92154be
to
7959b2d
Compare
|
YannLocatelli
force-pushed
the
yann/rename/imu/rename-corelsm6dsox-into-coreimu
branch
2 times, most recently
from
d1e4b0d
to
19017cc
Compare
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
from
7959b2d
to
0f38bca
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
YannLocatelli
force-pushed
the
yann/rename/imu/rename-corelsm6dsox-into-coreimu
branch
2 times, most recently
from
5f8b48a
to
5fd7cd9
Compare
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
from
0f38bca
to
d47f4db
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
YannLocatelli
force-pushed
the
yann/rename/imu/rename-corelsm6dsox-into-coreimu
branch
from
5fd7cd9
to
4358c82
Compare
ladislas
requested changes
Jul 17, 2024
| void CoreIMU::disableOnDataAvailable() | ||
| { | ||
| lsm6dsox_pin_int1_route_t lsm6dsox_int1 { | ||
| .drdy_xl = PROPERTY_DISABLE, |
There was a problem hiding this comment.
ici aussi ça doit être .drdy_g je pense
drivers/CoreIMU/include/CoreIMU.hpp
Outdated
| @@ -52,6 +55,7 @@ class CoreIMU : public interface::IMU, public interface::DeepSleepEnabled | |||
| std::array<int16_t, 3> data_raw_xl {}; | |||
| std::array<int16_t, 3> data_raw_gy {}; | |||
| data_available_callback_t _on_data_available_callback {}; | |||
| std::function<void()> _on_data_available_wrapper_callback {}; | |||
There was a problem hiding this comment.
est-ce qu'on a vraiment besoin de stocker le wrapper?
dans le code d'avant, l'équivalent du wrapper était en auto local lambda et ça marchait très bien.
je préfère qu'on garde ça plutôt que d'introduire une nouvelle variable dont la différence avec _on_data_available_callback est pas clair à moins de lire le code en détails.
There was a problem hiding this comment.
de ce que je comprends c'est parce que quand tu disable tu mets in callback vide, mais je vois pas pourquoi tu fais ça puisque tu as disabled.
disable ça veut pas forcément dire mettre un callback vide.
est-ce qu'on est obligé de mettre un callback vide quand on disable?
si on en a pas besoin, on le fait pas et on fait que disable
si on est sûr qu'on a besoin d'un callback vide, la question qu'il faut se poser c'est
est-ce que c'est user facing (pour les devs) ou c'est internal facing (pour le code en lui même)
si c'est user facing, il faut une API claire pour mettre un callback vide:
- soit l'utilisateur va register le callback vide
- soit on a une fonction
clearRegisteredCallbackouresetRegisteredCallback
si c'est en interne (et pour moi ce que je vois du code c'est surtout interne, il suffit d'avoir ça :
void CoreIMU::enableOnDataReadyInterrupt()
{
lsm6dsox_pin_int1_route_t lsm6dsox_int1 {
.drdy_xl = PROPERTY_ENABLE,
.den_flag = PROPERTY_ENABLE,
};
lsm6dsox_pin_int1_route_set(&_register_io_function, lsm6dsox_int1);
auto on_data_ready_callback = [this] {
auto timestamp = rtos::Kernel::Clock::now();
_event_queue.call([this, timestamp] { onDataAvailableHandler(timestamp); });
};
setDataReadyInterruptCallback(on_data_ready_callback);
}
YannLocatelli
force-pushed
the
yann/rename/imu/rename-corelsm6dsox-into-coreimu
branch
2 times, most recently
from
cff58d4
to
3d2a51f
Compare
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
2 times, most recently
from
9d2d183
to
8c6b29a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
from
8c6b29a
to
80d420e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ladislas
force-pushed
the
yann/rename/imu/rename-corelsm6dsox-into-coreimu
branch
from
3d2a51f
to
2270bd0
Compare
Base automatically changed from
yann/rename/imu/rename-corelsm6dsox-into-coreimu
to
develop
July 18, 2024 21:34
drdy is confusing since it is not only for data ready
More explicit and understandable
YannLocatelli
force-pushed
the
yann/refactor/imu/standardize-naming
branch
from
80d420e
to
25dfefe
Compare
🔖 Version comparison
|
📈 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
|
|
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.
Requis