introduce PX4 sensor classes and use to simplify mpu6000 #11216

dagar · 2019-01-14T21:23:53Z

WIP

This PR adds sensor driver classes that satisfy what PX4 requires from each type of driver. With these we can simplify the majority of drivers in the system that have manually duplicated the majority of this logic many times.

More importantly once all drivers start using these classes it will be relatively easy to start making more interesting changes.
Examples

split gyro publication to enable running the rate control much faster
add a high rate raw logging mechanism
kill off the remaining IOCTLs

dagar · 2019-01-15T01:10:30Z

The overall diff is large, but if you look at it per commit you'll see the changes to mpu6000 are actually relatively straightforward and mostly deletions. Most of the removed code now exists in PX4Accelerometer/PX4Gyroscope (to be shared with all drivers), or was removed entirely (eg poll rate setup via ioctl).

davids5

@dagar - this is moving in a good direction.

davids5 · 2019-01-15T17:23:05Z

src/lib/drivers/accelerometer/PX4Accelerometer.cpp

+
+	// force initial publish to allocate uORB buffer
+	// TODO: can be removed once all drivers are in threads
+	_sensor_accel_pub.update();


@dagar Can any calls in the constructor fail? If so Keeping the init() phase would make sense.

The initial publish (advertise) could fail if out of memory, but even now in master that's only handled with a warning in init. At least when using the uORB publication wrappers like this it will try the advertise again next publish.

Since this will be a repeated pattern and constructors can not return a value. I would separate the construtor and the things that can return errors. This gives us a LCD for the embedded no exception model.

src/drivers/imu/mpu6000/mpu6000.h

davids5 · 2019-01-15T17:27:40Z

src/drivers/imu/mpu6000/mpu6000_main.cpp

