Shriyans406 commented on PR #19359: URL: https://github.com/apache/nuttx/pull/19359#issuecomment-4921681848
Dear @acassis and @linguini1, Thank you both very much for taking the time to review this PR and for providing such feedback. The existing mpu60x0.c driver is built around the legacy character/register driver model (sensor_ops_s). However, per the official Apache NuttX Sensor Driver Documentation, the legacy approach for new sensor drivers is no longer recommended. This new mpu6050_uorb.c (CONFIG_SENSORS_MPU6050) driver implements the modern lower-half uORB sensor framework (sensor_lowerhalf_s). It registers directly to standard uORB sensor topics (SENSOR_TYPE_ACCELEROMETER and SENSOR_TYPE_GYROSCOPE), allowing user applications to subscribe via standard uORB APIs cleanly without needing custom character device ioctls. On Wed, Jul 8, 2026 at 8:46 PM Matteo Golin ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > You should attach the logs from your testing to this PR. > ------------------------------ > > In include/nuttx/sensors/mpu6050.h > <https://github.com/apache/nuttx/pull/19359#discussion_r3545102722>: > > > +#define MPU6050_SMPLRT_DIV 0x19 > +#define MPU6050_CONFIG 0x1a > +#define MPU6050_GYRO_CONFIG 0x1b > +#define MPU6050_ACCEL_CONFIG 0x1c > +#define MPU6050_INT_ENABLE 0x38 > +#define MPU6050_INT_STATUS 0x3a > +#define MPU6050_ACCEL_XOUT_H 0x3b > +#define MPU6050_ACCEL_XOUT_L 0x3c > +#define MPU6050_ACCEL_YOUT_H 0x3d > +#define MPU6050_ACCEL_YOUT_L 0x3e > +#define MPU6050_ACCEL_ZOUT_H 0x3f > +#define MPU6050_ACCEL_ZOUT_L 0x40 > +#define MPU6050_TEMP_OUT_H 0x41 > +#define MPU6050_TEMP_OUT_L 0x42 > +#define MPU6050_GYRO_XOUT_H 0x43 > +#define MPU6050_GYRO_XOUT_L 0x44 > +#define MPU6050_GYRO_YOUT_H 0x45 > > Register definitions should only go in the private C file, they don't need > to be accessible via the public API. > ------------------------------ > > In drivers/sensors/Kconfig > <https://github.com/apache/nuttx/pull/19359#discussion_r3545106216>: > > > > config MPU60X0_DLPF_CFG > int "MPU60x0 digital low-pass filter bandwidth" > default 1 > ---help--- > DLPF_CFG[2..0] > DLPF_CFG: digital low-pass filter bandwidth > - (see datasheet, it's ... complicated) > + (see datasheet, its ... complicated) > > it's is correct here. > ------------------------------ > > In drivers/sensors/Kconfig > <https://github.com/apache/nuttx/pull/19359#discussion_r3545109798>: > > > @@ -1533,6 +1533,13 @@ config MPU60X0_ACCEL_AFS_SEL > > endif # SENSORS_MPU60X0 > > +config SENSORS_MPU6050 > > How is your support different from the MPU60X0 support? > ------------------------------ > > In drivers/sensors/mpu6050_uorb.c > <https://github.com/apache/nuttx/pull/19359#discussion_r3545136154>: > > > + int freq; > + mutex_t dev_lock; > +}; > + > +struct mpu6050_sensor_s > +{ > + struct sensor_lowerhalf_s lower; > + float scale; > + FAR struct mpu6050_dev_s *dev; > + bool enabled; > +}; > + > +struct mpu6050_uorb_dev_s > +{ > + struct mpu6050_dev_s base; > + struct mpu6050_sensor_s priv[MPU6050_MAX_IDX]; > > Why do it this way? Just add two members; accel and gyro. It is more > readable. > ------------------------------ > > In drivers/sensors/mpu6050_uorb.c > <https://github.com/apache/nuttx/pull/19359#discussion_r3545140682>: > > > + FAR struct file *filep, bool enable); > +static int mpu6050_fetch(FAR struct sensor_lowerhalf_s *lower, > + FAR struct file *filep, > + FAR char *buffer, size_t buflen); > +static int mpu6050_control(FAR struct sensor_lowerhalf_s *lower, > + FAR struct file *filep, > + int cmd, unsigned long arg); > + > +/**************************************************************************** > + * Private Data > + ****************************************************************************/ > + > +static const struct sensor_ops_s g_mpu6050_ops = > +{ > + .activate = mpu6050_activate, > + .fetch = mpu6050_fetch, > > How fast can this sensor sample? It might be worth it to have a kthread or > interrupt-based approach so the user doesn't have to poll. > ------------------------------ > > On drivers/sensors/mpu6050_uorb.c > <https://github.com/apache/nuttx/pull/19359#discussion_r3545147768>: > > Please document this sensor. > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/nuttx/pull/19359?email_source=notifications&email_token=BMTFX7SVC3DZJE4OC7QEQAT5DZQTHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRVGU2DKMZQGEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4655453013>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/BMTFX7RZ4FSMYPCTAXN27W35DZQTHAVCNFSNUABFKJSXA33TNF2G64TZHMZDEOBRGAZTENZTHNEXG43VMU5TIOBSG4ZTCOBVGI42C5QC> > . > Triage notifications, keep track of coding agent tasks and review pull > requests on the go with GitHub Mobile for iOS > <https://github.com/notifications/mobile/ios/BMTFX7TW6ZDQ3MZXWTCAMB35DZQTHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRVGU2DKMZQGEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> > and Android > <https://github.com/notifications/mobile/android/BMTFX7T27AZBIZAALP6P7735DZQTHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRVGU2DKMZQGEZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. > Download it today! > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
