Crazyflie drivers #9462

DanielePettenuzzo · 2018-05-14T21:01:41Z

This PR enables Crazyflie's optical flow deck by adding the drivers for the optical flow module (pmw3901) and the range sensor (vl53lxx). Moreover adds support for logging on sd card and increases the imu sampling rate.

…gyro data during simple rotation

…about 10ms)

…and device in pmw3901 driver)

… topic_listener

…rd on crazyflie)

… 200 Hz for this board

dagar · 2018-05-14T21:51:15Z

src/drivers/imu/mpu9250/mpu9250.cpp

@@ -139,7 +139,11 @@ MPU9250::MPU9250(device::Device *interface, device::Device *mag_interface, const
 	_gyro_range_scale(0.0f),
 	_gyro_range_rad_s(0.0f),
 	_dlpf_freq(MPU9250_DEFAULT_ONCHIP_FILTER_FREQ),
+#ifdef CONFIG_ARCH_BOARD_CRAZYFLIE


Can we find a way to do this without board specific defines in drivers? How about defaulting to 200 Hz for I2C?

@dagar I can get it to run up to 330Hz on the crazyflie but at this frequency I get some weird behavior because the delay between two samples becomes quite variable. It seems to work well between 200 and 250Hz. Could changing this create issues on other architectures?

Isn't the important difference SPI vs I2C rather than anything crazyflie specific?

Ok I can fix this

dagar · 2018-05-14T21:51:41Z

src/drivers/pmw3901/CMakeLists.txt

+	MODULE drivers__pmw3901
+	MAIN pmw3901
+	COMPILE_FLAGS
+		-Wno-sign-compare


dagar · 2018-05-14T21:52:01Z

src/drivers/pmw3901/CMakeLists.txt

+	SRCS
+		pmw3901.cpp
+	DEPENDS
+		platforms__common


This can be deleted, although it's harmless if left.

dagar · 2018-05-14T21:56:25Z

src/drivers/pmw3901/pmw3901.cpp

+
+#include <conversion/rotation.h>
+
+#include <systemlib/perf_counter.h>


<systemlib/perf_counter.h> -> <perf/perf_counter.h>

DanielePettenuzzo · 2018-05-14T22:28:10Z

FYI @barzanisar

RomanBapst · 2018-05-15T06:08:50Z

src/drivers/pmw3901/pmw3901.cpp

+
+	usleep(1000);
+
+	// set performance optimization registers


@DanielePettenuzzo Any reference we can put here?

not really but I can look more

RomanBapst · 2018-05-15T06:10:33Z

src/drivers/pmw3901/pmw3901.cpp

+	/* For devices competing with NuttX SPI drivers on a bus (Crazyflie SD Card expansion board) */
+	SPI::set_lockmode(LOCK_THREADS);
+
+	/* do I2C init (and probe) first */


Comment lies

RomanBapst · 2018-05-15T06:17:07Z

src/drivers/pmw3901/pmw3901.cpp

+	/* notify anyone waiting for data */
+	poll_notify(POLLIN);
+
+	ret = OK;


Maybe just initialize this to 'OK' at the top?

RomanBapst · 2018-05-15T06:18:37Z

src/drivers/pmw3901/pmw3901.cpp

+	_reports->flush();
+
+	/* schedule a cycle to start things */
+	work_queue(LPWORK, &_work, (worker_t)&PMW3901::cycle_trampoline, this, 1000);


Can we use USEC2TICK(PMW3901_SAMPLE_RATE) here?

PMW3901_SAMPLE_RATE is 10ms.. I will create a define for 1ms and use the USEC2TICK

RomanBapst · 2018-05-15T06:31:35Z

src/drivers/pmw3901/pmw3901.cpp

+		goto fail;
+	}
+
+	exit(0);


@dagar If we see a chance of this driver ever running in on a linux platform should we then get rid of this NuttX specific stuff?

In general I would suggest assuming that possibility nearly everywhere as we go and even bother including them in the posix builds (sitl https://github.com/PX4/Firmware/blob/master/cmake/configs/posix_sitl_default.cmake#L6, rpi, etc).

At a minimum it will cause the code to be checked by tools like coverity and clang-tidy.

All that's actually required to "port" most NuttX drivers to Linux/QuRT is to use the px4 headers instead of anything NuttX specific, and prevent it from directly exiting (exit, err).

bkueng

Reviewed, the general structure looks good, but needs a second pass to cleanup smaller things.

bkueng · 2018-05-15T08:53:07Z

src/drivers/boards/crazyflie/spi.c

+		return -ENODEV;
+	}
+
+	//#ifdef CONFIG_MMCSD


Why is this commented?
Same for message("[boot] FAILED to bind SPI port 1 to the MMCSD driver\n");