+	int device_type = MPU_DEVICE_TYPE_MPU6000;
+	enum Rotation rotation = ROTATION_NONE;
+
+	while ((ch = px4_getopt(argc, argv, "T:XISsZzR:a:", &myoptind, &myoptarg)) != EOF) {


Can we get a utility class to do this and the bus definitions?

dakejahl · 2019-01-17T21:32:26Z

Let's kill the *_main.cpp files for all drivers ASAP. Maybe with ModuleBaseMulti, or maybe even a new base class that isn't templated.

Otherwise this looks pretty good.

dakejahl · 2019-01-17T21:38:20Z

src/drivers/imu/mpu6000/MPU6000.cpp

+
+	state = px4_enter_critical_section();
+	_reset_wait = hrt_absolute_time() + 30000;
+	px4_leave_critical_section(state);


Is this really necessary?

Well it is 64 bit operation on a 32 bit CPU. For a variable operated on in the foreground and used in the background. I would guess it is. Have a look at the assembly to know for sure,

dakejahl · 2019-01-17T21:39:10Z

src/drivers/imu/mpu6000/MPU6000.cpp

+		// Enable I2C bus or Disable I2C bus (recommended on data sheet)
+		write_checked_reg(MPUREG_USER_CTRL, is_i2c() ? 0 : BIT_I2C_IF_DIS);
+
+		px4_leave_critical_section(state);


I am confused about why there are so many of these enter/exit critical sections.

If reset were only called of the HRT's ISR, then enter/exit would do nothing (but kill the real-time response of the system) But I believe rest use to be called from the main. So enter/exit makes the operation atomic that insures the timing. I doubt this is close to correct for the new calling structure. Moreover this may have ONLY been needed because the chip was powered by it's IO pins prior to VDD 3v3 Sensors voltage being applied.

We should do a few things here:

Do the correct power management. There was a PR that came in did this. But was later augmented because of some clones were rebooting. I shared an approach with @dagar on how to do this correctly.

Do a call graph so we can see how the functions in the driver are called. Then comment each one, so there is a clear understanding of the context and thread the functions run on (is this an ISR, is this on Init's thread, is this on a workqueue dispatch or all 3)

Design the chip rest and sensors reset so they work and do not kill the RT of the system.

Are we doing the correct power management on boards other than fmu-v2/v3?

We should be but an audit is warranted.

dakejahl · 2019-01-17T21:41:29Z

src/drivers/imu/mpu6000/MPU6000.cpp

+	// 2000 deg/s = (2000/180)*PI = 34.906585 rad/s
+	// scaling factor:
+	// 1/(2^15)*(2000/180)*PI
+	_px4_gyro.set_scale(0.0174532f / 16.4f);//1.0f / (32768.0f * (2000.0f / 180.0f) * M_PI_F);


Magic numbers? Do we want to clean up the code or just implement the new architecture? It does seem like a hassle, especially since this is pretty legacy at this point

Yes it is but at least well documented :) We could move this to a const. But Keep the comments with it.

dakejahl · 2019-01-17T21:45:30Z

src/drivers/imu/mpu6000/MPU6000.cpp

+
+	// get baseline values without self-test enabled
+	for (uint8_t i = 0; i < repeats; i++) {
+		up_udelay(1000);


Loop delay from an ISR...?

Gosh I hope not. See comment about call graph.

dakejahl · 2019-01-17T21:53:36Z

src/drivers/imu/mpu6000/MPU6000.cpp

+		hrt_cancel(&_call);
+
+	} else {
+#ifdef USE_I2C


Eliminating this kinda crap too would be nice. I'm going to end my review here, there is a lot of cruft in this driver and probably not worth trying to clean up and then regression test it.

I have suggested we have create a single schedule API. Then there would be one cancel. The caller can configure things such as RT, LP, HP. The RT use the hrt, the LP and HP use work queues. We can also have FIFO, RR, Single and a priority.

davids5

@dagar can we get one more level of common code to a PX4SensorBase class?

davids5 · 2019-01-18T12:05:31Z

src/drivers/imu/mpu6000/MPU6000.cpp

+		// Enable I2C bus or Disable I2C bus (recommended on data sheet)
+		write_checked_reg(MPUREG_USER_CTRL, is_i2c() ? 0 : BIT_I2C_IF_DIS);
+
+		px4_leave_critical_section(state);


If reset were only called of the HRT's ISR, then enter/exit would do nothing (but kill the real-time response of the system) But I believe rest use to be called from the main. So enter/exit makes the operation atomic that insures the timing. I doubt this is close to correct for the new calling structure. Moreover this may have ONLY been needed because the chip was powered by it's IO pins prior to VDD 3v3 Sensors voltage being applied.

We should do a few things here:

Do the correct power management. There was a PR that came in did this. But was later augmented because of some clones were rebooting. I shared an approach with @dagar on how to do this correctly.

Do a call graph so we can see how the functions in the driver are called. Then comment each one, so there is a clear understanding of the context and thread the functions run on (is this an ISR, is this on Init's thread, is this on a workqueue dispatch or all 3)

Design the chip rest and sensors reset so they work and do not kill the RT of the system.

davids5 · 2019-01-18T12:09:09Z

src/drivers/imu/mpu6000/MPU6000.cpp

+
+	state = px4_enter_critical_section();
+	_reset_wait = hrt_absolute_time() + 30000;
+	px4_leave_critical_section(state);


Well it is 64 bit operation on a 32 bit CPU. For a variable operated on in the foreground and used in the background. I would guess it is. Have a look at the assembly to know for sure,

davids5 · 2019-01-18T12:12:28Z

src/drivers/imu/mpu6000/MPU6000.cpp

+	// 2000 deg/s = (2000/180)*PI = 34.906585 rad/s
+	// scaling factor:
+	// 1/(2^15)*(2000/180)*PI
+	_px4_gyro.set_scale(0.0174532f / 16.4f);//1.0f / (32768.0f * (2000.0f / 180.0f) * M_PI_F);


Yes it is but at least well documented :) We could move this to a const. But Keep the comments with it.

davids5 · 2019-01-18T12:13:57Z

src/drivers/imu/mpu6000/MPU6000.cpp

+
+	// get baseline values without self-test enabled
+	for (uint8_t i = 0; i < repeats; i++) {
+		up_udelay(1000);


Gosh I hope not. See comment about call graph.

davids5 · 2019-01-18T12:21:37Z

src/drivers/imu/mpu6000/MPU6000.cpp

+		hrt_cancel(&_call);
+
+	} else {
+#ifdef USE_I2C


I have suggested we have create a single schedule API. Then there would be one cancel. The caller can configure things such as RT, LP, HP. The RT use the hrt, the LP and HP use work queues. We can also have FIFO, RR, Single and a priority.

davids5 · 2019-01-18T12:29:29Z

src/lib/drivers/accelerometer/PX4Accelerometer.cpp

+void PX4Accelerometer::set_device_type(uint8_t devtype)
+{
+	// current DeviceStructure
+	union device::Device::DeviceId device_id;


why is this not a class?

davids5 · 2019-01-18T12:33:07Z

src/lib/drivers/accelerometer/PX4Accelerometer.cpp

+	device_id.devid_s.devtype = devtype;
+
+	// copy back to report
+	_sensor_accel_pub.get().device_id = device_id.devid;


Shouldn't the pub have a DeviceId and a setter?

This really only exists for legacy reasons that I'd like to phase out eventually. It only exists because the mpu6000 has different device ids for the accel and gyro. https://github.com/PX4/Firmware/blob/master/src/drivers/drv_sensor.h#L64-L69

The only reason I kept it now is to avoid losing existing sensor calibrations.

davids5 · 2019-01-18T12:45:17Z

src/lib/drivers/device/integrator.h

@@ -108,12 +108,12 @@ class Integrator
 	/**
 	 * Set auto reset interval during runtime. This won't reset the integrator.
 	 *
-	 * @param auto_reset_interval	    	New reset time interval for the integrator.
+	 * @param auto_reset_interval	    	New reset time interval for the integrator (+- 10%).


We should set the % in a const header constant and docomment why the % is needed and 10 is chosen.

davids5 · 2019-01-18T12:48:19Z

src/lib/drivers/gyroscope/PX4Gyroscope.cpp

+	_sensor_gyro_pub.get().scaling = 1.0f;
+
+	// set software low pass filter for controllers
+	updateParams();


Same comment about construction results.

davids5 · 2019-01-18T13:01:43Z

src/lib/drivers/accelerometer/PX4Accelerometer.cpp

+
+void PX4Accelerometer::update(hrt_abstime timestamp, int16_t x, int16_t y, int16_t z)
+{
+	sensor_accel_s &report = _sensor_accel_pub.get();


This feels wrong. I see what the motivation is, but the getter is used to be the setter.
.ref() my feel better.

Is the timestamp stored in the report but not published suggesting something or does all this belong in the if? If it is the latter comment it.

I agree it's a bit weird, but already a fairly common pattern. The Publication class wraps a copy of the structure and the orb handling (advertise and publish).

I would propose we review the uORB::Publication wrappers directly and update the interface to be clear and hard to misuse. https://github.com/PX4/Firmware/blob/master/src/modules/uORB/Publication.hpp

dagar · 2019-01-18T15:01:04Z

Thanks for the detailed review everyone.

My goal here was mainly to gather the common portions into PX4Accelerometer and PX4Gyroscope as soon as possible so that we can 1) reduce the manually duplicated code across all drivers 2) be in a position where we can sanely make sensor pipeline changes.

As for next steps with mpu6000 cleanup I think the biggest missing piece is some kind of handler for the start/stop/status boilerplate. The reason I didn't go straight for module base is that there's still a larger "board manifest" aspect missing here. I want boards to be able to cleanly and simply define which sensors they and where (bus, spi chip select or i2c address, rotation, etc). All of that needs to be pulled out of each individual sensor driver.

dagar · 2019-01-18T15:38:52Z

I verified a full fresh calibration matches current master (parameters and published orb topics).

Fixes regression from #11216.

dagar force-pushed the pr-px4driver_classes branch from ad86815 to 35faa48 Compare January 14, 2019 21:29

dagar added 3 commits January 14, 2019 19:21

PX4 sensor driver helpers

9144d86

mpu6000 use new PX4Accelerometer and PX4Gyroscope classes

3ba05b0

mpu6000 split into separate main, header, implementation

a3fc81c

dagar force-pushed the pr-px4driver_classes branch from 35faa48 to a3fc81c Compare January 15, 2019 00:22

dagar requested review from davids5 and dakejahl January 15, 2019 01:09

davids5 approved these changes Jan 15, 2019

View reviewed changes

dakejahl approved these changes Jan 17, 2019

View reviewed changes

davids5 reviewed Jan 18, 2019

View reviewed changes

dagar merged commit 52c848a into master Jan 18, 2019

dagar deleted the pr-px4driver_classes branch January 18, 2019 15:39

weekly-digest bot mentioned this pull request Jan 19, 2019

Weekly Digest (12 January, 2019 - 19 January, 2019) #11244

Closed

bkueng added a commit that referenced this pull request Feb 11, 2019

fix MPU6000: restore orb priority

c04ba44

Fixes regression from #11216.

bkueng mentioned this pull request Feb 11, 2019

fix MPU6000: restore orb priority #11432

Merged

LorenzMeier pushed a commit that referenced this pull request Feb 11, 2019

fix MPU6000: restore orb priority

0acdffa

Fixes regression from #11216.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce PX4 sensor classes and use to simplify mpu6000 #11216

introduce PX4 sensor classes and use to simplify mpu6000 #11216

dagar commented Jan 14, 2019 •

edited by AuterionWrikeBot

Loading

dagar commented Jan 15, 2019 •

edited

Loading

davids5 left a comment

davids5 Jan 15, 2019

dagar Jan 17, 2019

davids5 Jan 17, 2019

davids5 Jan 15, 2019

dakejahl commented Jan 17, 2019 •

edited

Loading

dakejahl Jan 17, 2019

davids5 Jan 18, 2019

dakejahl Jan 17, 2019

davids5 Jan 18, 2019

dagar Jan 18, 2019

davids5 Jan 18, 2019

dakejahl Jan 17, 2019

davids5 Jan 18, 2019

dakejahl Jan 17, 2019

davids5 Jan 18, 2019

dakejahl Jan 17, 2019

davids5 Jan 18, 2019

davids5 left a comment

davids5 Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

dagar Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

davids5 Jan 18, 2019

dagar Jan 18, 2019 •

edited

Loading

dagar commented Jan 18, 2019

dagar commented Jan 18, 2019

introduce PX4 sensor classes and use to simplify mpu6000 #11216

introduce PX4 sensor classes and use to simplify mpu6000 #11216

Conversation

dagar commented Jan 14, 2019 • edited by AuterionWrikeBot Loading

dagar commented Jan 15, 2019 • edited Loading

davids5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakejahl commented Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davids5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

dagar commented Jan 18, 2019

dagar commented Jan 18, 2019

dagar commented Jan 14, 2019 •

edited by AuterionWrikeBot

Loading

dagar commented Jan 15, 2019 •

edited

Loading

dakejahl commented Jan 17, 2019 •

edited

Loading

dagar Jan 18, 2019 •

edited

Loading