i2c spi buses: enforce drivers to set default SPI/I2C bus frequency

Not a lot of drivers use the global default, which is somewhat arbitrary.
PX4 · Mar 18, 2020 · d6bb5b3 · d6bb5b3
1 parent 8ebde51
commit d6bb5b3
Show file tree

Hide file tree

Showing 13 changed files with 60 additions and 2 deletions.
diff --git a/platforms/common/i2c_spi_buses.cpp b/platforms/common/i2c_spi_buses.cpp
@@ -62,6 +62,10 @@ const char *BusCLIArguments::parseDefaultArguments(int argc, char *argv[])
 int BusCLIArguments::getopt(int argc, char *argv[], const char *options)
 {
 	if (_options[0] == 0) { // need to initialize
+		if (!validateConfiguration()) {
+			return EOF;
+		}
+
 		char *p = (char *)&_options;
 
 		if (_i2c_support) {
@@ -182,6 +186,23 @@ int BusCLIArguments::getopt(int argc, char *argv[], const char *options)
 	return ch;
 }
 
+bool BusCLIArguments::validateConfiguration()
+{
+	bool success = true;
+
+	if (_i2c_support && default_i2c_frequency == -1) {
+		PX4_ERR("Bug: driver %s does not set default_i2c_frequency", px4_get_taskname());
+		success = false;
+	}
+
+	if (_spi_support && default_spi_frequency == -1) {
+		PX4_ERR("Bug: driver %s does not set default_spi_frequency", px4_get_taskname());
+		success = false;
+	}
+
+	return success;
+}
+
 
 BusInstanceIterator::BusInstanceIterator(const char *module_name,
 		const BusCLIArguments &cli_arguments, uint16_t devid_driver_index)

diff --git a/platforms/common/include/px4_platform_common/i2c_spi_buses.h b/platforms/common/include/px4_platform_common/i2c_spi_buses.h
@@ -117,10 +117,12 @@ class BusCLIArguments
 	void *custom_data{nullptr}; ///< driver-specific custom argument
 
 	// driver defaults, if not specified via CLI
-	int default_spi_frequency{20 * 1000 * 1000}; ///< default spi bus frequency [Hz]
-	int default_i2c_frequency{400 * 1000}; ///< default i2c bus frequency [Hz]
+	int default_spi_frequency{-1}; ///< default spi bus frequency (driver needs to set this) [Hz]
+	int default_i2c_frequency{-1}; ///< default i2c bus frequency (driver needs to set this) [Hz]
 
 private:
+	bool validateConfiguration();
+
 	char _options[32] {};
 	int _optind{1};
 	const char *_optarg{nullptr};

diff --git a/src/drivers/barometer/dps310/dps310_main.cpp b/src/drivers/barometer/dps310/dps310_main.cpp
@@ -98,6 +98,7 @@ extern "C" int dps310_main(int argc, char *argv[])
 	using ThisDriver = DPS310;
 	BusCLIArguments cli{true, true};
 	cli.i2c_address = 0x77;
+	cli.default_i2c_frequency = 400000;
 	cli.default_spi_frequency = 10 * 1000 * 1000;
 
 	const char *verb = cli.parseDefaultArguments(argc, argv);

diff --git a/src/drivers/barometer/lps22hb/lps22hb_main.cpp b/src/drivers/barometer/lps22hb/lps22hb_main.cpp
@@ -89,6 +89,7 @@ extern "C" int lps22hb_main(int argc, char *argv[])
 {
 	using ThisDriver = LPS22HB;
 	BusCLIArguments cli{true, true};
+	cli.default_i2c_frequency = 400000;
 	cli.default_spi_frequency = 10 * 1000 * 1000;
 
 	const char *verb = cli.parseDefaultArguments(argc, argv);

diff --git a/src/drivers/barometer/lps25h/lps25h_main.cpp b/src/drivers/barometer/lps25h/lps25h_main.cpp
@@ -89,7 +89,9 @@ extern "C" int lps25h_main(int argc, char *argv[])
 {
 	using ThisDriver = LPS25H;
 	BusCLIArguments cli{true, true};
+	cli.default_i2c_frequency = 400000;
 	cli.default_spi_frequency = 11 * 1000 * 1000;
+
 	const char *verb = cli.parseDefaultArguments(argc, argv);
 
 	if (!verb) {

diff --git a/src/drivers/barometer/mpl3115a2/mpl3115a2_main.cpp b/src/drivers/barometer/mpl3115a2/mpl3115a2_main.cpp
@@ -67,6 +67,8 @@ extern "C" int mpl3115a2_main(int argc, char *argv[])
 {
 	using ThisDriver = MPL3115A2;
 	BusCLIArguments cli{true, false};
+	cli.default_i2c_frequency = 400000;
+
 	const char *verb = cli.parseDefaultArguments(argc, argv);
 
 	if (!verb) {

diff --git a/src/drivers/barometer/ms5611/ms5611_main.cpp b/src/drivers/barometer/ms5611/ms5611_main.cpp
@@ -96,6 +96,8 @@ extern "C" int ms5611_main(int argc, char *argv[])
 	int ch;
 	BusCLIArguments cli{true, true};
 	cli.type = MS5611_DEVICE;
+	cli.default_i2c_frequency = 400000;
+	cli.default_spi_frequency = 20 * 1000 * 1000;
 	uint16_t dev_type_driver = DRV_BARO_DEVTYPE_MS5611;
 
 	while ((ch = cli.getopt(argc, argv, "T:")) != EOF) {

diff --git a/src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp b/src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp
@@ -816,6 +816,7 @@ extern "C" __EXPORT int vl53lxx_main(int argc, char *argv[])
 	int ch;
 	using ThisDriver = VL53LXX;
 	BusCLIArguments cli{true, false};
+	cli.default_i2c_frequency = 400000;
 	cli.orientation = distance_sensor_s::ROTATION_DOWNWARD_FACING;
 
 	while ((ch = cli.getopt(argc, argv, "R:")) != EOF) {

diff --git a/src/drivers/imu/mpu6000/mpu6000_main.cpp b/src/drivers/imu/mpu6000/mpu6000_main.cpp
@@ -128,6 +128,7 @@ mpu6000_main(int argc, char *argv[])
 	using ThisDriver = MPU6000;
 	BusCLIArguments cli{true, true};
 	cli.type = MPU_DEVICE_TYPE_MPU6000;
+	cli.default_i2c_frequency = 400000;
 	cli.default_spi_frequency = 1000 * 1000; // low speed bus frequency
 
 	while ((ch = cli.getopt(argc, argv, "T:R:")) != EOF) {

diff --git a/src/drivers/irlock/irlock.cpp b/src/drivers/irlock/irlock.cpp
@@ -430,6 +430,7 @@ extern "C" __EXPORT int irlock_main(int argc, char *argv[])
 	using ThisDriver = IRLOCK;
 	BusCLIArguments cli{true, false};
 	cli.i2c_address = IRLOCK_I2C_ADDRESS;
+	cli.default_i2c_frequency = 400000;
 
 	const char *verb = cli.parseDefaultArguments(argc, argv);
 

diff --git a/src/drivers/magnetometer/ak09916/ak09916.cpp b/src/drivers/magnetometer/ak09916/ak09916.cpp
@@ -271,6 +271,7 @@ ak09916_main(int argc, char *argv[])
 	int ch;
 	using ThisDriver = AK09916;
 	BusCLIArguments cli{true, false};
+	cli.default_i2c_frequency = 400000;
 
 	while ((ch = cli.getopt(argc, argv, "R:")) != EOF) {
 		switch (ch) {

diff --git a/src/drivers/rpm/pcf8583/pcf8583_main.cpp b/src/drivers/rpm/pcf8583/pcf8583_main.cpp
@@ -66,6 +66,7 @@ extern "C" __EXPORT int pcf8583_main(int argc, char *argv[])
 {
 	using ThisDriver = PCF8583;
 	BusCLIArguments cli{true, false};
+	cli.default_i2c_frequency = 400000;
 
 	const char *verb = cli.parseDefaultArguments(argc, argv);
 

diff --git a/src/systemcmds/tests/test_i2c_spi_cli.cpp b/src/systemcmds/tests/test_i2c_spi_cli.cpp
@@ -110,6 +110,8 @@ bool I2CSPICLITest::test_basic()
 
 	{
 		BusCLIArguments cli{true, true};
+		cli.default_i2c_frequency = 11111;
+		cli.default_spi_frequency = 22222;
 		const char *argv[] = { "-S", "-b", "3", "stop"};
 		CLIArgsHelper cli_args(argv, 4);
 		const char *verb = cli.parseDefaultArguments(cli_args.argc, cli_args.argv);
@@ -121,6 +123,8 @@ bool I2CSPICLITest::test_basic()
 
 	{
 		BusCLIArguments cli{true, true};
+		cli.default_i2c_frequency = 11111;
+		cli.default_spi_frequency = 22222;
 		cli.i2c_address = 0xab;
 		const char *argv[] = { "start", "-X", "-a", "0x14"};
 		CLIArgsHelper cli_args(argv, 4);
@@ -139,6 +143,7 @@ bool I2CSPICLITest::test_invalid()
 	{
 		// SPI disabled, but SPI option provided
 		BusCLIArguments cli{true, false};
+		cli.default_i2c_frequency = 11111;
 		const char *argv[] = { "start", "-S"};
 		CLIArgsHelper cli_args(argv, 2);
 		const char *verb = cli.parseDefaultArguments(cli_args.argc, cli_args.argv);
@@ -148,15 +153,28 @@ bool I2CSPICLITest::test_invalid()
 	{
 		// Unknown argument
 		BusCLIArguments cli{true, true};
+		cli.default_i2c_frequency = 11111;
+		cli.default_spi_frequency = 22222;
 		const char *argv[] = { "start", "-I", "-x", "3"};
 		CLIArgsHelper cli_args(argv, 3);
 		const char *verb = cli.parseDefaultArguments(cli_args.argc, cli_args.argv);
 		ut_assert_true(verb == nullptr);
 	}
 
+	{
+		// default bus frequency not set
+		BusCLIArguments cli{true, true};
+		const char *argv[] = { "start", "-I"};
+		CLIArgsHelper cli_args(argv, 2);
+		const char *verb = cli.parseDefaultArguments(cli_args.argc, cli_args.argv);
+		ut_assert_true(verb == nullptr);
+	}
+
 	{
 		// Another unknown argument
 		BusCLIArguments cli{true, true};
+		cli.default_i2c_frequency = 11111;
+		cli.default_spi_frequency = 22222;
 		const char *argv[] = { "-x", "start" };
 		CLIArgsHelper cli_args(argv, 2);
 		const char *verb = cli.parseDefaultArguments(cli_args.argc, cli_args.argv);
@@ -170,6 +188,8 @@ bool I2CSPICLITest::test_custom()
 	{
 		// custom argument
 		BusCLIArguments cli{true, true};
+		cli.default_i2c_frequency = 11111;
+		cli.default_spi_frequency = 22222;
 		const char *argv[] = { "start", "-T", "432", "-a", "12"};
 		CLIArgsHelper cli_args(argv, 5);
 		int ch;
@@ -196,6 +216,8 @@ bool I2CSPICLITest::test_custom()
 	{
 		// duplicate argument
 		BusCLIArguments cli{true, true};
+		cli.default_i2c_frequency = 11111;
+		cli.default_spi_frequency = 22222;
 		const char *argv[] = { "start"};
 		CLIArgsHelper cli_args(argv, 1);
 		int ch;