bkueng · 2018-05-15T08:53:27Z

src/drivers/boards/crazyflie/spi.c

+	/* SPI select is active low, so write !selected to select the device */
+
+	int sel = (int) devid;
+	//ASSERT(PX4_SPI_BUS_ID(sel) == PX4_SPI_BUS_EXPANSION);


Please remove if not needed

bkueng · 2018-05-15T08:54:16Z

src/drivers/boards/crazyflie/spi.c

+	stm32_configgpio(GPIO_SPI1_MOSI_OFF);
+
+	/* set the sensor rail off */
+	//stm32_gpiowrite(GPIO_VDD_3V3_SENSORS_EN, 0);


Please remove or uncomment if needed (same below)

bkueng · 2018-05-15T08:55:03Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

@@ -0,0 +1,1223 @@
+/****************************************************************************
+ *
+ *   Copyright (c) 2013-2017 PX4 Development Team. All rights reserved.


Requires updating

bkueng · 2018-05-15T08:56:20Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+#define DYNAMIC_SPAD_NUM_REQUESTED_REF_SPAD_REG				0x4E
+#define GLOBAL_CONFIG_REF_EN_START_SELECT_REG				0xB6
+#define GLOBAL_CONFIG_SPAD_ENABLES_REF_0_REG				0xB0
+#define SYSTEM_INTERRUPT_CONFIG_GPIO_REG					0x0A


The tabs here are completely off, can you make sure these are aligned?

bkueng · 2018-05-15T09:11:19Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+		/* get a publish handle on the range finder topic */
+		struct distance_sensor_s ds_report;
+
+		_reports->get(&ds_report);


You are reading from _reports, but nothing has been added to the ringbuffer yet.

bkueng · 2018-05-15T09:37:32Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+		switch (ch) {
+		case 'R':
+			rotation = (uint8_t)atoi(myoptarg);
+			PX4_INFO("Setting sensor orientation to %d", (int)rotation);


This is a bit verbose: can you remove it?

bkueng · 2018-05-15T09:38:26Z

src/drivers/pmw3901/pmw3901.cpp

@@ -0,0 +1,944 @@
+/****************************************************************************
+ *
+ *   Copyright (c) 2013-2017 PX4 Development Team. All rights reserved.


requires updating

bkueng · 2018-05-15T09:41:04Z

src/drivers/pmw3901/pmw3901.cpp

+		delete _reports;
+	}
+
+	// if (_class_instance != -1) {


commented code: can you remove it if not needed?

bkueng · 2018-05-15T09:44:06Z

src/drivers/pmw3901/pmw3901.cpp

+		switch (ch) {
+		case 'R':
+			rotation = (uint8_t)atoi(myoptarg);
+			PX4_INFO("Setting sensor orientation to %d", (int)rotation);


a bit too verbose

DanielePettenuzzo · 2018-05-16T13:47:33Z

src/drivers/imu/mpu9250/mpu9250.cpp

+	 * make the integration autoreset faster so that we integrate just one
+	 * sample since the sampling rate is already low.
+	*/
+	if (is_i2c()) {


@bkueng @dagar this is how I reduce the sampling rate and decrease the integration interval when i2c is used for mpu9250. I also had to create the set_autoreset_interval function in the integrator class.

Looks good, but I'd use something like _accel_int.set_autoreset_interval(1000000 / 1000);, so that it does not depend on the macro.

Ok. Sounds good!

bkueng

Thanks for the updates. I noticed a few more things that require to be fixed.

bkueng · 2018-05-17T11:21:08Z

src/drivers/distance_sensor/vl53lxx/CMakeLists.txt

+	MODULE drivers__vl53lxx
+	MAIN vl53lxx
+	COMPILE_FLAGS
+		-Wno-sign-compare


Can you remove this?

bkueng · 2018-05-17T11:24:02Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+#define VL53LXX_RA_IDENTIFICATION_MODEL_ID 0xC0
+#define VL53LXX_IDENTIFICATION_MODEL_ID 0xEEAA
+
+#define VL53LXX_MS 1000 /*  1ms */


This is in us, so should be named VL53LXX_US

bkueng · 2018-05-17T11:25:59Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+		return ret;
+	}
+
+	ret = OK;


This is not needed

bkueng · 2018-05-17T11:26:56Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+	work_queue(LPWORK, &_work, (worker_t)&VL53LXX::cycle_trampoline, this, USEC2TICK(VL53LXX_MS));
+
+	/* notify about state change */
+	struct subsystem_info_s info = {};


info.timestamp is not set

bkueng · 2018-05-17T11:27:15Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+	info.ok = true;
+	info.subsystem_type = subsystem_info_s::SUBSYSTEM_TYPE_RANGEFINDER;
+
+	static orb_advert_t pub = nullptr;


Please use a class member instead of static

bkueng · 2018-05-17T11:29:32Z

src/drivers/pmw3901/pmw3901.cpp

+	info.ok = true;
+	info.subsystem_type = subsystem_info_s::SUBSYSTEM_TYPE_OPTICALFLOW;
+
+	static orb_advert_t pub = nullptr;


Please avoid static

bkueng · 2018-05-17T11:32:38Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+	}
+
+	/* read from the sensor */
+	ret = transfer(nullptr, 0, &val[0], length);


