Conversation
31e316b to
25eef29
Compare
|
Can you correct all the mistake identified by ruff ? |
b9d0624 to
d1f20c1
Compare
e84f9a7 to
488ba5b
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new MicroPython library under lib/lis2mdl/ implementing a driver for the ST LIS2MDL 3‑axis magnetometer, along with documentation and example scripts.
Changes:
- Introduces the
LIS2MDLdriver (I2C register config, raw/µT reads, calibration helpers, heading + tilt compensation utilities). - Adds LIS2MDL register/constants definitions and package exports.
- Provides a README and multiple example/test scripts for validation and usage.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/lis2mdl/manifest.py | Package manifest stub (currently empty). |
| lib/lis2mdl/lis2mdl/device.py | Main LIS2MDL driver implementation (config, readouts, calibration, heading, power/reset). |
| lib/lis2mdl/lis2mdl/const.py | Register addresses and constants for LIS2MDL. |
| lib/lis2mdl/lis2mdl/init.py | Package export (LIS2MDL). |
| lib/lis2mdl/exemple/magnet_test.py | Large interactive test script covering reads, calibration, heading, power/reset. |
| lib/lis2mdl/exemple/magnet_fieldForce.py | Example loop printing magnetic field magnitude. |
| lib/lis2mdl/exemple/magnet_compass.py | Example loop printing heading + direction label. |
| lib/lis2mdl/README.md | Feature overview, wiring, installation, and usage docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Questions/remarques à la relecture de la PR
manifest.py est vide
Le fichier existe mais est vide. Tous les autres drivers complétés (hts221, apds9960, bq27441, etc.) déclarent un metadata() + package(). Il faudrait :
metadata(
description="Driver for the LIS2MDL 3-axis magnetometer",
version="0.0.1",
)
package("lis2mdl")
Répertoire exemple/ au lieu de examples/
Tous les autres drivers utilisent examples/ (en anglais, au pluriel). Ici c'est exemple/ (français, singulier). A renommer pour la cohérence du projet.
Valeurs de calibration en dur (lignes 12-17 de device.py)
x_off = -132.0
y_off = -521.5
z_off = -891.0
x_scale = 273.0
y_scale = 313.5
z_scale = 295.0
Ces valeurs sont spécifiques à un capteur particulier. Pour un driver générique distribué en bibliothèque, les défauts devraient être neutres (0.0 pour les offsets, 1.0 pour les scales). L'utilisateur calibre ensuite son propre capteur. Avec les valeurs actuelles, un utilisateur qui oublie de calibrer obtiendra des résultats faux sans avertissement.
_bits définie en double dans magnet_test.py
La fonction _bits est définie ligne 24 et ligne 517 avec le même corps. La deuxième définition est morte ou écrase la première inutilement. Il faut supprimer le doublon (ligne 517).
Convention de nommage mixte dans device.py
Le driver mélange deux styles :
- camelCase : setReg (L147), _MAG_LSB_TO_uT (L190)
- snake_case : set_mode, set_odr, read_magnet_raw, etc.
Cependant, en regardant le driver existant hts221/device.py, on constate que setReg/getReg en camelCase est une convention existante du projet. Donc setReg est acceptable pour la cohérence interne, mais les nouvelles méthodes devraient idéalement suivre un seul style.
bare except (lignes 39 et 559 de device.py)
try:
time.sleep_ms(10)
except:
pass
L'intention est de supporter à la fois MicroPython (sleep_ms) et CPython (sleep), mais le bare except avale toutes les exceptions, y compris KeyboardInterrupt. Même si E722 est ignoré par ruff dans ce projet, c'est une pratique discutable.
Import wildcard from lis2mdl.const import *
Ignoré par la config ruff (F403/F405), et c'est le même pattern utilisé dans tous les autres drivers (from hts221.const import *). C'est donc cohérent avec le projet — pas de changement nécessaire.
class LIS2MDL(object)
En Python 3, hériter de object est implicite. Cependant, hts221/device.py fait la même chose (class HTS221(object)). C'est donc une convention du projet. On peut le signaler comme amélioration cosmétique mais ce n'est pas bloquant.
Format des commits
Les messages passent la regex CI (^[^!]+: [A-Za-z]+.+ .+.$), donc pas de blocage. Quelques remarques mineures :
Espace avant le : dans 2 commits (lis2mdl : fix...) — inconsistant avec les autres
Typo "exemple" au lieu de "example" dans les messages
Le message "initialise LIS2MDL driver." est un peu vague pour un premier commit
Commentaire erroné dans magnet_fieldForce.py:1)
# exemple of heading reading with direction label
C'est un copier-coller de magnet_compass.py. Le commentaire devrait décrire la lecture de la force du champ magnétique, pas le heading.
le 0x80 dans read_magnet (device.py:221)
buf = self.i2c.readfrom_mem(self.address, LIS2MDL_OUTX_L_REG | 0x80, 6)
Le | 0x80 est le bit d'auto-incrémentation pour la lecture multi-octets. C'est correct pour le LIS2MDL, mais ce magic number mériterait une constante dans const.py (ex: LIS2MDL_AUTO_INC = const(0x80)) pour la lisibilité.
le soft reset dans init (device.py:36)
self.setReg(0x20, LIS2MDL_CFG_REG_A) # SOFT_RST=1 (not 0x10)
Le commentaire dit not 0x10, ce qui est confus. 0x20 = bit 5 = SOFT_RST, c'est correct. Mais le commentaire semble être un résidu de debug. De plus, 0x20 devrait être 1 << 5 ou une constante nommée pour être cohérent avec le reste du code qui utilise des shifts de bits explicites.
nedseb
left a comment
There was a problem hiding this comment.
Re-review après corrections
Bon travail sur les corrections ! Le manifest, le renommage examples/, les calibrations neutres et la typo INT_CTRL_REG sont OK.
Il reste 4 points mineurs à corriger avant d'approuver.
Still doesnt point to north but can see rotation
Axes X and Y were inverted which caused a 90° heading offset. Calibration is required before usage because offsets depend on the board.
|
🎉 This PR is included in version 0.0.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Closes #7
Add LIS2MDL magnetometer driver
This pull request introduces a new MicroPython driver for the STMicroelectronics LIS2MDL 3-axis magnetometer.
The driver provides an API for reading magnetic field data, calibration, heading computation, and power management via the I²C interface.