-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LSM6DSOX - Get Configuration #10
Conversation
Goal set in README.md and ressources added.
lsm6dsox is split in two parts: accelerometer and gyroscope. Code is based on mbed version of lsm6dsox driver. In order to check the correct link to component, id is asked. Some conflicts appear with typedef that could be resolved with a common class to both (e.g. component-lsm6dsox). Some functions in driver can be explored next time.
Il manque l'intégration avec les nouvelles fonctions proposées par le driver situées dans le |
Aussi, il y a une redondance dans l'utilisation de l'I2C entre l'accéléromètre et le gyroscope qui peut être allégé avec une nouvelle classe qui gèrera n'importe quelle interface I2C. Exemple: |
@YannLocatelli tu peux mettre le lien? |
@YannLocatelli il y a plusieurs solutions:
Je testerai la |
Quoi que... maintenant que j'écris ça, je suis pas sûr que d'avoir une class Dans le cas là, on voit qu'on utilise deux fois la même chose mais c'est parce que c'est le même composant. Si on avait eux deux composants différents, on aurait peut être un en i2c et l'autre en spi, on se dirait pas qu'on a deux fois le même. C'est du code qu'on va pas réécrire plusieurs fois et qui est hardware spécifique, ça me choque en fait pas qu'il soit dédoublé. On peut laisser pour le moment. |
Le lien pour le Pour ce qui est de l'I2C, la structure actuelle sera la même pour les composants en I2C de ST vus jusque là, de sûr ça concernera le LSM303AGR et le HTS221 à minima. Sans oublier qu'elle sera encore dédoublée pour les classes MLC et FSM du LSM6DSOX à venir.
Je reste sur ça pour le moment, ça sera un point à revoir si on redéfinie la structure. |
*/ | ||
Accelerometer::Accelerometer(I2C *i2c, uint8_t address) : _i2c(i2c), _address(address) | ||
{ | ||
assert (i2c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
il faudra bien vérifier qu'on compile après avec NDEBUG
https://en.cppreference.com/w/cpp/error/assert
voir aussi si c'est conseillé en dehors des tests de mettre des assert dans le code. pas sûr que ce soit utile de crasher le robot si l'i2c ne marche plus. on voudrait plutôt continuer en faisant remonter le problème.
Trying to use some functions in "Basic configuration" group of the driver. For both accelerometer and gyroscope, 4 status were extracted from device: * Power mode * Output Data Range * Full Scale (or range) * Data Ready flag on interrupt
LKExp-Sensors-LSM6DSOX_Examples/lib/accelerometer/accelerometer.cpp
Outdated
Show resolved
Hide resolved
LKExp-Sensors-LSM6DSOX_Examples/lib/accelerometer/accelerometer.cpp
Outdated
Show resolved
Hide resolved
* @param dataReady flag | ||
* @retval 0 in case of success, an error code otherwise | ||
*/ | ||
LSM6DSOXAccStatusTypeDef Accelerometer::get_int_status(uint8_t *dataReady) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
le nom de la fonction est pas clair et ce qu'elle fait ne sont pas clair pour moi.
tu dis get_int_status
mais en même temps tu récupères des données en passant un pointeur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
une autre manière de découper pourrait être la suivante:
bool dataAvailable() {
if (lsm6dsox_all_sources_get(&_reg_ctx, &_int_status) == LSM6DSOX_Acc_OK) { // on check pour le positif, c'est plus naturel (en général)
// et on return true si c'est ok
return true;
}
else { // on garde le else pour bien montrer que c'est l'un ou l'autre.
return false;
}
}
// et plus tard dans le code
if (dataAvailable()) {
auto ret = getAvailableData(&newData);
if (ret == status::ok) {
analyzeData(&newData);
}
}
@@ -27,6 +27,15 @@ typedef enum | |||
LSM6DSOX_Acc_ERROR = -1 | |||
} LSM6DSOXAccStatusTypeDef; | |||
|
|||
typedef enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
il faut préféré les enum class
, voir ici: http://www.stroustrup.com/C++11FAQ.html#enum
C'est Bjarn lui même qui le dit ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ca a été ajouté dans le commit 34c9ea8
Même si ça allège le nom, les comparaison (avec !=
) ne passe plus, je suis obligé de cast à chaque comparaison avec des éléments externes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu peux donner un type à ton enum avec enum class: uint8_t
par exemple.
testes et dis moi si ça marche :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LKExp-Sensors-LSM6DSOX_Examples/lib/accelerometer/accelerometer.h
Outdated
Show resolved
Hide resolved
return LSM6DSOX_Gyro_ERROR; | ||
} | ||
|
||
switch((_status.ui.gy.odr & 0x0F)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
même remarque sur pourquoi et à quoi ça sert le &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le commentaire a été ajouté dans le commit 34c9ea8
@@ -38,6 +38,114 @@ void testID(Gyroscope *gyroscope) | |||
return; | |||
} | |||
|
|||
void testAllStatus(Accelerometer *accelerometer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je renommerai en testAccelerometerStatus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je resterai sur un même nom afin de fusionner cette fonction avec celle du gyroscope afin d'en avoir qu'une seule au lieu de deux, c'est pour ça que je suis resté sur un nom identique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, et dans ce cas là, qu'apporte le All
?
tu testes quoi dans cette méthode ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De base il n'y avait que les status
puis les int_status
se sont ajoutés. Donc pour éviter une confusion j'ai mis All
.
Après la restructuration, les interrupts seront considéré à part entière et il ne restera plus que les status
normaux, le All
perdra alors son sens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
justement je comprends pas bien ce découpage.
je pense (en relisant le code) que le problème vient aussi de status
qui est pas vraiment le status
(ouvert ou fermé, on/off, etc.) mais plutôt une config
qui d'ailleurs serait mieux encapsulée dans une struct
(pour le futur c'est mieux).
Ensuite tu get_config()
tout simplement si vraiment tu veux la récupérer quelque part.
Mais surtout, que ce soit en détails ou en struct, tout ça doit être des member variables (donc dans la classe) et tu dois pouvoir les récupérer avec get_power_mode()
Ca pose la question du error handling. A mon sens dans le design final, quand tu utiliseras accelerometer, tu veux pas avoir à savoir si l'accelerometer marche ou pas. si ça marche pas, ça doit s'arrêter et être remonté à un ErrorHandler au niveau de l'application qui pourra le transmettre à l'iPad.
C'est un vaste sujet qu'il faudra définir.
LKExp-Sensors-LSM6DSOX_Examples/lib/accelerometer/accelerometer.h
Outdated
Show resolved
Hide resolved
LKExp-Sensors-LSM6DSOX_Examples/lib/accelerometer/accelerometer.h
Outdated
Show resolved
Hide resolved
Power_Off_Acc = 4 | ||
}PowerModeAcc; | ||
|
||
typedef enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pareil ici:
enum class AccTransmissionStatus {
ok = 0,
error = -1,
overflow = -2
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
et d'ailleurs pourquoi -1 et -2 plutôt que 1 et 2?
0 c'est que tout est OK mais une raison particulière à aller dans le négatif?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L'idée d'affecter des enum
pour les OK/ERROR vient du stm32duino. Et ils avaient basé leur ERROR à -1.
Par contre je doute que ça ait plus de sens d'aller dans les négatifs, donc on peut aller dans les positifs sans soucis si ça a un intérêt d'optimisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l'intérêt de donner un numéro c'est qu'après on peut checker l'error avec try-block
.
Si on a des errors spécifiques à nous, on peut faire un enum. sinon c'est bien aussi d'utiliser les exceptions de std::
https://en.cppreference.com/w/cpp/error/exception qui intègre overflow et runtime error par exemple.
LSM6DSOX_Gyro_ERROR = -1 | ||
} LSM6DSOXGyroStatusTypeDef; | ||
|
||
typedef enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
même remarque pour enum class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fait dans le commit 34c9ea8
void testID(Gyroscope *gyroscope) | ||
{ | ||
uint8_t id; | ||
LSM6DSOXGyroStatusTypeDef success; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pareil pour le nom ici.
#include "accelerometer.h" | ||
#include "gyroscope.h" | ||
|
||
#define PIN_I2C_SDA (D14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je préfère les constexpr
que les #define
, au moins le compilateur peut les checker.
|
||
uint8_t dataReady; | ||
|
||
success = accelerometer->get_int_status(&dataReady); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c'est quoi la nature de dataReady
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu entends quoi par nature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qu'est-ce que ça représente? c'est quoi comme data? comment je peux l'utiliser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est relié à l'interrupt, il représente l'état de l'interrupt "Data Ready" de l'accéléromètre.
Dans l'état actuel il n'a que peu d'intérêt puisqu'au lieu de recevoir une notification au travers d'un interrupt (INT1
ou INT2
du LSM6DSOX), il y a un aller-retour pour aller l'interroger.
En dehors des pins interrupts, il pourra être utilisé avant de récupérer la data. Dans ce cas là, l'utilisateur n'aura pas besoin d'avoir connaissance de ce flag.
Data is read in raw value then converted to get "true" value in both accelerometer and gyroscope. In init, configuration is get before set in order to keep already set configuration.
* Add datasheet in README.md * Explain in commment the bitwise operation * Replace ternary operator with if/else * Replace "typedef enum" with "enum class" * Replace the variable name "success" with "outputStatus" in main and "ret" for the rest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentaires sur les difficultés rencontrés pour cet issue et de suggestions pour la suite.
int32_t LSM6DSOX_acc_io_write(void *handle, uint8_t WriteAddr, uint8_t *pBuffer, uint16_t nBytesToWrite) | ||
{ | ||
return ((Accelerometer *)handle)->io_write(pBuffer, WriteAddr, nBytesToWrite); | ||
} | ||
|
||
int32_t LSM6DSOX_acc_io_read(void *handle, uint8_t ReadAddr, uint8_t *pBuffer, uint16_t nBytesToRead) | ||
{ | ||
return ((Accelerometer *)handle)->io_read(pBuffer, ReadAddr, nBytesToRead); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentionné dans un appel, il serait intéressant de rentrer cette fonction à l'intérieur de la classe afin de simplifier son utilisation.
printf("Accelerometer is turned off\r\n"); | ||
break; | ||
} | ||
printf("Output Data Rate is (approximately) %dHz\r\n", (int)dataRate); //printf does not handle float, dataRate was reduced to int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sous mbed, printf()
n'arrive pas à afficher directement un float
. Ca a été casté en int
ici et n’est pas indispensable pour l’utilisation normal mais prévoir une meilleur alternative pour plus tard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c'est parce que de base, c'est minimal printf qui est utilisé et qui effectivement ne print pas les float pour limiter la taille mémoire que ça prend (j'ai pu les chiffres le float fait passer du simple au double voir pire).
pour le moment on touche pas, on garde le minimal et on verra par la suite.
//if (lsm6dsox_data_get(&_reg_ctx, NULL, &_status, &_data) != AccStatus::OK) //Lot of mistakes in the driver's function | ||
//{ | ||
// return AccStatus::ERROR; | ||
//} | ||
//*mg_X = _data.ui.xl.mg[0]; | ||
//*mg_Y = _data.ui.xl.mg[1]; | ||
//*mg_Z = _data.ui.xl.mg[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L'utilisation de la fonction lsm6dsox_data_get()
a été abandonnée, il y a encore trop d'erreur dans cette fonction.
Je ferrai remonté les erreurs en temps libre.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curieux de voir les erreurs! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_status.ui.xl.odr = _status.ui.xl.LSM6DSOX_XL_UI_52Hz_LP; | ||
_status.ui.xl.fs = _status.ui.xl.LSM6DSOX_XL_UI_4g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La forme est illisible, la faute à un enum
anonyme imbriquée dans plusieurs struct
anonymes. Le côté anonyme signifie qu'il n'y a pas de nom, ça n'empêche pas d'avoir un type.
On peut retrouver ladite structure ici.
Si il y a un moyen d'accéder au enum
sans avoir à passer les autres struct
je suis preneur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
si tu parles de _status.ui.xl.odr
je ne suis pas d'accord, pour le coup c'est ultra standard comme forme (on peut reprocher les abréviations pas claires mais c'est très spécifique), avec les .
. En Swift on aura que ça.
Pour le coup je trouve que leur structure est assez propre.
De toute manière, ce genre de chose ne sera écrit qu'une fois et bien documenté. Après on en parlera plus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je parlais plus de la partie droite de l'affectation : _status.ui.xl.LSM6DSOX_XL_UI_52Hz_LP
On peut voir une redondance des termes ui
et xl
et la valeur de l'enum ne peut être affecté qu'à partir d'un objet, ici _status
.
Ca implique que laissé tel quel, pour passer en paramètre il faut entrer _status.ui.xl.LSM6DSOX_XL_UI_52Hz_LP
plutôt que simplement LSM6DSOX_XL_UI_52Hz_LP
if (lsm6dsox_mode_get(&_reg_ctx, NULL, &_status) != (int32_t)AccStatus::OK) | ||
{ | ||
return AccStatus::ERROR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
La fonction proposée dans le driver récupère TOUTE la configuration, contrairement à la version sous mbed qui configure UN PAR UN chaque paramètre.
La raison pour laquelle il n'y a pas de set_status
est dû au choix à faire entre ces deux manières (configurer tous les paramètres en une fois ou un par un).
Il faut aussi prendre en compte un commentaire fait plus haut qui implique un accès lourd aux valeurs (enum
) de la configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je pense que le problème vient surtout du design et du manque d'encapsulation.
ta class Accelerometer
devrait avoir en private member
:
- soit une struct
config
- soit des membres séparés
// Soit une struct - plus propre je trouve
struct AccelerometerConfig {
AccPowerMode powerMode,
float dataRate,
uint16_t scale,
}
class LSM6DSOXAccelerometer { // ⚠️ je pense que c'est important de renommer les classes actuelles avec le prefix `LSM6DSOX` pour bien montrer le `tight coupling`
public:
// ... code
// Soit tu get la config au total dans une struct bien définie
void getConfig(AccelerometerConfig * config);
// Soit tu get les données individuelles
// tu peux d'ailleurs faire les deux si besoin, c'est pas l'un ou l'autre
AccPowerMode getPowerMode();
float getDataRate();
uint16_t getScale();
// et tu peux faire pareil avec le set
void setConfig(AccelerometerConfig * config) {
_config = * config;
}
void setPowerMode(AccPowerMode mode) {
// Soit
_config.powerMode = mode;
// ou
_powermode = mode;
}
private:
// Soit une struct - plus propre je trouve
Config _config;
// Soit des membres séparés
AccPowerMode _powerMode,
float _dataRate,
uint16_t _scale,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En fait, _status
est le _config
de ton exemple et la struct lsm6dsox_md_t
[Lien vers sa définition] qui correspond à la struct Config
dans ton exemple...
La question porte plus sur l'utilisation d'une telle struct ou de tout découper. On est favorable à une struct plutôt que des membres séparés, mais soit :
- on utilise la struct
lsm6dsox_md_t
proposé par le driver avec l'inconvénient mentionné un peu plus haut, - on réécrit une struct (exemple
Config
) pour d'une part palier au soucis du point précédent et d'autre part ajouter des paramètres supplémentaires tel que lePowerMode
.
Bien que la seconde nécessite plus de travail et de code, il permettra d'avoir un code plus clair.
switch((_status.ui.xl.odr & 0x0F)) //Value of odr is 0xYZ with Y the power mode and Z an enum frequence | ||
{ | ||
case 0: | ||
*dataRate = 0; | ||
break; | ||
case 1: | ||
*dataRate = 12.5f; | ||
break; | ||
case 2: | ||
*dataRate = 26.0f; | ||
break; | ||
case 3: | ||
*dataRate = 52.0f; | ||
break; | ||
case 4: | ||
*dataRate = 104.0f; | ||
break; | ||
case 5: | ||
*dataRate = 208.0f; | ||
break; | ||
case 6: | ||
*dataRate = 416.0f; | ||
break; | ||
case 7: | ||
*dataRate = 833.0f; | ||
break; | ||
case 8: | ||
*dataRate = 1667.0f; | ||
break; | ||
case 9: | ||
*dataRate = 3333.0f; | ||
break; | ||
case 10: | ||
*dataRate = 6667.0f; | ||
break; | ||
case 11: | ||
*dataRate = 1.6f; | ||
break; | ||
default: | ||
ret = AccStatus::ERROR; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il n'existe pas de struct
/enum
pour associer la fréquence à un float
. Seulement est disponible un identifiant en hexadecimal qui est reconvertis ici.
Il faudra donc prévoir une fonction ou un dictionnaire (map
en C++) pour convertir dans les deux sens plus facilement, accessible de partout puisque ces paramètres seront souvent réutilisés.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu peux me pointer dans la doc où tout ça est décrit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah je comprends ! en fait tu veux fair ressortir le data rate dans un format qu'on peut lire?
@YannLocatelli j'ai refait un passage ce matin au réveil après une grande tasse de café. Il y a plusieurs choses qui me chiffonnent ;)
Tous ces points vont dans le sens du "on a été trop vite, on est pas parti dans le bon sens". Heureusement, pour résoudre tout ça, il suffit de faire ce qu'on aurait du faire dès le début... 🥁
Commençons par ça, un papier, un crayon, et un schema qui explique ce qu'on va faire et pourquoi on va le faire. |
Je me permets de remettre ici ce que j'ai pu en partie dire par téléphone:
Proposition de solution [1] Proposition de solution [2] (discuté par téléphone)
En attendant de répondre à ces demandes, je ferme cette PR. |
Le but est de sortir une version avec une simple connexion i2c et récupérer la configuration.
La difficulté ici est d'utiliser le driver et de faire une bonne première structure.