You can have a buffer overflow here if readRegisterMulti is called with length > 6. Can you directly use value instead of val?

bkueng · 2018-05-17T11:35:28Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

+
+	cmd[0] = reg_address;
+
+	memcpy(&cmd[1], &value[0], length);


This needs a length check, or an API documentation for writeRegisterMulti about the maximum length (length must not be more than 5).

bkueng

Good to go

dpettenuzzo and others added 22 commits May 14, 2018 22:51

created new distance sensor driver for vl53lxx

0cda469

vl53lxx driver: change to I2C4 on fmu-v5 (I2C connector)

0d6d828

created driver for pmw3901 optical flow module

827718b

added path to pmw3901 driver for fmu-v4pro cmake file

1e879b4

pmw3901 driver: fixed communication with sensor

ce002c7

pmw3901 driver: added integrator

fb23eae

vl53lxx driver: cleanup for pull request

a069248

pmw3901 driver: cleanup for pull request

8ada531

rcS: start vl53lxx and pmw3901 drivers

f5fea91

pmw3901 driver: multiply output of integrator by a constant to match …

271317b

…gyro data during simple rotation

vl53lxx driver: added work queue between measure and collect

250851b

vl53lxx driver: add saturation when distance goes beyond max_distance

cfa2ab7

pmw3901 driver: remove integrator and publish data at sampling rate (…

d606d53

…about 10ms)

vl53lxx and pmw3901 drivers: style fix

919b086

crazyflie: add spi expansion to board configurations (modify spi bus …

c55cbe9

…and device in pmw3901 driver)

vl53lxx driver: change I2C frequency to 400 kHz

74150f7

nuttx_crazyflie_default.cmake: add vl53lxx driver, pmw3901 driver and…

ab82a7b

… topic_listener

crazyflie: support for sd card board via SPI

60fba8d

pmw3901 driver: lock bus during transfers (avoid conflicts with sd ca…

6a49d3c

…rd on crazyflie)

crazyflie: increase imu reading rate

76e2b6e

crazyflie: removed crazyflie specific #ifdefs from mpu9250

7e45114

crazyflie: add probe() to vl53lxx driver and set imu sampling rate to…

bc3eba4

… 200 Hz for this board

DanielePettenuzzo requested review from bkueng, LorenzMeier and RomanBapst May 14, 2018 21:01

dagar reviewed May 14, 2018

View reviewed changes

src/drivers/pmw3901/CMakeLists.txt Outdated

MODULE drivers__pmw3901

MAIN pmw3901

COMPILE_FLAGS

-Wno-sign-compare

Copy link

Member

dagar May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed?

dagar reviewed May 14, 2018

View reviewed changes

RomanBapst reviewed May 15, 2018

View reviewed changes

bkueng reviewed May 15, 2018

View reviewed changes

DanielePettenuzzo added 2 commits May 16, 2018 09:49

crazyflie: optical flow and distace sensor driver fixes

37355dc

mpu9250: decrease sampling rate when using i2c

33164bc

DanielePettenuzzo commented May 16, 2018

View reviewed changes

mpu9250: remove define from integrator interval when using i2c

23aebf6

bkueng reviewed May 17, 2018

View reviewed changes

DanielePettenuzzo added 2 commits May 18, 2018 16:13

crazyflie: add downsampling to pwm3901 optflow driver

913abc9

crazyflie: clean up

1af47ef

LorenzMeier approved these changes May 20, 2018

View reviewed changes

bkueng approved these changes May 22, 2018

View reviewed changes

bkueng merged commit 9cad11d into master May 22, 2018

bkueng deleted the pr-crazyflie-drivers branch May 22, 2018 10:21

mrpollo mentioned this pull request Jun 5, 2018

v1.8 Changelog scratchpad #9593

Closed


		#include <conversion/rotation.h>

		#include <systemlib/perf_counter.h>

Crazyflie drivers #9462

Crazyflie drivers #9462

Conversation

DanielePettenuzzo commented May 14, 2018 • edited by AuterionWrikeBot 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

DanielePettenuzzo commented May 14, 2018

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

bkueng 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng 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

bkueng left a comment

Choose a reason for hiding this comment

DanielePettenuzzo commented May 14, 2018 •

edited by AuterionWrikeBot

Loading